Re: [PATCH v8] btrfs: consolidate device_list_mutex in prepare_sprout to its parent

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

On 14/10/2021 23:30, Josef Bacik wrote:
> On Wed, Oct 13, 2021 at 04:01:37PM +0800, 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_init_sprout() and
>> btrfs_setup_sprout(). This split is essential because device_list_mutex
>> shouldn't be held for allocs in btrfs_init_sprout() but must be held for
>> btrfs_setup_sprout(). So now a common device_list_mutex can be used
>> between btrfs_init_new_device() and btrfs_setup_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>
>> ---
>> v8:
>>   Change log update:
>>   s/btrfs_alloc_sprout/btrfs_init_sprout/g
>>   s/btrfs_splice_sprout/btrfs_setup_sprout/g
>>   No code change.
>>
>> v7:
>>   . Not part of the patchset "btrfs: cleanup prepare_sprout" anymore as
>>   1/3 is merged and 2/3 is dropped.
>>   . Rename btrfs_alloc_sprout() to btrfs_init_sprout() as it does more
>>   than just alloc and change return to btrfs_device *.
>>   . Rename btrfs_splice_sprout() to btrfs_setup_sprout() as it does more
>>   than just the splice.
>>   . Add lockdep_assert_held(&uuid_mutex) and
>>   lockdep_assert_held(&fs_devices->device_list_mutex) in btrfs_setup_sprout().
>>
>> 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 | 73 +++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 53 insertions(+), 20 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 8e2b76b5fd14..10227b13a1a6 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -2378,21 +2378,14 @@ 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 struct btrfs_fs_devices *btrfs_init_sprout(struct btrfs_fs_info *fs_info)
>>   {
>>   	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;
>> +		return ERR_PTR(-EINVAL);
>>   
>>   	/*
>>   	 * Private copy of the seed devices, anchored at
>> @@ -2400,7 +2393,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
>>   	 */
>>   	seed_devices = alloc_fs_devices(NULL, NULL);
>>   	if (IS_ERR(seed_devices))
>> -		return PTR_ERR(seed_devices);
>> +		return seed_devices;
>>   
>>   	/*
>>   	 * It's necessary to retain a copy of the original seed fs_devices in
>> @@ -2411,9 +2404,10 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
>>   	old_devices = clone_fs_devices(fs_devices);
>>   	if (IS_ERR(old_devices)) {
>>   		kfree(seed_devices);
>> -		return PTR_ERR(old_devices);
>> +		return old_devices;
>>   	}
>>   
>> +	lockdep_assert_held(&uuid_mutex);
> 
> There's no reason to move this down here, you can leave it at the top of this
> function.  Fix that up and you can add
> 

  I thought placing the lockdep_assert_held()s just before the critical
  section that wanted the lock makes it easy to reason. In this case, it
  is the next line that is

       list_add(&old_devices->fs_list, &fs_uuids);

  No? I can move it back if it is unnecessary.


> Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks, Anand

> 
> Thanks,
> 
> Josef
>