Re: [PATCH 4/5] btrfs: open block devices after superblock creation

[Boris Burkov]

On Wed, Feb 14, 2024 at 08:42:15AM -0800, Johannes Thumshirn wrote:
> From: Christoph Hellwig <hch@xxxxxx>
> 
> Currently btrfs_mount_root opens the block devices before committing to
> allocating a super block. That creates problems for restricting the
> number of writers to a device, and also leads to a unusual and not very
> helpful holder (the fs_type).
> 
> Reorganize the code to first check whether the superblock for a
> particular fsid does already exist and open the block devices only if it
> doesn't, mirroring the recent changes to the VFS mount helpers.  To do
> this the increment of the in_use counter moves out of btrfs_open_devices
> and into the only caller in btrfs_mount_root so that it happens before
> dropping uuid_mutex around the call to sget.

I believe this commit message is now out of date as of
'btrfs: remove old mount API code'
which got rid of btrfs_mount_root.

As far as I can tell, the code itself is updated and fine.

> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@xxxxxxx>
> ---
>  fs/btrfs/super.c   | 41 +++++++++++++++++++++++++----------------
>  fs/btrfs/volumes.c | 15 +++++----------
>  2 files changed, 30 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 51b8fd272b15..1fa7d83d02c1 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1794,7 +1794,6 @@ static int btrfs_get_tree_super(struct fs_context *fc)
>  	struct btrfs_fs_info *fs_info = fc->s_fs_info;
>  	struct btrfs_fs_context *ctx = fc->fs_private;
>  	struct btrfs_fs_devices *fs_devices = NULL;
> -	struct block_device *bdev;
>  	struct btrfs_device *device;
>  	struct super_block *sb;
>  	blk_mode_t mode = btrfs_open_mode(fc);
> @@ -1817,15 +1816,8 @@ static int btrfs_get_tree_super(struct fs_context *fc)
>  	fs_devices = device->fs_devices;
>  	fs_info->fs_devices = fs_devices;
>  
> -	ret = btrfs_open_devices(fs_devices, mode, &btrfs_fs_type);
> +	fs_devices->in_use++;
>  	mutex_unlock(&uuid_mutex);
> -	if (ret)
> -		return ret;
> -
> -	if (!(fc->sb_flags & SB_RDONLY) && fs_devices->rw_devices == 0)
> -		return -EACCES;
> -
> -	bdev = fs_devices->latest_dev->bdev;
>  
>  	/*
>  	 * From now on the error handling is not straightforward.
> @@ -1843,24 +1835,41 @@ static int btrfs_get_tree_super(struct fs_context *fc)
>  	set_device_specific_options(fs_info);
>  
>  	if (sb->s_root) {
> -		if ((fc->sb_flags ^ sb->s_flags) & SB_RDONLY)
> +		if ((fc->sb_flags ^ sb->s_flags) & SB_RDONLY) {
>  			ret = -EBUSY;
> +			goto error_deactivate;
> +		}
>  	} else {
> -		snprintf(sb->s_id, sizeof(sb->s_id), "%pg", bdev);
> +		struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
> +
> +		mutex_lock(&uuid_mutex);
> +		ret = btrfs_open_devices(fs_devices, mode, &btrfs_fs_type);
> +		mutex_unlock(&uuid_mutex);
> +		if (ret)
> +			goto error_deactivate;
> +
> +		if (!(fc->sb_flags & SB_RDONLY) && !fs_devices->rw_devices) {
> +			ret = -EACCES;
> +			goto error_deactivate;
> +		}
> +
> +		snprintf(sb->s_id, sizeof(sb->s_id), "%pg",
> +			 fs_devices->latest_dev->bdev);
>  		shrinker_debugfs_rename(sb->s_shrink, "sb-btrfs:%s", sb->s_id);
>  		btrfs_sb(sb)->bdev_holder = &btrfs_fs_type;
>  		ret = btrfs_fill_super(sb, fs_devices, NULL);
> -	}
> -
> -	if (ret) {
> -		deactivate_locked_super(sb);
> -		return ret;
> +		if (ret)
> +			goto error_deactivate;
>  	}
>  
>  	btrfs_clear_oneshot_options(fs_info);
>  
>  	fc->root = dget(sb->s_root);
>  	return 0;
> +
> +error_deactivate:
> +	deactivate_locked_super(sb);
> +	return ret;
>  }
>  
>  /*
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index f27af155abf0..6e82bd7ce501 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1220,8 +1220,6 @@ static int devid_cmp(void *priv, const struct list_head *a,
>  int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>  		       blk_mode_t flags, void *holder)
>  {
> -	int ret;
> -
>  	lockdep_assert_held(&uuid_mutex);
>  	/*
>  	 * The device_list_mutex cannot be taken here in case opening the
> @@ -1230,14 +1228,11 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>  	 * We also don't need the lock here as this is called during mount and
>  	 * exclusion is provided by uuid_mutex
>  	 */
> -	if (!fs_devices->is_open) {
> -		list_sort(NULL, &fs_devices->devices, devid_cmp);
> -		ret = open_fs_devices(fs_devices, flags, holder);
> -		if (ret)
> -			return ret;
> -	}
> -	fs_devices->in_use++;
> -	return 0;
> +	ASSERT(fs_devices->in_use);
> +	if (fs_devices->is_open)
> +		return 0;
> +	list_sort(NULL, &fs_devices->devices, devid_cmp);
> +	return open_fs_devices(fs_devices, flags, holder);
>  }
>  
>  void btrfs_release_disk_super(struct btrfs_super_block *super)
> 
> -- 
> 2.43.0
>