* [PATCH v6 0/3] btrfs: cleanup prepare_sprout
@ 2021-09-21 4:33 Anand Jain
2021-09-21 4:33 ` [PATCH v6 1/3] btrfs: declare seeding_dev in init_new_device as bool Anand Jain
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Anand Jain @ 2021-09-21 4:33 UTC (permalink / raw)
To: linux-btrfs, dsterba
Patch 3/3 was part of the patchset [1] before.
[1] btrfs: device_list_mutex fix lockdep warn and cleanup
As the patch 1/2 in [1] is already merged, so the patch 2/2
in [1] along with the other cleanup has formed a new set here.
Patch 1/3 and 2/3 are a minor cleanup in btrfs_prepare_sprout().
Patch 3/3 cleans btrfs_prepare_sprout() so that a part of it
can be within the device_list_mutex in the parent function
btrfs_init_new_device().
v6:
Patch 1/3 and 2/3 are new.
Removed RFC from 3/3. Take memory allocation out of device_list_mutex.
Anand Jain (3):
btrfs: declare seeding_dev in init_new_device as bool
btrfs: remove unused device_list_mutex for seed fs_devices
btrfs: consolidate device_list_mutex in prepare_sprout to its parent
fs/btrfs/volumes.c | 51 ++++++++++++++++++++++++++++++----------------
1 file changed, 33 insertions(+), 18 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH v6 1/3] btrfs: declare seeding_dev in init_new_device as bool 2021-09-21 4:33 [PATCH v6 0/3] btrfs: cleanup prepare_sprout Anand Jain @ 2021-09-21 4:33 ` Anand Jain 2021-09-21 11:01 ` Nikolay Borisov 2021-10-13 8:00 ` Anand Jain 2021-09-21 4:33 ` [PATCH v6 2/3] btrfs: remove unused device_list_mutex for seed fs_devices Anand Jain 2021-09-21 4:33 ` [PATCH v6 3/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent Anand Jain 2 siblings, 2 replies; 14+ messages in thread From: Anand Jain @ 2021-09-21 4:33 UTC (permalink / raw) To: linux-btrfs, dsterba This patch declares int seeding_dev as a bool. Also, move its declaration a line below to adjust packing. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- v6: new 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 5117c5816922..c4bc49e58b69 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -2532,8 +2532,8 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; u64 orig_super_total_bytes; u64 orig_super_num_devices; - int seeding_dev = 0; int ret = 0; + bool seeding_dev = false; bool locked = false; if (sb_rdonly(sb) && !fs_devices->seeding) @@ -2550,7 +2550,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path } if (fs_devices->seeding) { - seeding_dev = 1; + seeding_dev = true; down_write(&sb->s_umount); mutex_lock(&uuid_mutex); locked = true; -- 2.31.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/3] btrfs: declare seeding_dev in init_new_device as bool 2021-09-21 4:33 ` [PATCH v6 1/3] btrfs: declare seeding_dev in init_new_device as bool Anand Jain @ 2021-09-21 11:01 ` Nikolay Borisov 2021-10-13 8:00 ` Anand Jain 1 sibling, 0 replies; 14+ messages in thread From: Nikolay Borisov @ 2021-09-21 11:01 UTC (permalink / raw) To: Anand Jain, linux-btrfs, dsterba On 21.09.21 г. 7:33, Anand Jain wrote: > This patch declares int seeding_dev as a bool. Also, move its declaration > a line below to adjust packing. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/3] btrfs: declare seeding_dev in init_new_device as bool 2021-09-21 4:33 ` [PATCH v6 1/3] btrfs: declare seeding_dev in init_new_device as bool Anand Jain 2021-09-21 11:01 ` Nikolay Borisov @ 2021-10-13 8:00 ` Anand Jain 2021-10-28 4:39 ` Anand Jain 1 sibling, 1 reply; 14+ messages in thread From: Anand Jain @ 2021-10-13 8:00 UTC (permalink / raw) To: dsterba; +Cc: linux-btrfs Ping? On 21/09/2021 12:33, Anand Jain wrote: > This patch declares int seeding_dev as a bool. Also, move its declaration > a line below to adjust packing. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > v6: new > 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 5117c5816922..c4bc49e58b69 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -2532,8 +2532,8 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path > struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; > u64 orig_super_total_bytes; > u64 orig_super_num_devices; > - int seeding_dev = 0; > int ret = 0; > + bool seeding_dev = false; > bool locked = false; > > if (sb_rdonly(sb) && !fs_devices->seeding) > @@ -2550,7 +2550,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path > } > > if (fs_devices->seeding) { > - seeding_dev = 1; > + seeding_dev = true; > down_write(&sb->s_umount); > mutex_lock(&uuid_mutex); > locked = true; > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/3] btrfs: declare seeding_dev in init_new_device as bool 2021-10-13 8:00 ` Anand Jain @ 2021-10-28 4:39 ` Anand Jain 2021-11-08 20:06 ` David Sterba 0 siblings, 1 reply; 14+ messages in thread From: Anand Jain @ 2021-10-28 4:39 UTC (permalink / raw) To: dsterba; +Cc: linux-btrfs 2nd try. Ping? (Pls note, patch 2/3 dropped based on Nik's comments. patch 3/3 separated from this set, as it went thru revisions and not related) Thanks, Anand On 13/10/2021 16:00, Anand Jain wrote: > > Ping? > > > On 21/09/2021 12:33, Anand Jain wrote: >> This patch declares int seeding_dev as a bool. Also, move its declaration >> a line below to adjust packing. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> v6: new >> 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 5117c5816922..c4bc49e58b69 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -2532,8 +2532,8 @@ int btrfs_init_new_device(struct btrfs_fs_info >> *fs_info, const char *device_path >> struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; >> u64 orig_super_total_bytes; >> u64 orig_super_num_devices; >> - int seeding_dev = 0; >> int ret = 0; >> + bool seeding_dev = false; >> bool locked = false; >> if (sb_rdonly(sb) && !fs_devices->seeding) >> @@ -2550,7 +2550,7 @@ int btrfs_init_new_device(struct btrfs_fs_info >> *fs_info, const char *device_path >> } >> if (fs_devices->seeding) { >> - seeding_dev = 1; >> + seeding_dev = true; >> down_write(&sb->s_umount); >> mutex_lock(&uuid_mutex); >> locked = true; >> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/3] btrfs: declare seeding_dev in init_new_device as bool 2021-10-28 4:39 ` Anand Jain @ 2021-11-08 20:06 ` David Sterba 0 siblings, 0 replies; 14+ messages in thread From: David Sterba @ 2021-11-08 20:06 UTC (permalink / raw) To: Anand Jain; +Cc: dsterba, linux-btrfs On Thu, Oct 28, 2021 at 12:39:54PM +0800, Anand Jain wrote: > > 2nd try. Ping? > > (Pls note, patch 2/3 dropped based on Nik's comments. patch 3/3 > separated from this set, as it went thru revisions and not related) Patch 1 added to misc-next, thanks. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v6 2/3] btrfs: remove unused device_list_mutex for seed fs_devices 2021-09-21 4:33 [PATCH v6 0/3] btrfs: cleanup prepare_sprout Anand Jain 2021-09-21 4:33 ` [PATCH v6 1/3] btrfs: declare seeding_dev in init_new_device as bool Anand Jain @ 2021-09-21 4:33 ` Anand Jain 2021-09-21 6:44 ` Nikolay Borisov 2021-09-21 4:33 ` [PATCH v6 3/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent Anand Jain 2 siblings, 1 reply; 14+ messages in thread From: Anand Jain @ 2021-09-21 4:33 UTC (permalink / raw) To: linux-btrfs, dsterba The btrfs_fs_devices::seed_list is of type btrfs_fs_devices as well and, it is a pointer to the seed btrfs_fs_device in a btrfs created from a seed devices. In this struct, when it points to seed, the device_list_mutex is not used. So drop its initialize. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- v6: new fs/btrfs/volumes.c | 1 - 1 file changed, 1 deletion(-) diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index c4bc49e58b69..e4079e25db70 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -2418,7 +2418,6 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) seed_devices->opened = 1; INIT_LIST_HEAD(&seed_devices->devices); 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, -- 2.31.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/3] btrfs: remove unused device_list_mutex for seed fs_devices 2021-09-21 4:33 ` [PATCH v6 2/3] btrfs: remove unused device_list_mutex for seed fs_devices Anand Jain @ 2021-09-21 6:44 ` Nikolay Borisov 2021-09-22 11:29 ` Anand Jain 0 siblings, 1 reply; 14+ messages in thread From: Nikolay Borisov @ 2021-09-21 6:44 UTC (permalink / raw) To: Anand Jain, linux-btrfs, dsterba On 21.09.21 г. 7:33, Anand Jain wrote: > The btrfs_fs_devices::seed_list is of type btrfs_fs_devices as well and, seed_list is in fact a list_head which points to btrfs_fs_devices, so that sentence is somewhat misleading. > it is a pointer to the seed btrfs_fs_device in a btrfs created from a > seed devices. > In this struct, when it points to seed, the device_list_mutex is not > used. So drop its initialize. Even if the device_list_mutex is not used in the seed_devices it will have whatever is the state from the original fs_devices so re-initializing it to a sane state is the right thing to do. We don't need this. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > v6: new > fs/btrfs/volumes.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index c4bc49e58b69..e4079e25db70 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -2418,7 +2418,6 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) > seed_devices->opened = 1; > INIT_LIST_HEAD(&seed_devices->devices); > 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, > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/3] btrfs: remove unused device_list_mutex for seed fs_devices 2021-09-21 6:44 ` Nikolay Borisov @ 2021-09-22 11:29 ` Anand Jain 0 siblings, 0 replies; 14+ messages in thread From: Anand Jain @ 2021-09-22 11:29 UTC (permalink / raw) To: Nikolay Borisov; +Cc: linux-btrfs, dsterba On 21/09/2021 14:44, Nikolay Borisov wrote: > > > On 21.09.21 г. 7:33, Anand Jain wrote: >> The btrfs_fs_devices::seed_list is of type btrfs_fs_devices as well and, > > seed_list is in fact a list_head which points to btrfs_fs_devices, so > that sentence is somewhat misleading. > >> it is a pointer to the seed btrfs_fs_device in a btrfs created from a >> seed devices. >> In this struct, when it points to seed, the device_list_mutex is not >> used. So drop its initialize. > > Even if the device_list_mutex is not used in the seed_devices it will > have whatever is the state from the original fs_devices so > re-initializing it to a sane state is the right thing to do. > > We don't need this. I agree. Maybe at some point, we should have a separate new struct for the seed's fs_devices, as few members in it aren't in use. We can drop this patch. Thanks, Anand > >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> v6: new >> fs/btrfs/volumes.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >> index c4bc49e58b69..e4079e25db70 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -2418,7 +2418,6 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) >> seed_devices->opened = 1; >> INIT_LIST_HEAD(&seed_devices->devices); >> 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, >> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v6 3/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent 2021-09-21 4:33 [PATCH v6 0/3] btrfs: cleanup prepare_sprout Anand Jain 2021-09-21 4:33 ` [PATCH v6 1/3] btrfs: declare seeding_dev in init_new_device as bool Anand Jain 2021-09-21 4:33 ` [PATCH v6 2/3] btrfs: remove unused device_list_mutex for seed fs_devices Anand Jain @ 2021-09-21 4:33 ` Anand Jain 2021-09-21 11:01 ` Nikolay Borisov 2 siblings, 1 reply; 14+ messages in thread From: Anand Jain @ 2021-09-21 4:33 UTC (permalink / raw) To: linux-btrfs, dsterba btrfs_prepare_sprout() splices 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(). This patch splits btrfs_prepare_sprout() into btrfs_alloc_sprout() and btrfs_splice_sprout(). This split is essential because device_list_mutex shouldn't be held for btrfs_alloc_sprout() but must be held for btrfs_splice_sprout(). So now a common device_list_mutex can be used between btrfs_init_new_device() and btrfs_splice_sprout(). This patch also moves the lockdep_assert_held(&uuid_mutex) from the starting of the function to just above the line where we need this lock. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- v6: Remove RFC. Split btrfs_prepare_sprout so that the allocation part can be outside of the device_list_mutex in the parent function btrfs_init_new_device(). fs/btrfs/volumes.c | 46 +++++++++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index e4079e25db70..b21eac32ec98 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -2376,19 +2376,13 @@ struct btrfs_device *btrfs_find_device_by_devspec( return btrfs_find_device_by_path(fs_info, device_path); } -/* - * does all the dirty work required for changing file system's UUID. - */ -static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) +static int btrfs_alloc_sprout(struct btrfs_fs_info *fs_info, + struct btrfs_fs_devices **seed_devices_ret) { struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; struct btrfs_fs_devices *old_devices; struct btrfs_fs_devices *seed_devices; - struct btrfs_super_block *disk_super = fs_info->super_copy; - struct btrfs_device *device; - u64 super_flags; - lockdep_assert_held(&uuid_mutex); if (!fs_devices->seeding) return -EINVAL; @@ -2412,6 +2406,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) return PTR_ERR(old_devices); } + lockdep_assert_held(&uuid_mutex); list_add(&old_devices->fs_list, &fs_uuids); memcpy(seed_devices, fs_devices, sizeof(*seed_devices)); @@ -2419,7 +2414,23 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) INIT_LIST_HEAD(&seed_devices->devices); INIT_LIST_HEAD(&seed_devices->alloc_list); - mutex_lock(&fs_devices->device_list_mutex); + *seed_devices_ret = seed_devices; + + return 0; +} + +/* + * Splice seed devices into the sprout fs_devices. + * Generate a new fsid for the sprouted readwrite btrfs. + */ +static void btrfs_splice_sprout(struct btrfs_fs_info *fs_info, + struct btrfs_fs_devices *seed_devices) +{ + struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; + struct btrfs_super_block *disk_super = fs_info->super_copy; + struct btrfs_device *device; + u64 super_flags; + list_splice_init_rcu(&fs_devices->devices, &seed_devices->devices, synchronize_rcu); list_for_each_entry(device, &seed_devices->devices, dev_list) @@ -2435,13 +2446,10 @@ 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; btrfs_set_super_flags(disk_super, super_flags); - - return 0; } /* @@ -2529,6 +2537,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path struct super_block *sb = fs_info->sb; struct rcu_string *name; struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; + struct btrfs_fs_devices *seed_devices; u64 orig_super_total_bytes; u64 orig_super_num_devices; int ret = 0; @@ -2612,18 +2621,25 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path if (seeding_dev) { btrfs_clear_sb_rdonly(sb); - ret = btrfs_prepare_sprout(fs_info); + + /* GFP_KERNEL alloc should not be under device_list_mutex */ + ret = btrfs_alloc_sprout(fs_info, &seed_devices); if (ret) { btrfs_abort_transaction(trans, ret); goto error_trans; } + } + + mutex_lock(&fs_devices->device_list_mutex); + if (seeding_dev) { + btrfs_splice_sprout(fs_info, seed_devices); + btrfs_assign_next_active_device(fs_info->fs_devices->latest_dev, device); } 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); @@ -2685,7 +2701,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path /* * fs_devices now represents the newly sprouted filesystem and - * its fsid has been changed by btrfs_prepare_sprout + * its fsid has been changed by btrfs_sprout_splice(). */ btrfs_sysfs_update_sprout_fsid(fs_devices); } -- 2.31.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v6 3/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent 2021-09-21 4:33 ` [PATCH v6 3/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent Anand Jain @ 2021-09-21 11:01 ` Nikolay Borisov 2021-09-22 11:41 ` Anand Jain 0 siblings, 1 reply; 14+ messages in thread From: Nikolay Borisov @ 2021-09-21 11:01 UTC (permalink / raw) To: Anand Jain, linux-btrfs, dsterba On 21.09.21 г. 7:33, Anand Jain wrote: > btrfs_prepare_sprout() splices 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(). > > This patch splits btrfs_prepare_sprout() into btrfs_alloc_sprout() and > btrfs_splice_sprout(). This split is essential because device_list_mutex > shouldn't be held for btrfs_alloc_sprout() but must be held for > btrfs_splice_sprout(). So now a common device_list_mutex can be used > between btrfs_init_new_device() and btrfs_splice_sprout(). > > This patch also moves the lockdep_assert_held(&uuid_mutex) from the > starting of the function to just above the line where we need this lock. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > v6: > Remove RFC. > Split btrfs_prepare_sprout so that the allocation part can be outside > of the device_list_mutex in the parent function btrfs_init_new_device(). > > fs/btrfs/volumes.c | 46 +++++++++++++++++++++++++++++++--------------- > 1 file changed, 31 insertions(+), 15 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index e4079e25db70..b21eac32ec98 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -2376,19 +2376,13 @@ struct btrfs_device *btrfs_find_device_by_devspec( > return btrfs_find_device_by_path(fs_info, device_path); > } > > -/* > - * does all the dirty work required for changing file system's UUID. > - */ > -static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) > +static int btrfs_alloc_sprout(struct btrfs_fs_info *fs_info, > + struct btrfs_fs_devices **seed_devices_ret) Nope, make the function return a struct btrfs_fs_devices *. > { > struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; > struct btrfs_fs_devices *old_devices; > struct btrfs_fs_devices *seed_devices; > - struct btrfs_super_block *disk_super = fs_info->super_copy; > - struct btrfs_device *device; > - u64 super_flags; > > - lockdep_assert_held(&uuid_mutex); > if (!fs_devices->seeding) > return -EINVAL; > > @@ -2412,6 +2406,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) > return PTR_ERR(old_devices); > } > > + lockdep_assert_held(&uuid_mutex); > list_add(&old_devices->fs_list, &fs_uuids); > > memcpy(seed_devices, fs_devices, sizeof(*seed_devices)); > @@ -2419,7 +2414,23 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) > INIT_LIST_HEAD(&seed_devices->devices); > INIT_LIST_HEAD(&seed_devices->alloc_list); > > - mutex_lock(&fs_devices->device_list_mutex); > + *seed_devices_ret = seed_devices; > + > + return 0; > +} > + > +/* > + * Splice seed devices into the sprout fs_devices. > + * Generate a new fsid for the sprouted readwrite btrfs. > + */ > +static void btrfs_splice_sprout(struct btrfs_fs_info *fs_info, > + struct btrfs_fs_devices *seed_devices) > +{ This function is missing a lockdep_assert_held annotation and it depends on the device_list_mutex being held. However looking at the resulting code it doesn't look good, because btrfs_splice_sporut suggests you simply add the seed device to a bunch of places, yet looking at the function's body it's evident it actually finishes some parts of the initialization, changes the uuid of the fs_devices. I'm not convinced it really makes the code better or at the very least the 'splice_sprout' needs to be changed, because splicing is a minot part of what this function really does. <snip> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 3/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent 2021-09-21 11:01 ` Nikolay Borisov @ 2021-09-22 11:41 ` Anand Jain 2021-09-23 6:52 ` Nikolay Borisov 0 siblings, 1 reply; 14+ messages in thread From: Anand Jain @ 2021-09-22 11:41 UTC (permalink / raw) To: Nikolay Borisov; +Cc: linux-btrfs, dsterba On 21/09/2021 19:01, Nikolay Borisov wrote: > > > On 21.09.21 г. 7:33, Anand Jain wrote: >> btrfs_prepare_sprout() splices 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(). >> >> This patch splits btrfs_prepare_sprout() into btrfs_alloc_sprout() and >> btrfs_splice_sprout(). This split is essential because device_list_mutex >> shouldn't be held for btrfs_alloc_sprout() but must be held for >> btrfs_splice_sprout(). So now a common device_list_mutex can be used >> between btrfs_init_new_device() and btrfs_splice_sprout(). >> >> This patch also moves the lockdep_assert_held(&uuid_mutex) from the >> starting of the function to just above the line where we need this lock. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> v6: >> Remove RFC. >> Split btrfs_prepare_sprout so that the allocation part can be outside >> of the device_list_mutex in the parent function btrfs_init_new_device(). >> >> fs/btrfs/volumes.c | 46 +++++++++++++++++++++++++++++++--------------- >> 1 file changed, 31 insertions(+), 15 deletions(-) >> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >> index e4079e25db70..b21eac32ec98 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -2376,19 +2376,13 @@ struct btrfs_device *btrfs_find_device_by_devspec( >> return btrfs_find_device_by_path(fs_info, device_path); >> } >> >> -/* >> - * does all the dirty work required for changing file system's UUID. >> - */ >> -static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) >> +static int btrfs_alloc_sprout(struct btrfs_fs_info *fs_info, >> + struct btrfs_fs_devices **seed_devices_ret) > > Nope, make the function return a struct btrfs_fs_devices *. > Ok. >> { >> struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; >> struct btrfs_fs_devices *old_devices; >> struct btrfs_fs_devices *seed_devices; >> - struct btrfs_super_block *disk_super = fs_info->super_copy; >> - struct btrfs_device *device; >> - u64 super_flags; >> >> - lockdep_assert_held(&uuid_mutex); >> if (!fs_devices->seeding) >> return -EINVAL; >> >> @@ -2412,6 +2406,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) >> return PTR_ERR(old_devices); >> } >> >> + lockdep_assert_held(&uuid_mutex); >> list_add(&old_devices->fs_list, &fs_uuids); >> >> memcpy(seed_devices, fs_devices, sizeof(*seed_devices)); >> @@ -2419,7 +2414,23 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) >> INIT_LIST_HEAD(&seed_devices->devices); >> INIT_LIST_HEAD(&seed_devices->alloc_list); >> >> - mutex_lock(&fs_devices->device_list_mutex); >> + *seed_devices_ret = seed_devices; >> + >> + return 0; >> +} >> + >> +/* >> + * Splice seed devices into the sprout fs_devices. >> + * Generate a new fsid for the sprouted readwrite btrfs. >> + */ >> +static void btrfs_splice_sprout(struct btrfs_fs_info *fs_info, >> + struct btrfs_fs_devices *seed_devices) >> +{ > > This function is missing a lockdep_assert_held annotation and it depends > on the device_list_mutex being held. You mean lockdep_assert_held(&device_list_mutex); and not lockdep_assert_held(&uuid_mutex); right? > However looking at the resulting code it doesn't look good, because > btrfs_splice_sporut suggests you simply add the seed device to a bunch > of places, yet looking at the function's body it's evident it actually > finishes some parts of the initialization, changes the uuid of the > fs_devices. I'm not convinced it really makes the code better or at the > very least the 'splice_sprout' needs to be changed, because splicing is > a minot part of what this function really does. The purpose of the split of btrfs_prepare_sprout() was to use a common device_list_mutex. So I tend to avoid any other changes, but I think I will do it now based on the comments. Thanks, Anand > > > <snip> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 3/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent 2021-09-22 11:41 ` Anand Jain @ 2021-09-23 6:52 ` Nikolay Borisov 2021-09-23 11:55 ` Anand Jain 0 siblings, 1 reply; 14+ messages in thread From: Nikolay Borisov @ 2021-09-23 6:52 UTC (permalink / raw) To: Anand Jain; +Cc: linux-btrfs, dsterba On 22.09.21 г. 14:41, Anand Jain wrote: <snip> >>> @@ -2419,7 +2414,23 @@ static int btrfs_prepare_sprout(struct >>> btrfs_fs_info *fs_info) >>> INIT_LIST_HEAD(&seed_devices->devices); >>> INIT_LIST_HEAD(&seed_devices->alloc_list); >>> - mutex_lock(&fs_devices->device_list_mutex); >>> + *seed_devices_ret = seed_devices; >>> + >>> + return 0; >>> +} >>> + >>> +/* >>> + * Splice seed devices into the sprout fs_devices. >>> + * Generate a new fsid for the sprouted readwrite btrfs. >>> + */ >>> +static void btrfs_splice_sprout(struct btrfs_fs_info *fs_info, >>> + struct btrfs_fs_devices *seed_devices) >>> +{ >> >> This function is missing a lockdep_assert_held annotation and it depends >> on the device_list_mutex being held. > > You mean > lockdep_assert_held(&device_list_mutex); > and not > lockdep_assert_held(&uuid_mutex); > right? I meant that the new function - btrfs_splice_sprout doesn't have any lockdep annotation, and based on the old code it depends on device_list_mutex being locked. This is based on the following hunk in btrfs_init_new_device: + mutex_lock(&fs_devices->device_list_mutex); + if (seeding_dev) { + btrfs_splice_sprout(fs_info, seed_devices); The way I understand this is btrfs_splice_sprout indeed requires device_list_mutex being locked, no? > >> However looking at the resulting code it doesn't look good, because >> btrfs_splice_sporut suggests you simply add the seed device to a bunch >> of places, yet looking at the function's body it's evident it actually >> finishes some parts of the initialization, changes the uuid of the >> fs_devices. I'm not convinced it really makes the code better or at the >> very least the 'splice_sprout' needs to be changed, because splicing is >> a minot part of what this function really does. > > The purpose of the split of btrfs_prepare_sprout() was to use a common > device_list_mutex. So I tend to avoid any other changes, but I think I > will do it now based on the comments. > > Thanks, Anand >> >> >> <snip> >> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 3/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent 2021-09-23 6:52 ` Nikolay Borisov @ 2021-09-23 11:55 ` Anand Jain 0 siblings, 0 replies; 14+ messages in thread From: Anand Jain @ 2021-09-23 11:55 UTC (permalink / raw) To: Nikolay Borisov; +Cc: linux-btrfs, dsterba On 23/09/2021 14:52, Nikolay Borisov wrote: > > > On 22.09.21 г. 14:41, Anand Jain wrote: > > <snip> > >>>> @@ -2419,7 +2414,23 @@ static int btrfs_prepare_sprout(struct >>>> btrfs_fs_info *fs_info) >>>> INIT_LIST_HEAD(&seed_devices->devices); >>>> INIT_LIST_HEAD(&seed_devices->alloc_list); >>>> - mutex_lock(&fs_devices->device_list_mutex); >>>> + *seed_devices_ret = seed_devices; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +/* >>>> + * Splice seed devices into the sprout fs_devices. >>>> + * Generate a new fsid for the sprouted readwrite btrfs. >>>> + */ >>>> +static void btrfs_splice_sprout(struct btrfs_fs_info *fs_info, >>>> + struct btrfs_fs_devices *seed_devices) >>>> +{ >>> >>> This function is missing a lockdep_assert_held annotation and it depends >>> on the device_list_mutex being held. >> >> You mean >> lockdep_assert_held(&device_list_mutex); >> and not >> lockdep_assert_held(&uuid_mutex); >> right? > > I meant that the new function - btrfs_splice_sprout doesn't have any > lockdep annotation, and based on the old code it depends on > device_list_mutex being locked. This is based on the following hunk in > btrfs_init_new_device: > > + mutex_lock(&fs_devices->device_list_mutex); > + if (seeding_dev) { > + btrfs_splice_sprout(fs_info, seed_devices); > > The way I understand this is btrfs_splice_sprout indeed requires > device_list_mutex being locked, no? Yes it requires both uuid_mutex and device_list_mutex. I will add. With comments. Thx. >>> However looking at the resulting code it doesn't look good, because >>> btrfs_splice_sporut suggests you simply add the seed device to a bunch >>> of places, yet looking at the function's body it's evident it actually >>> finishes some parts of the initialization, changes the uuid of the >>> fs_devices. I'm not convinced it really makes the code better or at the >>> very least the 'splice_sprout' needs to be changed, because splicing is >>> a minot part of what this function really does. >> >> The purpose of the split of btrfs_prepare_sprout() was to use a common >> device_list_mutex. So I tend to avoid any other changes, but I think I >> will do it now based on the comments. >> >> Thanks, Anand >>> >>> >>> <snip> >>> >> ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-11-08 20:06 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-21 4:33 [PATCH v6 0/3] btrfs: cleanup prepare_sprout Anand Jain 2021-09-21 4:33 ` [PATCH v6 1/3] btrfs: declare seeding_dev in init_new_device as bool Anand Jain 2021-09-21 11:01 ` Nikolay Borisov 2021-10-13 8:00 ` Anand Jain 2021-10-28 4:39 ` Anand Jain 2021-11-08 20:06 ` David Sterba 2021-09-21 4:33 ` [PATCH v6 2/3] btrfs: remove unused device_list_mutex for seed fs_devices Anand Jain 2021-09-21 6:44 ` Nikolay Borisov 2021-09-22 11:29 ` Anand Jain 2021-09-21 4:33 ` [PATCH v6 3/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent Anand Jain 2021-09-21 11:01 ` Nikolay Borisov 2021-09-22 11:41 ` Anand Jain 2021-09-23 6:52 ` Nikolay Borisov 2021-09-23 11:55 ` 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.