Re: [PATCH 1/4] btrfs: add NO_FS_INFO to btrfs_printk

Anand Jain
2020-02-03T04:06:25Z

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