| Message ID | 20220824051208.19924-1-wangyugui@e16-tech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] btrfs: slience the sparse warn of rcu_string | expand |
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>
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.
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
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.
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();