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

[Anand Jain <anand.jain@oracle.com>]

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