David,
Ping on this.
Any idea if the open code as in the patch below [1] is better
or how do we fix it? My proposal about using fsid in the
btrfs_printk() instead of current sb->s_id is another choice.
[1]
[PATCH 1/2] btrfs: open code log helpers in device_list_add()
I think for now make it open code as in [1] so it stands correct.
Thanks, Anand
On 23/1/20 3:22 PM, Anand Jain wrote:
On 1/22/20 11:50 PM, David Sterba wrote:
On Tue, Jan 14, 2020 at 03:21:14PM +0800, Anand Jain wrote:
On 14/1/20 2:54 PM, Qu Wenruo wrote:
On 2020/1/14 下午2:09, Anand Jain wrote:
The first argument to btrfs_printk() wrappers such as
btrfs_warn_in_rcu(), btrfs_info_in_rcu(), etc.. is fs_info, but in
some
context like scan and assembling of the volumes there isn't fs_info
yet,
so those code generally don't use the btrfs_printk() wrappers and it
could could still use NULL but then it would become hard to
distinguish
whether fs_info is NULL for genuine reason or a bug.
So introduce a define NO_FS_INFO to be used instead of NULL so that we
know the code where fs_info isn't initialized and also we have a
consistent logging functions. Thanks.
I'm not sure why this is needed.
Could you give me an example in which NULL is not clear enough?
The first argument in btrfs_info_in_rcu() can be NULL like for example..
btrfs_info_in_rcu(NULL, ..) which then it shall print the prefix..
BTRFS info (device <unknown>):
Lets say due to some bug local copy of the variable fs_info wasn't
initialized then we end up printing the same unknown <unknown>.
So in the context of device_list_add() as there is no fs_info
genuinely and be different from unknown we use
btrfs_info_in_rcu(NO_FS_INFO, ..) to get prefix something like..
BTRFS info (device ...):
With the fixup to set fs_info to NULL on a device that's unmounted, do
we still need the NO_FS_INFO stub?
Its trivial we don't need NO_FS_INFO IMO.
The only difference I can see is a
to print "..." instead of "<unknown>" that I don't find too useful or
improving the output.
Agree. Two ways we can make it better one use open code.
Patch [1] disk that.
[1]
[PATCH 1/2] btrfs: open code log helpers in device_list_add()
two, change btrfs_printk() and all its wrapper to use fs_devices
instead of fs_info. It solves two purposes, device name
does not make sense for logging of volume so will use fsid,
and avoid confusion when the device errors are being reported.
Secondly we could use btrfs_printk() wrappers in the unmounted context.
And added bonus is logging becomes truly consistent.
Let me know if you think this is the right way, will look into it.
My idea about the stub fs info was to avoid any access to fs_info inside
device_list_add in case we can't reliably close the race where scan can
read device::fs_info during mount that sets it up, but as I'm told it's
not a problem anymore.
Using NULL instead of fs_info or open code or using fs_devices
(as discussed above) will solve it.
The analysis about the race needed few more information, so I am
not sure.
My idea is to make it theoretically correct. Using
uninitialized fs_info in device_list_add is wrong.
Thanks, Anand
