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

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

[Anand Jain]

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.

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

[PATCH 2/4] btrfs: stop using uninitiazlised fs_info in device_list_add()

[Anand Jain]

fs_info is born during mount, and operations before the mount such as
scanning and assembling of the device volume should happen without any
reference to fs_info.

However the patch commit a9261d4125c9 (btrfs: harden agaist duplicate
fsid on scanned devices) used fs_info to call btrfs_warn_in_rcu() and
btrfs_info_in_rcu(), so if fs_info is NULL, the stacked functions which
leads to btrfs_printk() which shall print "unknown" instead of sb->s_id.
Or even might UAF as reported in [1].

So do the right thing, don't use fs_info instead use NO_FS_INFO in
btrfs_warn_in_rcu() and btrfs_info_in_rcu() for the btrfs_printk()
to take care of it properly.

Link:
 [1] https://www.spinics.net/lists/linux-btrfs/msg96524.html
Fixes: a9261d4125c9 (btrfs: harden agaist duplicate fsid on scanned devices)
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6fd90270e2c7..0301c3d693d8 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -889,14 +889,14 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 			if (device->bdev != path_bdev) {
 				bdput(path_bdev);
 				mutex_unlock(&fs_devices->device_list_mutex);
-				btrfs_warn_in_rcu(device->fs_info,
+				btrfs_warn_in_rcu(NO_FS_INFO,
 			"duplicate device fsid:devid for %pU:%llu old:%s new:%s",
 					disk_super->fsid, devid,
 					rcu_str_deref(device->name), path);
 				return ERR_PTR(-EEXIST);
 			}
 			bdput(path_bdev);
-			btrfs_info_in_rcu(device->fs_info,
+			btrfs_info_in_rcu(NO_FS_INFO,
 				"device fsid %pU devid %llu moved old:%s new:%s",
 				disk_super->fsid, devid,
 				rcu_str_deref(device->name), path);
-- 
2.23.0

[PATCH 3/4] btrfs: make the scan logs consistent

[Anand Jain]

Typically we follow, a logging format <parameter> <value> and no ":"
so just follow that here.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
David,
If required you may roll this into the patch 2/4 in this series. I didn't
dare, as there may be some concerns that it isn't relevent there.

 fs/btrfs/volumes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0301c3d693d8..bafc57bc02c8 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -890,14 +890,14 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 				bdput(path_bdev);
 				mutex_unlock(&fs_devices->device_list_mutex);
 				btrfs_warn_in_rcu(NO_FS_INFO,
-			"duplicate device fsid:devid for %pU:%llu old:%s new:%s",
+			"duplicate device fsid %pU devid %llu exisitng %s new %s",
 					disk_super->fsid, devid,
 					rcu_str_deref(device->name), path);
 				return ERR_PTR(-EEXIST);
 			}
 			bdput(path_bdev);
 			btrfs_info_in_rcu(NO_FS_INFO,
-				"device fsid %pU devid %llu moved old:%s new:%s",
+				"device fsid %pU devid %llu moved old %s new %s",
 				disk_super->fsid, devid,
 				rcu_str_deref(device->name), path);
 		}
-- 
2.23.0

[PATCH 4/4] btrfs: use btrfs consistent logging wrappers

[Anand Jain]

Now as we could use the btrfs_info(), btrfs_warn() etc.. in the context
where fs_info is not yet initialized, so replace pr_info with btrfs_info
in device_list_add() for a consistent log format from btrfs.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index bafc57bc02c8..e30747949074 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -824,13 +824,13 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 		*new_device_added = true;
 
 		if (disk_super->label[0])
