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

[Nikolay Borisov <nborisov@suse.com>]

On 14.01.20 г. 9: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>.

This is wrong if the local copy variable is not initialized then it will
get random value from stack which will likely crash during deref because
it will most certainly not be NULL.

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