* [PATCH 0/3] btrf_show_devname related fixes @ 2021-08-19 18:18 Anand Jain 2021-08-19 18:18 ` [PATCH 1/3] btrfs: fix comment about the btrfs_show_devname Anand Jain ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Anand Jain @ 2021-08-19 18:18 UTC (permalink / raw) To: linux-btrfs; +Cc: dsterba, l Related to the bug/fix posted to the mailing list as below btrfs: traverse seed devices if fs_devices::devices is empty in show_devname Anand Jain (3): btrfs: fix comment about the btrfs_show_devname btrfs: consolidate device_list_mutex in prepare_sprout to its parent btrfs: use latest_bdev in btrfs_show_devname fs/btrfs/super.c | 25 +++---------------------- fs/btrfs/volumes.c | 11 ++++------- 2 files changed, 7 insertions(+), 29 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] btrfs: fix comment about the btrfs_show_devname 2021-08-19 18:18 [PATCH 0/3] btrf_show_devname related fixes Anand Jain @ 2021-08-19 18:18 ` Anand Jain 2021-08-19 18:18 ` [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent Anand Jain 2021-08-19 18:18 ` [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname Anand Jain 2 siblings, 0 replies; 15+ messages in thread From: Anand Jain @ 2021-08-19 18:18 UTC (permalink / raw) To: linux-btrfs; +Cc: dsterba, l There were few lock dep warnings because btrfs_show_devname() was using device_list_mutex as recorded in the commits ccd05285e7f (btrfs: fix a possible umount deadlock) 779bf3fefa8 (btrfs: fix lock dep warning, move scratch dev out of device_list_mutex and uuid_mutex) And finally, commit 88c14590cdd6 (btrfs: use RCU in btrfs_show_devname for device list traversal) removed the device_list_mutex from btrfs_show_devname for performance reasons. This patch fixes a stale comment about the function btrfs_show_devname. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/volumes.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 9cb4ed90888d..91b8422b3f67 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -2278,10 +2278,7 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_device *tgtdev) /* * The update_dev_time() with in btrfs_scratch_superblocks() - * may lead to a call to btrfs_show_devname() which will try - * to hold device_list_mutex. And here this device - * is already out of device list, so we don't have to hold - * the device_list_mutex lock. + * may lead to a call to btrfs_show_devname(). */ btrfs_scratch_superblocks(tgtdev->fs_info, tgtdev->bdev, tgtdev->name->str); -- 2.31.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent 2021-08-19 18:18 [PATCH 0/3] btrf_show_devname related fixes Anand Jain 2021-08-19 18:18 ` [PATCH 1/3] btrfs: fix comment about the btrfs_show_devname Anand Jain @ 2021-08-19 18:18 ` Anand Jain 2021-08-20 7:51 ` Su Yue 2021-08-19 18:18 ` [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname Anand Jain 2 siblings, 1 reply; 15+ messages in thread From: Anand Jain @ 2021-08-19 18:18 UTC (permalink / raw) To: linux-btrfs; +Cc: dsterba, l btrfs_prepare_sprout() moves seed devices into its own struct fs_devices, so that its parent function btrfs_init_new_device() can add the new sprout device to fs_info->fs_devices. Both btrfs_prepare_sprout() and btrfs_init_new_device() needs device_list_mutex. But they are holding it sequentially, thus creates a small window to an opportunity to race. Close this opportunity and hold device_list_mutex common to both btrfs_init_new_device() and btrfs_prepare_sprout(). Signed-off-by: Anand Jain <anand.jain@oracle.com> --- RFC because I haven't identified the other thread which could race with this, but still does this cleanup makes sense? fs/btrfs/volumes.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 91b8422b3f67..f490d1897c56 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -2366,6 +2366,8 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) u64 super_flags; lockdep_assert_held(&uuid_mutex); + lockdep_assert_held(&fs_devices->device_list_mutex); + if (!fs_devices->seeding) return -EINVAL; @@ -2397,7 +2399,6 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) INIT_LIST_HEAD(&seed_devices->alloc_list); mutex_init(&seed_devices->device_list_mutex); - mutex_lock(&fs_devices->device_list_mutex); list_splice_init_rcu(&fs_devices->devices, &seed_devices->devices, synchronize_rcu); list_for_each_entry(device, &seed_devices->devices, dev_list) @@ -2413,7 +2414,6 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) generate_random_uuid(fs_devices->fsid); memcpy(fs_devices->metadata_uuid, fs_devices->fsid, BTRFS_FSID_SIZE); memcpy(disk_super->fsid, fs_devices->fsid, BTRFS_FSID_SIZE); - mutex_unlock(&fs_devices->device_list_mutex); super_flags = btrfs_super_flags(disk_super) & ~BTRFS_SUPER_FLAG_SEEDING; @@ -2588,6 +2588,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path device->dev_stats_valid = 1; set_blocksize(device->bdev, BTRFS_BDEV_BLOCKSIZE); + mutex_lock(&fs_devices->device_list_mutex); if (seeding_dev) { btrfs_clear_sb_rdonly(sb); ret = btrfs_prepare_sprout(fs_info); @@ -2599,7 +2600,6 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path device->fs_devices = fs_devices; - mutex_lock(&fs_devices->device_list_mutex); mutex_lock(&fs_info->chunk_mutex); list_add_rcu(&device->dev_list, &fs_devices->devices); list_add(&device->dev_alloc_list, &fs_devices->alloc_list); -- 2.31.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent 2021-08-19 18:18 ` [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent Anand Jain @ 2021-08-20 7:51 ` Su Yue 2021-08-20 8:53 ` Anand Jain 0 siblings, 1 reply; 15+ messages in thread From: Su Yue @ 2021-08-20 7:51 UTC (permalink / raw) To: Anand Jain; +Cc: linux-btrfs, dsterba On Fri 20 Aug 2021 at 02:18, Anand Jain <anand.jain@oracle.com> wrote: > btrfs_prepare_sprout() moves seed devices into its own struct > fs_devices, > so that its parent function btrfs_init_new_device() can add the > new sprout > device to fs_info->fs_devices. > > Both btrfs_prepare_sprout() and btrfs_init_new_device() needs > device_list_mutex. But they are holding it sequentially, thus > creates a > small window to an opportunity to race. Close this opportunity > and hold > device_list_mutex common to both btrfs_init_new_device() and > btrfs_prepare_sprout(). > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > RFC because I haven't identified the other thread which could > race with > this, but still does this cleanup makes sense? > > fs/btrfs/volumes.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 91b8422b3f67..f490d1897c56 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -2366,6 +2366,8 @@ static int btrfs_prepare_sprout(struct > btrfs_fs_info *fs_info) > u64 super_flags; > > lockdep_assert_held(&uuid_mutex); > + lockdep_assert_held(&fs_devices->device_list_mutex); > + > Just a reminder: clone_fs_devices() still takes the mutex in misc-next. > if (!fs_devices->seeding) > return -EINVAL; > > @@ -2397,7 +2399,6 @@ static int btrfs_prepare_sprout(struct > btrfs_fs_info *fs_info) > INIT_LIST_HEAD(&seed_devices->alloc_list); > mutex_init(&seed_devices->device_list_mutex); > > - mutex_lock(&fs_devices->device_list_mutex); > list_splice_init_rcu(&fs_devices->devices, > &seed_devices->devices, > synchronize_rcu); > list_for_each_entry(device, &seed_devices->devices, dev_list) > @@ -2413,7 +2414,6 @@ static int btrfs_prepare_sprout(struct > btrfs_fs_info *fs_info) > generate_random_uuid(fs_devices->fsid); > memcpy(fs_devices->metadata_uuid, fs_devices->fsid, > BTRFS_FSID_SIZE); > memcpy(disk_super->fsid, fs_devices->fsid, BTRFS_FSID_SIZE); > - mutex_unlock(&fs_devices->device_list_mutex); > > super_flags = btrfs_super_flags(disk_super) & > ~BTRFS_SUPER_FLAG_SEEDING; > @@ -2588,6 +2588,7 @@ int btrfs_init_new_device(struct > btrfs_fs_info *fs_info, const char *device_path > device->dev_stats_valid = 1; > set_blocksize(device->bdev, BTRFS_BDEV_BLOCKSIZE); > > + mutex_lock(&fs_devices->device_list_mutex); > if (seeding_dev) { > btrfs_clear_sb_rdonly(sb); > ret = btrfs_prepare_sprout(fs_info); > the erorr case: if (ret) { mutex_unlock(&fs_devices->device_list_mutex); ... } Thanks. -- Su > @@ -2599,7 +2600,6 @@ int btrfs_init_new_device(struct > btrfs_fs_info *fs_info, const char *device_path > > device->fs_devices = fs_devices; > > - mutex_lock(&fs_devices->device_list_mutex); > mutex_lock(&fs_info->chunk_mutex); > list_add_rcu(&device->dev_list, &fs_devices->devices); > list_add(&device->dev_alloc_list, &fs_devices->alloc_list); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent 2021-08-20 7:51 ` Su Yue @ 2021-08-20 8:53 ` Anand Jain 2021-08-21 14:57 ` Su Yue 0 siblings, 1 reply; 15+ messages in thread From: Anand Jain @ 2021-08-20 8:53 UTC (permalink / raw) To: Su Yue; +Cc: linux-btrfs, dsterba >> @@ -2366,6 +2366,8 @@ static int btrfs_prepare_sprout(struct >> btrfs_fs_info *fs_info) >> u64 super_flags; >> >> lockdep_assert_held(&uuid_mutex); >> + lockdep_assert_held(&fs_devices->device_list_mutex); >> + >> > Just a reminder: clone_fs_devices() still takes the mutex in misc-next. As I am checking clone_fs_devices() does not take any lock. Could you pls recheck? >> @@ -2588,6 +2588,7 @@ int btrfs_init_new_device(struct btrfs_fs_info >> *fs_info, const char *device_path >> device->dev_stats_valid = 1; >> set_blocksize(device->bdev, BTRFS_BDEV_BLOCKSIZE); >> >> + mutex_lock(&fs_devices->device_list_mutex); >> if (seeding_dev) { >> btrfs_clear_sb_rdonly(sb); >> ret = btrfs_prepare_sprout(fs_info); >> > the erorr case: > if (ret) { > mutex_unlock(&fs_devices->device_list_mutex); > ... > } Right. I missed it will fix. Thanks. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent 2021-08-20 8:53 ` Anand Jain @ 2021-08-21 14:57 ` Su Yue 2021-08-21 15:00 ` Su Yue 0 siblings, 1 reply; 15+ messages in thread From: Su Yue @ 2021-08-21 14:57 UTC (permalink / raw) To: Anand Jain; +Cc: linux-btrfs, dsterba On Fri 20 Aug 2021 at 16:53, Anand Jain <anand.jain@oracle.com> wrote: >>> @@ -2366,6 +2366,8 @@ static int btrfs_prepare_sprout(struct >>> btrfs_fs_info *fs_info) >>> u64 super_flags; >>> >>> lockdep_assert_held(&uuid_mutex); >>> + lockdep_assert_held(&fs_devices->device_list_mutex); >>> + >>> >> Just a reminder: clone_fs_devices() still takes the mutex in >> misc-next. > As I am checking clone_fs_devices() does not take any lock. > Could you pls recheck? > Hmmmm... misc-next: https://github.com/kdave/btrfs-devel/blob/e05983195f31374ad51a0f3712efec381397f3cb/fs/btrfs/volumes.c#L381 -- Su > >>> @@ -2588,6 +2588,7 @@ int btrfs_init_new_device(struct >>> btrfs_fs_info >>> *fs_info, const char *device_path >>> device->dev_stats_valid = 1; >>> set_blocksize(device->bdev, BTRFS_BDEV_BLOCKSIZE); >>> >>> + mutex_lock(&fs_devices->device_list_mutex); >>> if (seeding_dev) { >>> btrfs_clear_sb_rdonly(sb); >>> ret = btrfs_prepare_sprout(fs_info); >>> >> the erorr case: >> if (ret) { >> mutex_unlock(&fs_devices->device_list_mutex); >> ... >> } > > Right. I missed it will fix. > > Thanks. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent 2021-08-21 14:57 ` Su Yue @ 2021-08-21 15:00 ` Su Yue 2021-08-23 10:34 ` Anand Jain 0 siblings, 1 reply; 15+ messages in thread From: Su Yue @ 2021-08-21 15:00 UTC (permalink / raw) To: Anand Jain; +Cc: linux-btrfs, dsterba On Sat 21 Aug 2021 at 22:57, Su Yue <l@damenly.su> wrote: > On Fri 20 Aug 2021 at 16:53, Anand Jain <anand.jain@oracle.com> > wrote: > >>>> @@ -2366,6 +2366,8 @@ static int btrfs_prepare_sprout(struct >>>> btrfs_fs_info *fs_info) >>>> u64 super_flags; >>>> >>>> lockdep_assert_held(&uuid_mutex); >>>> + lockdep_assert_held(&fs_devices->device_list_mutex); >>>> + >>>> >>> Just a reminder: clone_fs_devices() still takes the mutex in >>> misc-next. >> As I am checking clone_fs_devices() does not take any lock. >> Could you pls recheck? >> > > Hmmmm... misc-next: > > https://github.com/kdave/btrfs-devel/blob/e05983195f31374ad51a0f3712efec381397f3cb/fs/btrfs/volumes.c#L381 Sorry, it should be https://github.com/kdave/btrfs-devel/blob/misc-next/fs/btrfs/volumes.c#L995 -- Su ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent 2021-08-21 15:00 ` Su Yue @ 2021-08-23 10:34 ` Anand Jain 2021-08-23 10:54 ` Anand Jain 0 siblings, 1 reply; 15+ messages in thread From: Anand Jain @ 2021-08-23 10:34 UTC (permalink / raw) To: Su Yue; +Cc: linux-btrfs, dsterba On 21/08/2021 23:00, Su Yue wrote: > > On Sat 21 Aug 2021 at 22:57, Su Yue <l@damenly.su> wrote: > >> On Fri 20 Aug 2021 at 16:53, Anand Jain <anand.jain@oracle.com> >> wrote: >> >>>>> @@ -2366,6 +2366,8 @@ static int btrfs_prepare_sprout(struct >>>>> btrfs_fs_info *fs_info) >>>>> u64 super_flags; >>>>> >>>>> lockdep_assert_held(&uuid_mutex); >>>>> + lockdep_assert_held(&fs_devices->device_list_mutex); >>>>> + >>>>> >>>> Just a reminder: clone_fs_devices() still takes the mutex in >>>> misc-next. >>> As I am checking clone_fs_devices() does not take any lock. >>> Could you pls recheck? >>> >> >> Hmmmm... misc-next: >> >> https://github.com/kdave/btrfs-devel/blob/e05983195f31374ad51a0f3712efec381397f3cb/fs/btrfs/volumes.c#L381 >> > > Sorry, it should be > > https://github.com/kdave/btrfs-devel/blob/misc-next/fs/btrfs/volumes.c#L995 Some of the Git commands stopped working. I had to run git fsck. Now I see mutex in close_fs_devices(), not sure if I was blind to it before. Anyway, this is a bad patch. I am working to fix it. Thanks, Anand > > -- > Su ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent 2021-08-23 10:34 ` Anand Jain @ 2021-08-23 10:54 ` Anand Jain 2021-08-23 12:20 ` David Sterba 0 siblings, 1 reply; 15+ messages in thread From: Anand Jain @ 2021-08-23 10:54 UTC (permalink / raw) To: Su Yue; +Cc: linux-btrfs, dsterba On 23/08/2021 18:34, Anand Jain wrote: > On 21/08/2021 23:00, Su Yue wrote: >> >> On Sat 21 Aug 2021 at 22:57, Su Yue <l@damenly.su> wrote: >> >>> On Fri 20 Aug 2021 at 16:53, Anand Jain <anand.jain@oracle.com> >>> wrote: >>> >>>>>> @@ -2366,6 +2366,8 @@ static int btrfs_prepare_sprout(struct >>>>>> btrfs_fs_info *fs_info) >>>>>> u64 super_flags; >>>>>> >>>>>> lockdep_assert_held(&uuid_mutex); >>>>>> + lockdep_assert_held(&fs_devices->device_list_mutex); >>>>>> + >>>>>> >>>>> Just a reminder: clone_fs_devices() still takes the mutex in >>>>> misc-next. >>>> As I am checking clone_fs_devices() does not take any lock. >>>> Could you pls recheck? >>>> >>> >>> Hmmmm... misc-next: >>> >>> https://github.com/kdave/btrfs-devel/blob/e05983195f31374ad51a0f3712efec381397f3cb/fs/btrfs/volumes.c#L381 >>> >> >> Sorry, it should be >> >> https://github.com/kdave/btrfs-devel/blob/misc-next/fs/btrfs/volumes.c#L995 >> > > > Some of the Git commands stopped working. I had to run git fsck. > Now I see mutex in close_fs_devices(), not sure if I was blind to it > before. > Anyway, this is a bad patch. I am working to fix it. > Git errors were distracting. But why I didn't see the device_list_mutex is because I had the following patch [1] in my workspace, which is not yet integrated to the misc-next / commented. [1] btrfs: fix lockdep warning while mounting sprout fs https://lore.kernel.org/linux-btrfs/8325002d-f8a6-a9b7-a27d-4cf62cbbc672@oracle.com/ So as of now, I can say this patch needs the above patch. Could you please apply the above patch before this patch for tests? Thanks, Anand ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent 2021-08-23 10:54 ` Anand Jain @ 2021-08-23 12:20 ` David Sterba 0 siblings, 0 replies; 15+ messages in thread From: David Sterba @ 2021-08-23 12:20 UTC (permalink / raw) To: Anand Jain; +Cc: Su Yue, linux-btrfs, dsterba On Mon, Aug 23, 2021 at 06:54:04PM +0800, Anand Jain wrote: > > > On 23/08/2021 18:34, Anand Jain wrote: > > On 21/08/2021 23:00, Su Yue wrote: > >> > >> On Sat 21 Aug 2021 at 22:57, Su Yue <l@damenly.su> wrote: > >> > >>> On Fri 20 Aug 2021 at 16:53, Anand Jain <anand.jain@oracle.com> > >>> wrote: > >>> > >>>>>> @@ -2366,6 +2366,8 @@ static int btrfs_prepare_sprout(struct > >>>>>> btrfs_fs_info *fs_info) > >>>>>> u64 super_flags; > >>>>>> > >>>>>> lockdep_assert_held(&uuid_mutex); > >>>>>> + lockdep_assert_held(&fs_devices->device_list_mutex); > >>>>>> + > >>>>>> > >>>>> Just a reminder: clone_fs_devices() still takes the mutex in > >>>>> misc-next. > >>>> As I am checking clone_fs_devices() does not take any lock. > >>>> Could you pls recheck? > >>>> > >>> > >>> Hmmmm... misc-next: > >>> > >>> https://github.com/kdave/btrfs-devel/blob/e05983195f31374ad51a0f3712efec381397f3cb/fs/btrfs/volumes.c#L381 > >>> > >> > >> Sorry, it should be > >> > >> https://github.com/kdave/btrfs-devel/blob/misc-next/fs/btrfs/volumes.c#L995 > >> > > > > > > Some of the Git commands stopped working. I had to run git fsck. > > Now I see mutex in close_fs_devices(), not sure if I was blind to it > > before. > > Anyway, this is a bad patch. I am working to fix it. > > > > Git errors were distracting. But why I didn't see the > device_list_mutex is because I had the following patch [1] in my > workspace, which is not yet integrated to the misc-next / commented. > > [1] btrfs: fix lockdep warning while mounting sprout fs > > https://lore.kernel.org/linux-btrfs/8325002d-f8a6-a9b7-a27d-4cf62cbbc672@oracle.com/ > > So as of now, I can say this patch needs the above patch. > > Could you please apply the above patch before this patch for tests? So if there's a dependency I'd apply the whole series. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname 2021-08-19 18:18 [PATCH 0/3] btrf_show_devname related fixes Anand Jain 2021-08-19 18:18 ` [PATCH 1/3] btrfs: fix comment about the btrfs_show_devname Anand Jain 2021-08-19 18:18 ` [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent Anand Jain @ 2021-08-19 18:18 ` Anand Jain 2021-08-20 7:31 ` Su Yue 2021-08-20 10:57 ` David Sterba 2 siblings, 2 replies; 15+ messages in thread From: Anand Jain @ 2021-08-19 18:18 UTC (permalink / raw) To: linux-btrfs; +Cc: dsterba, l latest_bdev is updated according to the changes to the device list. That means we could use the latest_bdev to show the device name in /proc/self/mounts. So this patch makes that change. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- RFC because 1. latest_bdev might not be the lowest devid but, we showed the lowest devid in /proc/self/mount. 2. The device's path is not shown now but, previously we did. So does these break ABI? Maybe yes for 2 howabout for 1 above? fs/btrfs/super.c | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 1f9dd1a4faa3..4ad3fe174c41 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -2464,30 +2464,11 @@ static int btrfs_unfreeze(struct super_block *sb) static int btrfs_show_devname(struct seq_file *m, struct dentry *root) { struct btrfs_fs_info *fs_info = btrfs_sb(root->d_sb); - struct btrfs_device *dev, *first_dev = NULL; + char name[BDEVNAME_SIZE]; - /* - * Lightweight locking of the devices. We should not need - * device_list_mutex here as we only read the device data and the list - * is protected by RCU. Even if a device is deleted during the list - * traversals, we'll get valid data, the freeing callback will wait at - * least until the rcu_read_unlock. - */ - rcu_read_lock(); - list_for_each_entry_rcu(dev, &fs_info->fs_devices->devices, dev_list) { - if (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state)) - continue; - if (!dev->name) - continue; - if (!first_dev || dev->devid < first_dev->devid) - first_dev = dev; - } + seq_escape(m, bdevname(fs_info->fs_devices->latest_bdev, name), + " \t\n\\"); - if (first_dev) - seq_escape(m, rcu_str_deref(first_dev->name), " \t\n\\"); - else - WARN_ON(1); - rcu_read_unlock(); return 0; } -- 2.31.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname 2021-08-19 18:18 ` [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname Anand Jain @ 2021-08-20 7:31 ` Su Yue 2021-08-20 9:13 ` Anand Jain 2021-08-20 10:57 ` David Sterba 1 sibling, 1 reply; 15+ messages in thread From: Su Yue @ 2021-08-20 7:31 UTC (permalink / raw) To: Anand Jain; +Cc: linux-btrfs, dsterba On Fri 20 Aug 2021 at 02:18, Anand Jain <anand.jain@oracle.com> wrote: > latest_bdev is updated according to the changes to the device > list. > That means we could use the latest_bdev to show the device name > in > /proc/self/mounts. So this patch makes that change. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > > RFC because > 1. latest_bdev might not be the lowest devid but, we showed > the lowest devid in /proc/self/mount. > 2. The device's path is not shown now but, previously we did. > So does these break ABI? Maybe yes for 2 howabout for 1 above? > Mabybe a naive question but I have no time to dive into btrfs code recently. If a device which has highest devid was replaced, will the new device be the fs_devices->latest_bdev instead of the existing older one? Thanks. -- Su > fs/btrfs/super.c | 25 +++---------------------- > 1 file changed, 3 insertions(+), 22 deletions(-) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index 1f9dd1a4faa3..4ad3fe174c41 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -2464,30 +2464,11 @@ static int btrfs_unfreeze(struct > super_block *sb) > static int btrfs_show_devname(struct seq_file *m, struct dentry > *root) > { > struct btrfs_fs_info *fs_info = btrfs_sb(root->d_sb); > - struct btrfs_device *dev, *first_dev = NULL; > + char name[BDEVNAME_SIZE]; > > - /* > - * Lightweight locking of the devices. We should not need > - * device_list_mutex here as we only read the device data and > the list > - * is protected by RCU. Even if a device is deleted during > the list > - * traversals, we'll get valid data, the freeing callback will > wait at > - * least until the rcu_read_unlock. > - */ > - rcu_read_lock(); > - list_for_each_entry_rcu(dev, &fs_info->fs_devices->devices, > dev_list) { > - if (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state)) > - continue; > - if (!dev->name) > - continue; > - if (!first_dev || dev->devid < first_dev->devid) > - first_dev = dev; > - } > + seq_escape(m, bdevname(fs_info->fs_devices->latest_bdev, > name), > + " \t\n\\"); > > - if (first_dev) > - seq_escape(m, rcu_str_deref(first_dev->name), " \t\n\\"); > - else > - WARN_ON(1); > - rcu_read_unlock(); > return 0; > } ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname 2021-08-20 7:31 ` Su Yue @ 2021-08-20 9:13 ` Anand Jain 0 siblings, 0 replies; 15+ messages in thread From: Anand Jain @ 2021-08-20 9:13 UTC (permalink / raw) To: Su Yue; +Cc: linux-btrfs, dsterba On 20/08/2021 15:31, Su Yue wrote: > > On Fri 20 Aug 2021 at 02:18, Anand Jain <anand.jain@oracle.com> wrote: > >> latest_bdev is updated according to the changes to the device list. >> That means we could use the latest_bdev to show the device name in >> /proc/self/mounts. So this patch makes that change. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> >> RFC because >> 1. latest_bdev might not be the lowest devid but, we showed >> the lowest devid in /proc/self/mount. >> 2. The device's path is not shown now but, previously we did. >> So does these break ABI? Maybe yes for 2 howabout for 1 above? In v2 I am fix the path part. By replacing latest_bdev to latest_dev which means it shall hold the pointer to btrfs_device instead of just to its bdev. > Mabybe a naive question but I have no time to dive into btrfs code > recently. If a device which has highest devid was replaced, will the > new device be the fs_devices->latest_bdev instead of the existing older > one? It is handled by btrfs_assign_next_active_device(). If the replace source device is also the latest_dev then after replace it shall point to the replace target device. So the new device path is seen in /proc/self/mounts. Which is fine and normal to expect. Similarly btrfs_assign_next_active_device() is called during remove device as well. There is a bug that we didn't update the latest_bdev when we sprout I am fixing this as well in v2. Thanks. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname 2021-08-19 18:18 ` [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname Anand Jain 2021-08-20 7:31 ` Su Yue @ 2021-08-20 10:57 ` David Sterba 2021-08-20 11:03 ` Anand Jain 1 sibling, 1 reply; 15+ messages in thread From: David Sterba @ 2021-08-20 10:57 UTC (permalink / raw) To: Anand Jain; +Cc: linux-btrfs, dsterba, l On Fri, Aug 20, 2021 at 02:18:14AM +0800, Anand Jain wrote: > latest_bdev is updated according to the changes to the device list. > That means we could use the latest_bdev to show the device name in > /proc/self/mounts. So this patch makes that change. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > > RFC because > 1. latest_bdev might not be the lowest devid but, we showed > the lowest devid in /proc/self/mount. > 2. The device's path is not shown now but, previously we did. > So does these break ABI? Maybe yes for 2 howabout for 1 above? The path needs to be preserved, that would break a lot of things.. > fs/btrfs/super.c | 25 +++---------------------- > 1 file changed, 3 insertions(+), 22 deletions(-) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index 1f9dd1a4faa3..4ad3fe174c41 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -2464,30 +2464,11 @@ static int btrfs_unfreeze(struct super_block *sb) > static int btrfs_show_devname(struct seq_file *m, struct dentry *root) > { > struct btrfs_fs_info *fs_info = btrfs_sb(root->d_sb); > - struct btrfs_device *dev, *first_dev = NULL; > + char name[BDEVNAME_SIZE]; > > - /* > - * Lightweight locking of the devices. We should not need > - * device_list_mutex here as we only read the device data and the list > - * is protected by RCU. Even if a device is deleted during the list > - * traversals, we'll get valid data, the freeing callback will wait at > - * least until the rcu_read_unlock. > - */ > - rcu_read_lock(); > - list_for_each_entry_rcu(dev, &fs_info->fs_devices->devices, dev_list) { > - if (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state)) > - continue; > - if (!dev->name) > - continue; > - if (!first_dev || dev->devid < first_dev->devid) > - first_dev = dev; > - } > + seq_escape(m, bdevname(fs_info->fs_devices->latest_bdev, name), > + " \t\n\\"); No protection at all? So what if latest_bdev or latest_dev gets updated in parallel with devicre remove and there's a window where the pointer is invalid but still is accessed. The whole point of RCU section here is to prevent that. > > - if (first_dev) > - seq_escape(m, rcu_str_deref(first_dev->name), " \t\n\\"); > - else > - WARN_ON(1); > - rcu_read_unlock(); > return 0; > } > > -- > 2.31.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname 2021-08-20 10:57 ` David Sterba @ 2021-08-20 11:03 ` Anand Jain 0 siblings, 0 replies; 15+ messages in thread From: Anand Jain @ 2021-08-20 11:03 UTC (permalink / raw) To: dsterba, linux-btrfs, dsterba, l On 20/08/2021 18:57, David Sterba wrote: > On Fri, Aug 20, 2021 at 02:18:14AM +0800, Anand Jain wrote: >> latest_bdev is updated according to the changes to the device list. >> That means we could use the latest_bdev to show the device name in >> /proc/self/mounts. So this patch makes that change. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> >> RFC because >> 1. latest_bdev might not be the lowest devid but, we showed >> the lowest devid in /proc/self/mount. >> 2. The device's path is not shown now but, previously we did. >> So does these break ABI? Maybe yes for 2 howabout for 1 above? > > The path needs to be preserved, that would break a lot of things.. v2 fixed it. > >> fs/btrfs/super.c | 25 +++---------------------- >> 1 file changed, 3 insertions(+), 22 deletions(-) >> >> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c >> index 1f9dd1a4faa3..4ad3fe174c41 100644 >> --- a/fs/btrfs/super.c >> +++ b/fs/btrfs/super.c >> @@ -2464,30 +2464,11 @@ static int btrfs_unfreeze(struct super_block *sb) >> static int btrfs_show_devname(struct seq_file *m, struct dentry *root) >> { >> struct btrfs_fs_info *fs_info = btrfs_sb(root->d_sb); >> - struct btrfs_device *dev, *first_dev = NULL; >> + char name[BDEVNAME_SIZE]; >> >> - /* >> - * Lightweight locking of the devices. We should not need >> - * device_list_mutex here as we only read the device data and the list >> - * is protected by RCU. Even if a device is deleted during the list >> - * traversals, we'll get valid data, the freeing callback will wait at >> - * least until the rcu_read_unlock. >> - */ >> - rcu_read_lock(); >> - list_for_each_entry_rcu(dev, &fs_info->fs_devices->devices, dev_list) { >> - if (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state)) >> - continue; >> - if (!dev->name) >> - continue; >> - if (!first_dev || dev->devid < first_dev->devid) >> - first_dev = dev; >> - } >> + seq_escape(m, bdevname(fs_info->fs_devices->latest_bdev, name), >> + " \t\n\\"); > > No protection at all? So what if latest_bdev or latest_dev gets updated > in parallel with devicre remove and there's a window where the pointer > is invalid but still is accessed. The whole point of RCU section here is > to prevent that. > YEsh. I noticed it. I am just on it. Will fix in v3. >> >> - if (first_dev) >> - seq_escape(m, rcu_str_deref(first_dev->name), " \t\n\\"); >> - else >> - WARN_ON(1); >> - rcu_read_unlock(); >> return 0; >> } >> >> -- >> 2.31.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-08-23 12:23 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-19 18:18 [PATCH 0/3] btrf_show_devname related fixes Anand Jain 2021-08-19 18:18 ` [PATCH 1/3] btrfs: fix comment about the btrfs_show_devname Anand Jain 2021-08-19 18:18 ` [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent Anand Jain 2021-08-20 7:51 ` Su Yue 2021-08-20 8:53 ` Anand Jain 2021-08-21 14:57 ` Su Yue 2021-08-21 15:00 ` Su Yue 2021-08-23 10:34 ` Anand Jain 2021-08-23 10:54 ` Anand Jain 2021-08-23 12:20 ` David Sterba 2021-08-19 18:18 ` [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname Anand Jain 2021-08-20 7:31 ` Su Yue 2021-08-20 9:13 ` Anand Jain 2021-08-20 10:57 ` David Sterba 2021-08-20 11:03 ` Anand Jain
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.