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

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

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