[2/5] btrfs: call btrfs_close_devices from ->kill_sb

Commit Message

Christoph Hellwig Dec. 18, 2023, 4:49 a.m. UTC

blkdev_put must not be called under sb->s_umount to avoid a lock order
reversal with disk->open_mutex once call backs from block devices to
the file system using the holder ops are supported.  Move the call
to btrfs_close_devices into btrfs_free_fs_info so that it is closed
from ->kill_sb (which is also called from the mount failure handling
path unlike ->put_super) as well as when an fs_info is freed because
an existing superblock already exists.

Signed-off-by: Christoph Hellwig <[e-mail]>
---
 fs/btrfs/disk-io.c |  4 ++--
 fs/btrfs/super.c   | 27 ++++++++++++++-------------
 2 files changed, 16 insertions(+), 15 deletions(-)

Comments

Christian Brauner Dec. 18, 2023, 12:22 p.m. UTC | #1

On Mon, Dec 18, 2023 at 05:49:30AM +0100, Christoph Hellwig wrote:
> blkdev_put must not be called under sb->s_umount to avoid a lock order
> reversal with disk->open_mutex once call backs from block devices to
> the file system using the holder ops are supported.  Move the call

With what's in vfs.super that part isn't necessary anymore. Locking
order is guaranteed so that s_umount ranks above open_mutex as before as
you know ofc. And we've got lockdep asserts everywhere so that lockdep
would complain immediately. It's still nicer imho to close devices in
->kill_sb() but it isn't needed anymore.

btrfs folks might want to consider pulling in vfs.super. It's been
stable on v6.7-rc1 for weeks and I won't change it anymore. Last change
on that branch is from Tue, 28 November.

Eric Biggers Dec. 27, 2023, 5:09 p.m. UTC | #2

On Mon, Dec 18, 2023 at 05:49:30AM +0100, Christoph Hellwig wrote:
> blkdev_put must not be called under sb->s_umount to avoid a lock order
> reversal with disk->open_mutex once call backs from block devices to
> the file system using the holder ops are supported.

This contradicts the following from Documentation/filesystems/porting.rst:

    **mandatory**

    Lock ordering has been changed so that s_umount ranks above open_mutex again.
    All places where s_umount was taken under open_mutex have been fixed up.

So the rationale for this patch seems off.

It's still needed as a prerequisite for "fs: move fscrypt keyring destruction to
after ->put_super", if we indeed go with that instead of the alternative patch
"fscrypt: move the call to fscrypt_destroy_keyring() into ->put_super()".

- Eric

Patch

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index c6907d533fe839..c2f57c986069b7 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1265,6 +1265,8 @@  static void free_global_roots(struct btrfs_fs_info *fs_info)
 
 void btrfs_free_fs_info(struct btrfs_fs_info *fs_info)
 {
+	if (fs_info->fs_devices)
+		btrfs_close_devices(fs_info->fs_devices);
 	percpu_counter_destroy(&fs_info->dirty_metadata_bytes);
 	percpu_counter_destroy(&fs_info->delalloc_bytes);
 	percpu_counter_destroy(&fs_info->ordered_bytes);
@@ -3597,7 +3599,6 @@  int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 
 	iput(fs_info->btree_inode);
 fail:
-	btrfs_close_devices(fs_info->fs_devices);
 	ASSERT(ret < 0);
 	return ret;
 }
@@ -4377,7 +4378,6 @@  void __cold close_ctree(struct btrfs_fs_info *fs_info)
 	iput(fs_info->btree_inode);
 
 	btrfs_mapping_tree_free(fs_info);
-	btrfs_close_devices(fs_info->fs_devices);
 }
 
 void btrfs_mark_buffer_dirty(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index ba16ade1d79aea..7d38f973e991f6 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1810,10 +1810,8 @@  static int btrfs_get_tree_super(struct fs_context *fc)
 	if (ret)
 		return ret;
 
-	if (!(fc->sb_flags & SB_RDONLY) && fs_devices->rw_devices == 0) {
-		ret = -EACCES;
-		goto error;
-	}
+	if (!(fc->sb_flags & SB_RDONLY) && fs_devices->rw_devices == 0)
+		return -EACCES;
 
 	bdev = fs_devices->latest_dev->bdev;
 
@@ -1827,15 +1825,12 @@  static int btrfs_get_tree_super(struct fs_context *fc)
 	 * otherwise it's tied to the lifetime of the super_block.
 	 */
 	sb = sget_fc(fc, btrfs_fc_test_super, set_anon_super_fc);
-	if (IS_ERR(sb)) {
-		ret = PTR_ERR(sb);
-		goto error;
-	}
+	if (IS_ERR(sb))
+		return PTR_ERR(sb);
 
 	set_device_specific_options(fs_info);
 
 	if (sb->s_root) {
-		btrfs_close_devices(fs_devices);
 		if ((fc->sb_flags ^ sb->s_flags) & SB_RDONLY)
 			ret = -EBUSY;
 	} else {
@@ -1854,10 +1849,6 @@  static int btrfs_get_tree_super(struct fs_context *fc)
 
 	fc->root = dget(sb->s_root);
 	return 0;
-
-error:
-	btrfs_close_devices(fs_devices);
-	return ret;
 }
 
 /*
@@ -1950,10 +1941,20 @@  static int btrfs_get_tree_super(struct fs_context *fc)
  */
 static struct vfsmount *btrfs_reconfigure_for_mount(struct fs_context *fc)
 {
+	struct btrfs_fs_info *fs_info = fc->s_fs_info;
 	struct vfsmount *mnt;
 	int ret;
 	const bool ro2rw = !(fc->sb_flags & SB_RDONLY);
 
+	/*
+	 * We got a reference to our fs_devices, so we need to close it here to
+	 * make sure we don't leak our reference on the fs_devices.
+	 */
+	if (fs_info->fs_devices) {
+		btrfs_close_devices(fs_info->fs_devices);
+		fs_info->fs_devices = NULL;
+	}
+
 	/*
 	 * We got an EBUSY because our SB_RDONLY flag didn't match the existing
 	 * super block, so invert our setting here and retry the mount so we