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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3772 bytes --]



On 2020/1/14 下午3:21, 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>.

Then that's a bug to fix.

> 
>   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 ...):

But that's the only context we don't have an initialized fs_info.

I don't like the idea to use some random pointer as a special indicator
only for 1 or 2 call sites.

And if you don't like the "<unknown>" string, unifing them to "..."
looks better to me.

Thanks,
Qu

> 
> Thanks, Anand
> 
> 
>> Thanks,
>> Qu
>>
>>>
>>> Suggested-by: David Sterba <dsterba@suse.com>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>>   fs/btrfs/ctree.h |  5 +++++
>>>   fs/btrfs/super.c | 14 +++++++++++---
>>>   2 files changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>>> index 569931dd0ce5..625c7eee3d0f 100644
>>> --- a/fs/btrfs/ctree.h
>>> +++ b/fs/btrfs/ctree.h
>>> @@ -110,6 +110,11 @@ struct btrfs_ref;
>>>   #define BTRFS_STAT_CURR        0
>>>   #define BTRFS_STAT_PREV        1
>>>   +/*
>>> + * Used when we know that fs_info is not yet initialized.
>>> + */
>>> +#define    NO_FS_INFO    ((void *)0x1)
>>> +
>>>   /*
>>>    * Count how many BTRFS_MAX_EXTENT_SIZE cover the @size
>>>    */
>>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>>> index a906315efd19..5bd8a889fed0 100644
>>> --- a/fs/btrfs/super.c
>>> +++ b/fs/btrfs/super.c
>>> @@ -216,9 +216,17 @@ void __cold btrfs_printk(const struct
>>> btrfs_fs_info *fs_info, const char *fmt, .
>>>       vaf.fmt = fmt;
>>>       vaf.va = &args;
>>>   -    if (__ratelimit(ratelimit))
>>> -        printk("%sBTRFS %s (device %s): %pV\n", lvl, type,
>>> -            fs_info ? fs_info->sb->s_id : "<unknown>", &vaf);
>>> +    if (__ratelimit(ratelimit)) {
>>> +        if (fs_info == NULL)
>>> +            printk("%sBTRFS %s (device %s): %pV\n", lvl, type,
>>> +                "<unknown>", &vaf);
>>> +        else if (fs_info == NO_FS_INFO)
>>> +            printk("%sBTRFS %s (device %s): %pV\n", lvl, type,
>>> +                "...", &vaf);
>>> +        else
>>> +            printk("%sBTRFS %s (device %s): %pV\n", lvl, type,
>>> +                fs_info->sb->s_id, &vaf);
>>> +    }
>>>         va_end(args);
>>>   }
>>>
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]