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

[Anand Jain <anand.jain@oracle.com>]

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