* [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
* [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
* [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 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 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 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 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
* 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
* 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
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.