[PATCH v2] btrfs: slience the sparse warn of rcu_string

[PATCH v2] btrfs: slience the sparse warn of rcu_string

[Wang Yugui]

slience the sparse warn of rcu_string reported by 'make C=1'

warning example:
fs/btrfs/volumes.c:2300:41: warning: incorrect type in argument 3 (different address spaces)
fs/btrfs/volumes.c:2300:41:    expected char const *device_path
fs/btrfs/volumes.c:2300:41:    got char [noderef] __rcu *

Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: Wang Yugui <wangyugui@e16-tech.com>
---
changes since v1:
 add a warn example.
 add 'Reviewed-by: Johannes Thumshirn'.

 fs/btrfs/dev-replace.c |  2 +-
 fs/btrfs/volumes.c     | 14 +++++++-------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 70d001d..b30930e 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -1006,7 +1006,7 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 	btrfs_sysfs_update_devid(tgt_device);
 	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &src_device->dev_state))
 		btrfs_scratch_superblocks(fs_info, src_device->bdev,
-					  src_device->name->str);
+					  rcu_str_deref(src_device->name));
 
 	/* write back the superblocks */
 	trans = btrfs_start_transaction(root, 0);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9481108..2cd261c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -390,7 +390,7 @@ static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid,
 void btrfs_free_device(struct btrfs_device *device)
 {
 	WARN_ON(!list_empty(&device->post_commit_list));
-	rcu_string_free(device->name);
+	rcu_string_free(rcu_dereference(device->name));
 	extent_io_tree_release(&device->alloc_state);
 	btrfs_destroy_dev_zone_info(device);
 	kfree(device);
@@ -640,7 +640,7 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
 	if (!device->name)
 		return -EINVAL;
 
-	ret = btrfs_get_bdev_and_sb(device->name->str, flags, holder, 1,
+	ret = btrfs_get_bdev_and_sb(rcu_str_deref(device->name), flags, holder, 1,
 				    &bdev, &disk_super);
 	if (ret)
 		return ret;
@@ -908,7 +908,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 				disk_super->fsid, devid, found_transid, path,
 				current->comm, task_pid_nr(current));
 
-	} else if (!device->name || strcmp(device->name->str, path)) {
+	} else if (!device->name || strcmp(rcu_str_deref(device->name), path)) {
 		/*
 		 * When FS is already mounted.
 		 * 1. If you are here and if the device->name is NULL that
@@ -978,7 +978,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 			mutex_unlock(&fs_devices->device_list_mutex);
 			return ERR_PTR(-ENOMEM);
 		}
-		rcu_string_free(device->name);
+		rcu_string_free(rcu_dereference(device->name));
 		rcu_assign_pointer(device->name, name);
 		if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state)) {
 			fs_devices->missing_devices--;
@@ -1035,7 +1035,7 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
 		 * uuid mutex so nothing we touch in here is going to disappear.
 		 */
 		if (orig_dev->name) {
-			name = rcu_string_strdup(orig_dev->name->str,
+			name = rcu_string_strdup(rcu_str_deref(orig_dev->name),
 					GFP_KERNEL);
 			if (!name) {
 				btrfs_free_device(device);
@@ -2217,7 +2217,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info,
 	 */
 	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
 		btrfs_scratch_superblocks(fs_info, device->bdev,
-					  device->name->str);
+					  rcu_str_deref(device->name));
 		if (device->bdev) {
 			sync_blockdev(device->bdev);
 			invalidate_bdev(device->bdev);
@@ -2332,7 +2332,7 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_device *tgtdev)
 	mutex_unlock(&fs_devices->device_list_mutex);
 
 	btrfs_scratch_superblocks(tgtdev->fs_info, tgtdev->bdev,
-				  tgtdev->name->str);
+				  rcu_str_deref(tgtdev->name));
 
 	btrfs_close_bdev(tgtdev);
 	synchronize_rcu();
-- 
2.36.2

Re: [PATCH v2] btrfs: slience the sparse warn of rcu_string

[Anand Jain]

On 8/24/22 13:12, Wang Yugui wrote:
> slience the sparse warn of rcu_string reported by 'make C=1'
> 
> warning example:
> fs/btrfs/volumes.c:2300:41: warning: incorrect type in argument 3 (different address spaces)
> fs/btrfs/volumes.c:2300:41:    expected char const *device_path
> fs/btrfs/volumes.c:2300:41:    got char [noderef] __rcu *
> 
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Signed-off-by: Wang Yugui <wangyugui@e16-tech.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Re: [PATCH v2] btrfs: slience the sparse warn of rcu_string

[David Sterba]

On Wed, Aug 24, 2022 at 01:12:08PM +0800, Wang Yugui wrote:
> slience the sparse warn of rcu_string reported by 'make C=1'
> 
> warning example:
> fs/btrfs/volumes.c:2300:41: warning: incorrect type in argument 3 (different address spaces)
> fs/btrfs/volumes.c:2300:41:    expected char const *device_path
> fs/btrfs/volumes.c:2300:41:    got char [noderef] __rcu *
> 
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Signed-off-by: Wang Yugui <wangyugui@e16-tech.com>

With an updated changelog added to misc-next, thanks.

Re: [PATCH v2] btrfs: slience the sparse warn of rcu_string

[Wang Yugui]

Hi,

> slience the sparse warn of rcu_string reported by 'make C=1'
> 
> warning example:
> fs/btrfs/volumes.c:2300:41: warning: incorrect type in argument 3 (different address spaces)
> fs/btrfs/volumes.c:2300:41:    expected char const *device_path
> fs/btrfs/volumes.c:2300:41:    got char [noderef] __rcu *

I'm sorry that some new WARNING is triggered when 
lockdep(CONFIG_PROVE_LOCKING/...) check is enabled.

[  112.742722] =============================
[  112.746835] WARNING: suspicious RCU usage
[  112.750917] 6.0.0-4.0.el7.x86_64 #1 Not tainted
[  112.755540] -----------------------------
[  112.759624] fs/btrfs/volumes.c:918 suspicious rcu_dereference_check() usage!
[  112.766776] 
               other info that might help us debug this:

[  112.774921] 
               rcu_scheduler_active = 2, debug_locks = 1
[  112.781566] 2 locks held by mount/1725:
[  112.785471]  #0: ffffffffc50592a8 (uuid_mutex){+.+.}-{3:3}, at: btrfs_mount_root+0xfe/0x520 [btrfs]
[  112.794763]  #1: ffffa1147bf068e0 (&fs_devs->device_list_mutex){+.+.}-{3:3}, at: device_list_add+0x28d/0x830 [btrfs]


The fix is yet not clear,  we may need rcu_read_lock() just like
3181faa85bda3dc3f5e630a1846526c9caaa38e3 (cfq-iosched: fix a rcu warning)

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2022/09/05

> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Signed-off-by: Wang Yugui <wangyugui@e16-tech.com>
> ---
> changes since v1:
>  add a warn example.
>  add 'Reviewed-by: Johannes Thumshirn'.
> 
>  fs/btrfs/dev-replace.c |  2 +-
>  fs/btrfs/volumes.c     | 14 +++++++-------
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index 70d001d..b30930e 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -1006,7 +1006,7 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>  	btrfs_sysfs_update_devid(tgt_device);
>  	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &src_device->dev_state))
>  		btrfs_scratch_superblocks(fs_info, src_device->bdev,
> -					  src_device->name->str);
> +					  rcu_str_deref(src_device->name));
>  
>  	/* write back the superblocks */
>  	trans = btrfs_start_transaction(root, 0);
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 9481108..2cd261c 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -390,7 +390,7 @@ static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid,
>  void btrfs_free_device(struct btrfs_device *device)
>  {
>  	WARN_ON(!list_empty(&device->post_commit_list));
> -	rcu_string_free(device->name);
> +	rcu_string_free(rcu_dereference(device->name));
>  	extent_io_tree_release(&device->alloc_state);
>  	btrfs_destroy_dev_zone_info(device);
>  	kfree(device);
> @@ -640,7 +640,7 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
>  	if (!device->name)
>  		return -EINVAL;
>  
> -	ret = btrfs_get_bdev_and_sb(device->name->str, flags, holder, 1,
> +	ret = btrfs_get_bdev_and_sb(rcu_str_deref(device->name), flags, holder, 1,
>  				    &bdev, &disk_super);
>  	if (ret)
>  		return ret;
> @@ -908,7 +908,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>  				disk_super->fsid, devid, found_transid, path,
>  				current->comm, task_pid_nr(current));
>  
> -	} else if (!device->name || strcmp(device->name->str, path)) {
> +	} else if (!device->name || strcmp(rcu_str_deref(device->name), path)) {
>  		/*
>  		 * When FS is already mounted.
>  		 * 1. If you are here and if the device->name is NULL that
> @@ -978,7 +978,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>  			mutex_unlock(&fs_devices->device_list_mutex);
>  			return ERR_PTR(-ENOMEM);
>  		}
> -		rcu_string_free(device->name);
> +		rcu_string_free(rcu_dereference(device->name));
>  		rcu_assign_pointer(device->name, name);
>  		if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state)) {
>  			fs_devices->missing_devices--;
> @@ -1035,7 +1035,7 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
>  		 * uuid mutex so nothing we touch in here is going to disappear.
>  		 */
>  		if (orig_dev->name) {
> -			name = rcu_string_strdup(orig_dev->name->str,
> +			name = rcu_string_strdup(rcu_str_deref(orig_dev->name),
>  					GFP_KERNEL);
>  			if (!name) {
>  				btrfs_free_device(device);
> @@ -2217,7 +2217,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info,
>  	 */
>  	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
>  		btrfs_scratch_superblocks(fs_info, device->bdev,
> -					  device->name->str);
> +					  rcu_str_deref(device->name));
>  		if (device->bdev) {
>  			sync_blockdev(device->bdev);
>  			invalidate_bdev(device->bdev);
> @@ -2332,7 +2332,7 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_device *tgtdev)
>  	mutex_unlock(&fs_devices->device_list_mutex);
>  
>  	btrfs_scratch_superblocks(tgtdev->fs_info, tgtdev->bdev,
> -				  tgtdev->name->str);
> +				  rcu_str_deref(tgtdev->name));
>  
>  	btrfs_close_bdev(tgtdev);
>  	synchronize_rcu();
> -- 
> 2.36.2

Re: [PATCH v2] btrfs: slience the sparse warn of rcu_string

[David Sterba]

On Mon, Sep 05, 2022 at 11:57:26AM +0800, Wang Yugui wrote:
> Hi,
> 
> > slience the sparse warn of rcu_string reported by 'make C=1'
> > 
> > warning example:
> > fs/btrfs/volumes.c:2300:41: warning: incorrect type in argument 3 (different address spaces)
> > fs/btrfs/volumes.c:2300:41:    expected char const *device_path
> > fs/btrfs/volumes.c:2300:41:    got char [noderef] __rcu *
> 
> I'm sorry that some new WARNING is triggered when 
> lockdep(CONFIG_PROVE_LOCKING/...) check is enabled.
> 
> [  112.742722] =============================
> [  112.746835] WARNING: suspicious RCU usage
> [  112.750917] 6.0.0-4.0.el7.x86_64 #1 Not tainted
> [  112.755540] -----------------------------
> [  112.759624] fs/btrfs/volumes.c:918 suspicious rcu_dereference_check() usage!
> [  112.766776] 
>                other info that might help us debug this:
> 
> [  112.774921] 
>                rcu_scheduler_active = 2, debug_locks = 1
> [  112.781566] 2 locks held by mount/1725:
> [  112.785471]  #0: ffffffffc50592a8 (uuid_mutex){+.+.}-{3:3}, at: btrfs_mount_root+0xfe/0x520 [btrfs]
> [  112.794763]  #1: ffffa1147bf068e0 (&fs_devs->device_list_mutex){+.+.}-{3:3}, at: device_list_add+0x28d/0x830 [btrfs]
> 
> 
> The fix is yet not clear,  we may need rcu_read_lock() just like
> 3181faa85bda3dc3f5e630a1846526c9caaa38e3 (cfq-iosched: fix a rcu warning)

There are probably more instances that would need the rcu lock/unlock,
eg. in btrfs_open_one_device, btrfs_dev_replace_finishing,
btrfs_destroy_dev_replace_tgtdev, btrfs_rm_device, clone_fs_devices,
device_list_add, btrfs_open_one_device where the dereference is done in
parameter list.

There's one dereference in the message helpers but not using the _in_rcu
variant:

zoned.c:
1403                 case BLK_ZONE_COND_READONLY:
1404                         btrfs_err(fs_info,
1405                 "zoned: offline/readonly zone %llu on device %s (devid %llu)",
1406                                   physical[i] >> device->zone_info->zone_size_shift,
1407                                   rcu_str_deref(device->name), device->devid);

We can keep the the patch in for-next and add the missing annotations as
they appear, updating the patch so it may take some time but we'll fix
all the warnings eventually.