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

[Su Yue <l@damenly.su>]

On Fri 20 Aug 2021 at 02:18, Anand Jain <anand.jain@oracle.com> 
wrote:

> btrfs_prepare_sprout() moves 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().
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> RFC because I haven't identified the other thread which could 
> race with
> this, but still does this cleanup makes sense?
>
>  fs/btrfs/volumes.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 91b8422b3f67..f490d1897c56 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2366,6 +2366,8 @@ static int btrfs_prepare_sprout(struct 
> btrfs_fs_info *fs_info)
>  	u64 super_flags;
>
>  	lockdep_assert_held(&uuid_mutex);
> +	lockdep_assert_held(&fs_devices->device_list_mutex);
> +
>
Just a reminder: clone_fs_devices() still takes the mutex in 
misc-next.

>  	if (!fs_devices->seeding)
>  		return -EINVAL;
>
> @@ -2397,7 +2399,6 @@ static int btrfs_prepare_sprout(struct 
> btrfs_fs_info *fs_info)
>  	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,
>  			      synchronize_rcu);
>  	list_for_each_entry(device, &seed_devices->devices, dev_list)
> @@ -2413,7 +2414,6 @@ 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;
> @@ -2588,6 +2588,7 @@ int btrfs_init_new_device(struct 
> btrfs_fs_info *fs_info, const char *device_path
>  	device->dev_stats_valid = 1;
>  	set_blocksize(device->bdev, BTRFS_BDEV_BLOCKSIZE);
>
> +	mutex_lock(&fs_devices->device_list_mutex);
>  	if (seeding_dev) {
>  		btrfs_clear_sb_rdonly(sb);
>  		ret = btrfs_prepare_sprout(fs_info);
>
the erorr case:
if (ret) {
    mutex_unlock(&fs_devices->device_list_mutex);
    ...
}

Thanks.

--
Su

> @@ -2599,7 +2600,6 @@ int btrfs_init_new_device(struct 
> btrfs_fs_info *fs_info, const char *device_path
>
>  	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);