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

[Josef Bacik <josef@toxicpanda.com>]

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

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

Thanks,

Josef