[PATCH v6 0/3] btrfs: cleanup prepare_sprout

[PATCH v6 0/3] btrfs: cleanup prepare_sprout

[Anand Jain]

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

[PATCH v6 1/3] btrfs: declare seeding_dev in init_new_device as bool

[Anand Jain]

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

[PATCH v6 2/3] btrfs: remove unused device_list_mutex for seed fs_devices

[Anand Jain]

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

[PATCH v6 3/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent

[Anand Jain]

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

Re: [PATCH v6 2/3] btrfs: remove unused device_list_mutex for seed fs_devices

[Nikolay Borisov]

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,
> 

Re: [PATCH v6 3/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent

[Nikolay Borisov]

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>

Re: [PATCH v6 1/3] btrfs: declare seeding_dev in init_new_device as bool

[Nikolay Borisov]

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>

Re: [PATCH v6 2/3] btrfs: remove unused device_list_mutex for seed fs_devices

[Anand Jain]

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,
>>

Re: [PATCH v6 3/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent

[Anand Jain]

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>
> 

Re: [PATCH v6 3/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent

[Nikolay Borisov]

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>
>>
> 

Re: [PATCH v6 3/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent

[Anand Jain]

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>
>>>
>>

Re: [PATCH v6 1/3] btrfs: declare seeding_dev in init_new_device as bool

[Anand Jain]

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;
> 

Re: [PATCH v6 1/3] btrfs: declare seeding_dev in init_new_device as bool

[Anand Jain]

  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;
>>

Re: [PATCH v6 1/3] btrfs: declare seeding_dev in init_new_device as bool

[David Sterba]

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.