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

From: Boris Burkov
Date: Wed Feb 14 2024 - 13:57:23 EST


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
>