[4/5] btrfs: open block devices after superblock creation
Commit Message
From: Christoph Hellwig <hch@lst.de>
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.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
fs/btrfs/super.c | 41 +++++++++++++++++++++++++----------------
fs/btrfs/volumes.c | 15 +++++----------
2 files changed, 30 insertions(+), 26 deletions(-)
Comments
On Wed, Feb 14, 2024 at 08:42:15AM -0800, Johannes Thumshirn wrote:
> From: Christoph Hellwig <hch@lst.de>
>
> 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@lst.de>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
> 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
>
On Wed, Feb 14, 2024 at 10:58:09AM -0800, Boris Burkov wrote:
> On Wed, Feb 14, 2024 at 08:42:15AM -0800, Johannes Thumshirn wrote:
> > From: Christoph Hellwig <hch@lst.de>
> >
> > 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.
It's not just that, this patchset was sent before the conversion to new
mount API that changed how devices are scanned (and potentially race
with mount). The changelog should be updated at minimum.
I haven't found any problems so far, the locking around device opening
should serialize any races so the one thread winning will open the super
block and the other will inherit the fs_devices.
@@ -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;
}
/*
@@ -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)