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

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

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

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