-			pr_info(
-	"BTRFS: device label %s devid %llu transid %llu %s scanned by %s (%d)\n",
+			btrfs_info(NO_FS_INFO,
+	"device label %s devid %llu transid %llu %s scanned by %s (%d)",
 				disk_super->label, devid, found_transid, path,
 				current->comm, task_pid_nr(current));
 		else
-			pr_info(
-	"BTRFS: device fsid %pU devid %llu transid %llu %s scanned by %s (%d)\n",
+			btrfs_info(NO_FS_INFO,
+	"device fsid %pU devid %llu transid %llu %s scanned by %s (%d)",
 				disk_super->fsid, devid, found_transid, path,
 				current->comm, task_pid_nr(current));
 
-- 
2.23.0

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

[Qu Wenruo]

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



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?

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 --]

Re: [PATCH 2/4] btrfs: stop using uninitiazlised fs_info in device_list_add()

[Qu Wenruo]

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



On 2020/1/14 下午2:09, Anand Jain wrote:
> fs_info is born during mount, and operations before the mount such as
> scanning and assembling of the device volume should happen without any
> reference to fs_info.
> 
> However the patch commit a9261d4125c9 (btrfs: harden agaist duplicate
> fsid on scanned devices) used fs_info to call btrfs_warn_in_rcu() and
> btrfs_info_in_rcu(), so if fs_info is NULL, the stacked functions which
> leads to btrfs_printk() which shall print "unknown" instead of sb->s_id.
> Or even might UAF as reported in [1].

With your previous patch, which already checked NULL pointer, I didn't
see the need for NO_FS_INFO.

Or do you believe this calling site is a special?
If so, I still didn't get the point of NO_FS_INFO, just extra lines
using __func__ or "during scan: xxxxx" looks enough to me.

Thanks,
Qu

> 
> So do the right thing, don't use fs_info instead use NO_FS_INFO in
> btrfs_warn_in_rcu() and btrfs_info_in_rcu() for the btrfs_printk()
> to take care of it properly.
> 
> Link:
>  [1] https://www.spinics.net/lists/linux-btrfs/msg96524.html
> Fixes: a9261d4125c9 (btrfs: harden agaist duplicate fsid on scanned devices)
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 6fd90270e2c7..0301c3d693d8 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -889,14 +889,14 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>  			if (device->bdev != path_bdev) {
>  				bdput(path_bdev);
>  				mutex_unlock(&fs_devices->device_list_mutex);
> -				btrfs_warn_in_rcu(device->fs_info,
> +				btrfs_warn_in_rcu(NO_FS_INFO,
>  			"duplicate device fsid:devid for %pU:%llu old:%s new:%s",
>  					disk_super->fsid, devid,
>  					rcu_str_deref(device->name), path);
>  				return ERR_PTR(-EEXIST);
>  			}
>  			bdput(path_bdev);
> -			btrfs_info_in_rcu(device->fs_info,
> +			btrfs_info_in_rcu(NO_FS_INFO,
>  				"device fsid %pU devid %llu moved old:%s new:%s",
>  				disk_super->fsid, devid,
>  				rcu_str_deref(device->name), path);
> 


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

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

[Anand Jain]

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

Re: [PATCH 2/4] btrfs: stop using uninitiazlised fs_info in device_list_add()

[Nikolay Borisov]

On 14.01.20 г. 8:58 ч., Qu Wenruo wrote:
> 
> 
> On 2020/1/14 下午2:09, Anand Jain wrote:
>> fs_info is born during mount, and operations before the mount such as
>> scanning and assembling of the device volume should happen without any
>> reference to fs_info.
>>
>> However the patch commit a9261d4125c9 (btrfs: harden agaist duplicate
>> fsid on scanned devices) used fs_info to call btrfs_warn_in_rcu() and
>> btrfs_info_in_rcu(), so if fs_info is NULL, the stacked functions which
>> leads to btrfs_printk() which shall print "unknown" instead of sb->s_id.
>> Or even might UAF as reported in [1].
> 
> With your previous patch, which already checked NULL pointer, I didn't
> see the need for NO_FS_INFO.
> 
> Or do you believe this calling site is a special?
> If so, I still didn't get the point of NO_FS_INFO, just extra lines
> using __func__ or "during scan: xxxxx" looks enough to me.

I agree with this assessment. What value does NO_FS_INFO bring in
comparison to plain NULL that it warrants a special case?

> 
> Thanks,
> Qu
> 

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

[Nikolay Borisov]

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

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

[Qu Wenruo]

[-- 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 --]

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

[David Sterba]

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?  The only difference I can see is a
to print "..." instead of "<unknown>" that I don't find too useful or
improving the output.

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.

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

[Anand Jain]

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

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

[Anand Jain]

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