[PATCH v3 0/6] btrfs: use the super_block as bdev holder

[PATCH v3 0/6] btrfs: use the super_block as bdev holder

[Qu Wenruo]

[CHANGELOG]
v3:
- Drop the btrfs_fs_devices::opened split
  It turns out to cause problems during tests.

- Extra cleanup related to the btrfs_get_tree_*()
  Now the re-entry through vfs_get_tree() is completely dropped.

- Extra comments explaining the sget_fc() behavior

- Call bdev_fput() instead of fput()
  This alignes us to all the other fses.

- Updated patch to delay btrfs_open_devices() until sget_fc()
  Instead of relying on the previous solution (split
  btrfs_open_devices::opened), just expand the uuid_mutex critical
  section.

This is the refreshed series based on the one submitted by Johannes,
which is further based on the original series from Christoph.

This series is to make btrfs use its super block as blk_holder, this
allows future usage of fs_holder_ops to support block device operations.

This is done by:

- Get rid of the re-entry of btrfs_get_tree()
  Now it's a simple call chain:

   btrfs_get_tree() -> btrfs_get_tree_subvol() -> btrfs_get_tree_super()

- Make it more clear on the sget_fc() behavior
  And what should be cleaned up.

- Call btrfs_close_devices() from kill_sb() callback

- Call bdev_fput() instead of deferred fput()
  This ensures we won't get some block devices with invalid holder,
  while the fs is already closed.

- Delay btrfs_open_devices(0 until super block is created
  This is done in a simpler way, just expand the uuid_mutex critical
  section to cover sget_fc().

- Use super block as blk_holder


Christoph Hellwig (2):
  btrfs: call btrfs_close_devices from ->kill_sb
  btrfs: use the super_block as holder when mounting file systems

Qu Wenruo (4):
  btrfs: get rid of the re-entry of btrfs_get_tree()
  btrfs: add comments to make super block creation more clear
  btrfs: call bdev_fput() to reclaim the blk_holder immediately
  btrfs: delay btrfs_open_devices() until super block is created

 fs/btrfs/dev-replace.c |  4 +-
 fs/btrfs/disk-io.c     |  4 +-
 fs/btrfs/fs.h          |  2 -
 fs/btrfs/ioctl.c       |  4 +-
 fs/btrfs/super.c       | 96 ++++++++++++++++++------------------------
 fs/btrfs/volumes.c     | 22 +++++-----
 6 files changed, 59 insertions(+), 73 deletions(-)

-- 
2.49.0

[PATCH v3 1/6] btrfs: get rid of the re-entry of btrfs_get_tree()

[Qu Wenruo]

[EXISTING PROBLEM]
Currently btrfs mount is split into two parts:

- btrfs_get_tree_subvol()
  Which setups the very basic fs_info, and eventually call
  mount_subvol() to mount the target subvolume.

- btrfs_get_tree_super()
  This is the part doing super block allocation and if there is no
  existing super block, do the real open_ctree() to open the fs.

However currently we're doing this in a complex re-entry fashion:

vfs_get_tree()
|- btrfs_get_tree()
   |- btrfs_get_tree_subvol()
      |- vfs_get_tree()
      |  |- btrfs_get_tree()
      |     |- btrfs_get_tree_super()
      |- mount_subvol()

This is definitely not that easy to grasp.

[ENHANCEMENT]
The function vfs_get_tree() is only doing the following works:

- Call get_tree() call back
- Call super_wake()
- Call security_sb_set_mnt_opts()

In our case, super_wake() can be skipped, as after
btrfs_get_tree_subvol() finished, vfs_get_tree() will call super_wake()
on the super block we got anyway.

The same applies to security_sb_set_mnt_opts(), as long as we do not
free the security from our original fc in btrfs_get_tree_subvol(), the
first vfs_get_tree() call will handle the security correctly.

So here we only need to:

- Replace vfs_get_tree() call with btrfs_get_tree_super()

- Keep the existing fc->security for vfs_get_tree() to handle the
  security

This will remove the re-entry behavior and make thing much easier to
follow.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/super.c | 27 +++------------------------
 1 file changed, 3 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index b9e08a59da4e..d977d2da985e 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2046,15 +2046,7 @@ static int btrfs_get_tree_subvol(struct fs_context *fc)
 	 */
 	dup_fc->s_fs_info = fs_info;
 
-	/*
-	 * We'll do the security settings in our btrfs_get_tree_super() mount
-	 * loop, they were duplicated into dup_fc, we can drop the originals
-	 * here.
-	 */
-	security_free_mnt_opts(&fc->security);
-	fc->security = NULL;
-
-	ret = vfs_get_tree(dup_fc);
+	ret = btrfs_get_tree_super(dup_fc);
 	if (ret)
 		goto error;
 
@@ -2086,21 +2078,8 @@ static int btrfs_get_tree_subvol(struct fs_context *fc)
 
 static int btrfs_get_tree(struct fs_context *fc)
 {
-	/*
-	 * Since we use mount_subtree to mount the default/specified subvol, we
-	 * have to do mounts in two steps.
-	 *
-	 * First pass through we call btrfs_get_tree_subvol(), this is just a
-	 * wrapper around fc_mount() to call back into here again, and this time
-	 * we'll call btrfs_get_tree_super().  This will do the open_ctree() and
-	 * everything to open the devices and file system.  Then we return back
-	 * with a fully constructed vfsmount in btrfs_get_tree_subvol(), and
-	 * from there we can do our mount_subvol() call, which will lookup
-	 * whichever subvol we're mounting and setup this fc with the
-	 * appropriate dentry for the subvol.
-	 */
-	if (fc->s_fs_info)
-		return btrfs_get_tree_super(fc);
+	ASSERT(fc->s_fs_info == NULL);
+
 	return btrfs_get_tree_subvol(fc);
 }
 
-- 
2.49.0

[PATCH v3 2/6] btrfs: add comments to make super block creation more clear

[Qu Wenruo]

When calling sget_fc(), there are 3 different situations:

a) Critical error
   No super block created.

b) A new super block is created
   The fc->s_fs_info is transferred to the super block, and fc->s_fs_info
   is reset to NULL.

   In this case sb->s_root should still be NULL, and needs to be properly
   initialized later by btrfs_fill_super().

c) An existing super block is returned
   The fc->s_fs_info is untouched, and anything related to that fs_info
   should be properly cleaned up.

This is not obvious even with the extra comments at sget_fc().

Enhance the situation by:

- Add comments for case b) and c)
  Especially for case c), the fs_info and fs_devices cleanup happens at
  different timing, thus needs extra explanation.

- Move the comments closer to case b) and case c)

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/super.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index d977d2da985e..c5c3ad40d07e 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1876,15 +1876,6 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 
 	bdev = fs_devices->latest_dev->bdev;
 
-	/*
-	 * From now on the error handling is not straightforward.
-	 *
-	 * If successful, this will transfer the fs_info into the super block,
-	 * and fc->s_fs_info will be NULL.  However if there's an existing
-	 * super, we'll still have fc->s_fs_info populated.  If we error
-	 * completely out it'll be cleaned up when we drop the fs_context,
-	 * 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);
@@ -1894,6 +1885,20 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 	set_device_specific_options(fs_info);
 
 	if (sb->s_root) {
+		/*
+		 * Not the first mount of the fs thus got an existing super block.
+		 *
+		 * Will reuse the returned super block, fs_info and fs_devices.
+		 */
+		ASSERT(fc->s_fs_info == fs_info);
+
+		/*
+		 * fc->s_fs_info is not touched and will be later freed by
+		 * put_fs_context() through btrfs_free_fs_context().
+		 * 
+		 * But we have opened fs_devices at the beginning of the
+		 * function, thus still need to close them manually.
+		 */
 		btrfs_close_devices(fs_devices);
 		/*
 		 * At this stage we may have RO flag mismatch between
@@ -1902,6 +1907,13 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 		 * needed.
 		 */
 	} else {
+		/*
+		 * The first mount of the fs thus a new superblock, fc->s_fs_info
+		 * should be NULL, and the owner ship of our fs_info and fs_devices is
+		 * transferred to the super block.
+		 */
+		ASSERT(fc->s_fs_info == NULL);
+
 		snprintf(sb->s_id, sizeof(sb->s_id), "%pg", bdev);
 		shrinker_debugfs_rename(sb->s_shrink, "sb-btrfs:%s", sb->s_id);
 		btrfs_sb(sb)->bdev_holder = &btrfs_fs_type;
-- 
2.49.0

[PATCH v3 3/6] btrfs: call btrfs_close_devices from ->kill_sb

[Qu Wenruo]

From: Christoph Hellwig <hch@lst.de>

Although btrfs is not yet implementing blk_holder_ops yet, there is a
requirement for a proper blk_holder_ops:

- blkdev_put() must not be called under sb->s_umount
  The blkdev_put()/bdev_fput() must not be called under sb->s_umount to
  avoid lock order reversal with disk->open_mutex.
  This is for the proper blk_holder_ops callbacks.

  Currently we're fine because we call regular fput() which defers the
  blk holder reclaiming.

To prepare for the future of blk_holder_ops, move the
btrfs_close_devices() calls into btrfs_free_fs_info().

That will be called from kill_sb() callbacks, which is also called for
error handing during mount failures, or there is already an existing
super block.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c |  4 ++--
 fs/btrfs/super.c   | 28 +++++++++-------------------
 2 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 0d6ad7512f21..a9689cb98a90 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1246,6 +1246,8 @@ void btrfs_free_fs_info(struct btrfs_fs_info *fs_info)
 {
 	struct percpu_counter *em_counter = &fs_info->evictable_extent_maps;
 
+	if (fs_info->fs_devices)
+		btrfs_close_devices(fs_info->fs_devices);
 	percpu_counter_destroy(&fs_info->stats_read_blocks);
 	percpu_counter_destroy(&fs_info->dirty_metadata_bytes);
 	percpu_counter_destroy(&fs_info->delalloc_bytes);
@@ -3681,7 +3683,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;
 }
@@ -4428,7 +4429,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 c5c3ad40d07e..41c824d216fc 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1869,18 +1869,14 @@ 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;
 
 	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);
 
@@ -1889,17 +1885,15 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 		 * Not the first mount of the fs thus got an existing super block.
 		 *
 		 * Will reuse the returned super block, fs_info and fs_devices.
-		 */
-		ASSERT(fc->s_fs_info == fs_info);
-
-		/*
+		 *
 		 * fc->s_fs_info is not touched and will be later freed by
 		 * put_fs_context() through btrfs_free_fs_context().
 		 * 
-		 * But we have opened fs_devices at the beginning of the
-		 * function, thus still need to close them manually.
+		 * And the fs_info->fs_devices will also be closed by
+		 * btrfs_free_fs_context().
 		 */
-		btrfs_close_devices(fs_devices);
+		ASSERT(fc->s_fs_info == fs_info);
+
 		/*
 		 * At this stage we may have RO flag mismatch between
 		 * fc->sb_flags and sb->s_flags.  Caller should detect such
@@ -1928,10 +1922,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;
 }
 
 /*
-- 
2.49.0

[PATCH v3 4/6] btrfs: call bdev_fput() to reclaim the blk_holder immediately

[Qu Wenruo]

As part of the preparation for btrfs blk_holder_ops, we want to ensure
the holder of a block device has a proper lifespan.

However btrfs is always using fput() to close a block device, which has
one problem:

- fput() is deferred
  Meaning we can have a block device with invalid (aka, freed) holder.

To avoid the problem, and align the behavior to all the existing codes,
just call bdev_fput() instead.

There is some extra requirement on the locking, but that's all resolved
by previous patches and we should be safe to call bdev_fput().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/dev-replace.c |  2 +-
 fs/btrfs/ioctl.c       |  4 ++--
 fs/btrfs/volumes.c     | 18 +++++++++---------
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 2decb9fff445..6576867cc1e9 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -327,7 +327,7 @@ static int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 	return 0;
 
 error:
-	fput(bdev_file);
+	bdev_fput(bdev_file);
 	return ret;
 }
 
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 4eda35bdba71..608a63f07e6b 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2700,7 +2700,7 @@ static long btrfs_ioctl_rm_dev_v2(struct file *file, void __user *arg)
 err_drop:
 	mnt_drop_write_file(file);
 	if (bdev_file)
-		fput(bdev_file);
+		bdev_fput(bdev_file);
 out:
 	btrfs_put_dev_args_from_path(&args);
 	kfree(vol_args);
@@ -2751,7 +2751,7 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
 
 	mnt_drop_write_file(file);
 	if (bdev_file)
-		fput(bdev_file);
+		bdev_fput(bdev_file);
 out:
 	btrfs_put_dev_args_from_path(&args);
 out_free:
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1ebfc69012a2..3e701790dde9 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -488,7 +488,7 @@ btrfs_get_bdev_and_sb(const char *device_path, blk_mode_t flags, void *holder,
 	if (holder) {
 		ret = set_blocksize(*bdev_file, BTRFS_BDEV_BLOCKSIZE);
 		if (ret) {
-			fput(*bdev_file);
+			bdev_fput(*bdev_file);
 			goto error;
 		}
 	}
@@ -496,7 +496,7 @@ btrfs_get_bdev_and_sb(const char *device_path, blk_mode_t flags, void *holder,
 	*disk_super = btrfs_read_disk_super(bdev, 0, false);
 	if (IS_ERR(*disk_super)) {
 		ret = PTR_ERR(*disk_super);
-		fput(*bdev_file);
+		bdev_fput(*bdev_file);
 		goto error;
 	}
 
@@ -720,7 +720,7 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
 
 error_free_page:
 	btrfs_release_disk_super(disk_super);
-	fput(bdev_file);
+	bdev_fput(bdev_file);
 
 	return -EINVAL;
 }
@@ -1070,7 +1070,7 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
 			continue;
 
 		if (device->bdev_file) {
-			fput(device->bdev_file);
+			bdev_fput(device->bdev_file);
 			device->bdev = NULL;
 			device->bdev_file = NULL;
 			fs_devices->open_devices--;
@@ -1117,7 +1117,7 @@ static void btrfs_close_bdev(struct btrfs_device *device)
 		invalidate_bdev(device->bdev);
 	}
 
-	fput(device->bdev_file);
+	bdev_fput(device->bdev_file);
 }
 
 static void btrfs_close_one_device(struct btrfs_device *device)
@@ -1490,7 +1490,7 @@ struct btrfs_device *btrfs_scan_one_device(const char *path,
 	btrfs_release_disk_super(disk_super);
 
 error_bdev_put:
-	fput(bdev_file);
+	bdev_fput(bdev_file);
 
 	return device;
 }
@@ -2294,7 +2294,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info,
 	 * free the device.
 	 *
 	 * We cannot call btrfs_close_bdev() here because we're holding the sb
-	 * write lock, and fput() on the block device will pull in the
+	 * write lock, and bdev_fput() on the block device will pull in the
 	 * ->open_mutex on the block device and it's dependencies.  Instead
 	 *  just flush the device and let the caller do the final bdev_release.
 	 */
@@ -2473,7 +2473,7 @@ int btrfs_get_dev_args_from_path(struct btrfs_fs_info *fs_info,
 	else
 		memcpy(args->fsid, disk_super->fsid, BTRFS_FSID_SIZE);
 	btrfs_release_disk_super(disk_super);
-	fput(bdev_file);
+	bdev_fput(bdev_file);
 	return 0;
 }
 
@@ -2921,7 +2921,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 error_free_device:
 	btrfs_free_device(device);
 error:
-	fput(bdev_file);
+	bdev_fput(bdev_file);
 	if (locked) {
 		mutex_unlock(&uuid_mutex);
 		up_write(&sb->s_umount);
-- 
2.49.0

[PATCH v3 5/6] btrfs: delay btrfs_open_devices() until super block is created

[Qu Wenruo]

Currently btrfs always call btrfs_open_devices() before creating the
super block.

It's fine for now because:

- No blk_holder_ops is provided
- btrfs_fs_type is used as a holder

This means no matter who wins the device opening race, the holder will be
the same thus not affecting the later sget_fc() race.

And since no blk_holder_ops is provided, no bdev operation is depending on
the holder.

But this will no longer be true if we want to (we indeed want) implement
a proper blk_holder_ops using fs_holder_ops.

This means we will need a proper super block as the bdev holder.

To prepare for such change, delay the btrfs_open_devices() call until we
got a super block.
This is done by extending uuid_mutex to cover sget_fc(), so that we can
call btrfs_open_devices() after sget_fc().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/super.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 41c824d216fc..36c9df82efdf 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1841,7 +1841,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);
@@ -1864,19 +1863,11 @@ 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);
-	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;
-
 	sb = sget_fc(fc, btrfs_fc_test_super, set_anon_super_fc);
-	if (IS_ERR(sb))
+	if (IS_ERR(sb)) {
+		mutex_unlock(&uuid_mutex);
 		return PTR_ERR(sb);
+	}
 
 	set_device_specific_options(fs_info);
 
@@ -1888,12 +1879,16 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 		 *
 		 * fc->s_fs_info is not touched and will be later freed by
 		 * put_fs_context() through btrfs_free_fs_context().
-		 * 
-		 * And the fs_info->fs_devices will also be closed by
-		 * btrfs_free_fs_context().
 		 */
 		ASSERT(fc->s_fs_info == fs_info);
 
+		/*
+		 * But the fs_info->fs_devices is not opened, we should not let
+		 * btrfs_free_fs_context() to close them.
+		 */
+		fs_info->fs_devices = NULL;
+		mutex_unlock(&uuid_mutex);
+
 		/*
 		 * At this stage we may have RO flag mismatch between
 		 * fc->sb_flags and sb->s_flags.  Caller should detect such
@@ -1901,6 +1896,8 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 		 * needed.
 		 */
 	} else {
+		struct block_device *bdev;
+
 		/*
 		 * The first mount of the fs thus a new superblock, fc->s_fs_info
 		 * should be NULL, and the owner ship of our fs_info and fs_devices is
@@ -1908,6 +1905,17 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 		 */
 		ASSERT(fc->s_fs_info == NULL);
 
+		ret = btrfs_open_devices(fs_devices, mode, &btrfs_fs_type);
+		mutex_unlock(&uuid_mutex);
+		if (ret < 0) {
+			deactivate_locked_super(sb);
+			return ret;
+		}
+		if (!(fc->sb_flags & SB_RDONLY) && fs_devices->rw_devices == 0) {
+			deactivate_locked_super(sb);
+			return -EACCES;
+		}
+		bdev = fs_devices->latest_dev->bdev;
 		snprintf(sb->s_id, sizeof(sb->s_id), "%pg", bdev);
 		shrinker_debugfs_rename(sb->s_shrink, "sb-btrfs:%s", sb->s_id);
 		btrfs_sb(sb)->bdev_holder = &btrfs_fs_type;
-- 
2.49.0

[PATCH v3 6/6] btrfs: use the super_block as holder when mounting file systems

[Qu Wenruo]

From: Christoph Hellwig <hch@lst.de>

The file system type is not a very useful holder as it doesn't allow us
to go back to the actual file system instance.  Pass the super_block
instead which is useful when passed back to the file system driver.

This matches what is done for all other block device based file systems,
and allows us to remove btrfs_fs_info::bdev_holder completely.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/dev-replace.c | 2 +-
 fs/btrfs/fs.h          | 2 --
 fs/btrfs/super.c       | 3 +--
 fs/btrfs/volumes.c     | 4 ++--
 4 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 6576867cc1e9..c23847de4e99 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -250,7 +250,7 @@ static int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 	}
 
 	bdev_file = bdev_file_open_by_path(device_path, BLK_OPEN_WRITE,
-					fs_info->bdev_holder, NULL);
+					   fs_info->sb, NULL);
 	if (IS_ERR(bdev_file)) {
 		btrfs_err(fs_info, "target device %s is invalid!", device_path);
 		return PTR_ERR(bdev_file);
diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
index b239e4b8421c..d90304d4e32c 100644
--- a/fs/btrfs/fs.h
+++ b/fs/btrfs/fs.h
@@ -715,8 +715,6 @@ struct btrfs_fs_info {
 	u32 data_chunk_allocations;
 	u32 metadata_ratio;
 
-	void *bdev_holder;
-
 	/* Private scrub information */
 	struct mutex scrub_lock;
 	atomic_t scrubs_running;
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 36c9df82efdf..82ce0625b2f0 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1905,7 +1905,7 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 		 */
 		ASSERT(fc->s_fs_info == NULL);
 
-		ret = btrfs_open_devices(fs_devices, mode, &btrfs_fs_type);
+		ret = btrfs_open_devices(fs_devices, mode, sb);
 		mutex_unlock(&uuid_mutex);
 		if (ret < 0) {
 			deactivate_locked_super(sb);
@@ -1918,7 +1918,6 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 		bdev = fs_devices->latest_dev->bdev;
 		snprintf(sb->s_id, sizeof(sb->s_id), "%pg", 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);
 		if (ret) {
 			deactivate_locked_super(sb);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 3e701790dde9..c0e34a717f37 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2705,7 +2705,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 		return -EROFS;
 
 	bdev_file = bdev_file_open_by_path(device_path, BLK_OPEN_WRITE,
-					fs_info->bdev_holder, NULL);
+					   fs_info->sb, NULL);
 	if (IS_ERR(bdev_file))
 		return PTR_ERR(bdev_file);
 
@@ -7174,7 +7174,7 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
 	if (IS_ERR(fs_devices))
 		return fs_devices;
 
-	ret = open_fs_devices(fs_devices, BLK_OPEN_READ, fs_info->bdev_holder);
+	ret = open_fs_devices(fs_devices, BLK_OPEN_READ, fs_info->sb);
 	if (ret) {
 		free_fs_devices(fs_devices);
 		return ERR_PTR(ret);
-- 
2.49.0

Re: [PATCH v3 3/6] btrfs: call btrfs_close_devices from ->kill_sb

[Johannes Thumshirn]

On 17.06.25 07:20, Qu Wenruo wrote:
> Signed-off-by: Christoph Hellwig<hch@lst.de>
> Signed-off-by: Johannes Thumshirn<johannes.thumshirn@wdc.com>
> Signed-off-by: Qu Wenruo<wqu@suse.com>

I think you can drop my S-o-b here, as I only did that because I was 
re-sending these patches. Now that you're continuing, my S-o-b doesn't 
really apply anymore.

Re: [PATCH v3 0/6] btrfs: use the super_block as bdev holder

[David Sterba]

On Tue, Jun 17, 2025 at 02:49:33PM +0930, Qu Wenruo wrote:
> [CHANGELOG]
> v3:
> - Drop the btrfs_fs_devices::opened split
>   It turns out to cause problems during tests.
> 
> - Extra cleanup related to the btrfs_get_tree_*()
>   Now the re-entry through vfs_get_tree() is completely dropped.
> 
> - Extra comments explaining the sget_fc() behavior
> 
> - Call bdev_fput() instead of fput()
>   This alignes us to all the other fses.
> 
> - Updated patch to delay btrfs_open_devices() until sget_fc()
>   Instead of relying on the previous solution (split
>   btrfs_open_devices::opened), just expand the uuid_mutex critical
>   section.

I've added the patches to linux-next for testing.

Re: [PATCH v3 0/6] btrfs: use the super_block as bdev holder

[Qu Wenruo]

在 2025/6/19 22:59, David Sterba 写道:
> On Tue, Jun 17, 2025 at 02:49:33PM +0930, Qu Wenruo wrote:
>> [CHANGELOG]
>> v3:
>> - Drop the btrfs_fs_devices::opened split
>>    It turns out to cause problems during tests.
>>
>> - Extra cleanup related to the btrfs_get_tree_*()
>>    Now the re-entry through vfs_get_tree() is completely dropped.
>>
>> - Extra comments explaining the sget_fc() behavior
>>
>> - Call bdev_fput() instead of fput()
>>    This alignes us to all the other fses.
>>
>> - Updated patch to delay btrfs_open_devices() until sget_fc()
>>    Instead of relying on the previous solution (split
>>    btrfs_open_devices::opened), just expand the uuid_mutex critical
>>    section.
> 
> I've added the patches to linux-next for testing.

Thanks, although I have a 3-lines small patch to pass &fs_holder_ops 
into all the bdev_file_open_by_path() calls, to enable bdev freeze/thaw 
support.

That has already passed several rounds of tests here, and I believe it 
may solve the problem of btrfs corruption during hibernation/suspension.

Should I just send it right now for extra tests, or stick to my original 
plan to submit that along with the full shutdown support?

Thanks,
Qu

Re: [PATCH v3 0/6] btrfs: use the super_block as bdev holder

[David Sterba]

On Fri, Jun 20, 2025 at 08:34:05AM +0930, Qu Wenruo wrote:
> 
> 
> 在 2025/6/19 22:59, David Sterba 写道:
> > On Tue, Jun 17, 2025 at 02:49:33PM +0930, Qu Wenruo wrote:
> >> [CHANGELOG]
> >> v3:
> >> - Drop the btrfs_fs_devices::opened split
> >>    It turns out to cause problems during tests.
> >>
> >> - Extra cleanup related to the btrfs_get_tree_*()
> >>    Now the re-entry through vfs_get_tree() is completely dropped.
> >>
> >> - Extra comments explaining the sget_fc() behavior
> >>
> >> - Call bdev_fput() instead of fput()
> >>    This alignes us to all the other fses.
> >>
> >> - Updated patch to delay btrfs_open_devices() until sget_fc()
> >>    Instead of relying on the previous solution (split
> >>    btrfs_open_devices::opened), just expand the uuid_mutex critical
> >>    section.
> > 
> > I've added the patches to linux-next for testing.
> 
> Thanks, although I have a 3-lines small patch to pass &fs_holder_ops 
> into all the bdev_file_open_by_path() calls, to enable bdev freeze/thaw 
> support.
> 
> That has already passed several rounds of tests here, and I believe it 
> may solve the problem of btrfs corruption during hibernation/suspension.
> 
> Should I just send it right now for extra tests, or stick to my original 
> plan to submit that along with the full shutdown support?

Please send it seaparately for more tests, thanks.

Re: [PATCH v3 0/6] btrfs: use the super_block as bdev holder

[Qu Wenruo]

在 2025/6/19 22:59, David Sterba 写道:
> On Tue, Jun 17, 2025 at 02:49:33PM +0930, Qu Wenruo wrote:
>> [CHANGELOG]
>> v3:
>> - Drop the btrfs_fs_devices::opened split
>>    It turns out to cause problems during tests.
>>
>> - Extra cleanup related to the btrfs_get_tree_*()
>>    Now the re-entry through vfs_get_tree() is completely dropped.
>>
>> - Extra comments explaining the sget_fc() behavior
>>
>> - Call bdev_fput() instead of fput()
>>    This alignes us to all the other fses.
>>
>> - Updated patch to delay btrfs_open_devices() until sget_fc()
>>    Instead of relying on the previous solution (split
>>    btrfs_open_devices::opened), just expand the uuid_mutex critical
>>    section.
> 
> I've added the patches to linux-next for testing.
> 

Please drop the series.

The problem is exactly in the btrfs_open_devices() handling.

The new delayed behavior is causing lockdep warning, as now sget_fc() is 
called with uuid_mutex hold, this can cause a ABBA deadlock:

[   50.807904] ======================================================
[   50.809738] WARNING: possible circular locking dependency detected
[   50.811563] 6.16.0-rc1-custom+ #258 Not tainted
[   50.813040] ------------------------------------------------------
[   50.814761] mount/798 is trying to acquire lock:
[   50.816114] ffffffffa070b068 (uuid_mutex){+.+.}-{4:4}, at: 
btrfs_read_chunk_tree+0x7d/0x870 [btrfs]
[   50.818690]
[   50.818690] but task is already holding lock:
[   50.820964] ffff8881055d50e0 (&type->s_umount_key#46/1){+.+.}-{4:4}, 
at: alloc_super+0xe0/0x3c0
[   50.823553]
[   50.823553] which lock already depends on the new lock.
[   50.823553]
[   50.825852]
[   50.825852] the existing dependency chain (in reverse order) is:
[   50.827858]
[   50.827858] -> #1 (&type->s_umount_key#46/1){+.+.}-{4:4}:
[   50.829749]        down_write_nested+0x38/0xc0
[   50.831063]        alloc_super+0xe0/0x3c0
[   50.832175]        sget_fc+0x66/0x400
[   50.833203]        btrfs_get_tree+0x195/0x620 [btrfs]
[   50.834653]        vfs_get_tree+0x2c/0xe0
[   50.835767]        vfs_cmd_create+0x57/0xd0
[   50.836979]        __do_sys_fsconfig+0x4c0/0x660
[   50.838244]        do_syscall_64+0x6c/0x2c0
[   50.839397]        entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   50.840968]
[   50.840968] -> #0 (uuid_mutex){+.+.}-{4:4}:
[   50.842767]        __lock_acquire+0x14b7/0x22a0
[   50.844138]        lock_acquire+0xc9/0x300
[   50.845427]        __mutex_lock+0xb9/0xe00
[   50.846625]        btrfs_read_chunk_tree+0x7d/0x870 [btrfs]
[   50.848272]        open_ctree+0x9aa/0x1483 [btrfs]
[   50.849654]        btrfs_get_tree.cold+0xf3/0x19d [btrfs]
[   50.851173]        vfs_get_tree+0x2c/0xe0
[   50.852443]        vfs_cmd_create+0x57/0xd0
[   50.853768]        __do_sys_fsconfig+0x4c0/0x660
[   50.855094]        do_syscall_64+0x6c/0x2c0
[   50.856260]        entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   50.857773]
[   50.857773] other info that might help us debug this:
[   50.857773]
[   50.859955]  Possible unsafe locking scenario:
[   50.859955]
[   50.861558]        CPU0                    CPU1
[   50.862816]        ----                    ----
[   50.864067]   lock(&type->s_umount_key#46/1);
[   50.865367]                                lock(uuid_mutex);
[   50.866962] 
lock(&type->s_umount_key#46/1);
[   50.868821]   lock(uuid_mutex);
[   50.869716]
[   50.869716]  *** DEADLOCK ***
[   50.869716]
[   50.871325] 2 locks held by mount/798:
[   50.872370]  #0: ffff888102ea2c70 (&fc->uapi_mutex){+.+.}-{4:4}, at: 
__do_sys_fsconfig+0x48f/0x660
[   50.874799]  #1: ffff8881055d50e0 
(&type->s_umount_key#46/1){+.+.}-{4:4}, at: alloc_super+0xe0/0x3c0
[   50.877270]
[   50.877270] stack backtrace:
[   50.878480] CPU: 3 UID: 0 PID: 798 Comm: mount Not tainted 
6.16.0-rc1-custom+ #258 PREEMPT(full)
[   50.878486] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 
unknown 02/02/2022
[   50.878488] Call Trace:
[   50.878493]  <TASK>
[   50.878498]  dump_stack_lvl+0x76/0xa0
[   50.878505]  print_circular_bug.cold+0x185/0x1d0
[   50.878512]  check_noncircular+0x169/0x190
[   50.878518]  __lock_acquire+0x14b7/0x22a0
[   50.878526]  lock_acquire+0xc9/0x300
[   50.878529]  ? btrfs_read_chunk_tree+0x7d/0x870 [btrfs]
[   50.878626]  ? lock_acquire+0xc9/0x300
[   50.878628]  ? fs_reclaim_acquire+0x4f/0xe0
[   50.878634]  __mutex_lock+0xb9/0xe00
[   50.878640]  ? btrfs_read_chunk_tree+0x7d/0x870 [btrfs]
[   50.878685]  ? fs_reclaim_acquire+0x4f/0xe0
[   50.878687]  ? lock_release+0xdd/0x2e0
[   50.878690]  ? btrfs_read_chunk_tree+0x7d/0x870 [btrfs]
[   50.878726]  ? btrfs_read_chunk_tree+0x7d/0x870 [btrfs]
[   50.878759]  btrfs_read_chunk_tree+0x7d/0x870 [btrfs]
[   50.878793]  ? btrfs_root_node+0x22/0x220 [btrfs]
[   50.878844]  ? find_held_lock+0x2b/0x80
[   50.878849]  ? btrfs_root_node+0x9c/0x220 [btrfs]
[   50.878912]  ? load_super_root+0x95/0xc3 [btrfs]
[   50.878976]  open_ctree+0x9aa/0x1483 [btrfs]
[   50.879015]  btrfs_get_tree.cold+0xf3/0x19d [btrfs]
[   50.879053]  ? fscontext_read+0x11d/0x130
[   50.879056]  ? __do_sys_fsconfig+0x48f/0x660
[   50.879058]  vfs_get_tree+0x2c/0xe0
[   50.879062]  ? capable+0x3a/0x60
[   50.879066]  vfs_cmd_create+0x57/0xd0
[   50.879069]  __do_sys_fsconfig+0x4c0/0x660
[   50.879071]  do_syscall_64+0x6c/0x2c0
[   50.879077]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   50.879080] RIP: 0033:0x7f3442f9eefe
[   50.879086] Code: 73 01 c3 48 8b 0d 12 be 0c 00 f7 d8 64 89 01 48 83 
c8 ff c3 0f 1f 84 00 00 00 00 00 f3 0f 1e fa 49 89 ca b8 af 01 00 00 0f 
05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d e2 bd 0c 00 f7 d8 64 89 01 48
[   50.879088] RSP: 002b:00007ffe661382a8 EFLAGS: 00000246 ORIG_RAX: 
00000000000001af
[   50.879092] RAX: ffffffffffffffda RBX: 0000558220e95430 RCX: 
00007f3442f9eefe
[   50.879094] RDX: 0000000000000000 RSI: 0000000000000006 RDI: 
0000000000000003
[   50.879094] RBP: 00007ffe661382e0 R08: 0000000000000000 R09: 
0000000000000000
[   50.879095] R10: 0000000000000000 R11: 0000000000000246 R12: 
00007f34430c8980
[   50.879096] R13: 0000000000000000 R14: 0000558220e96720 R15: 
00007f34430bd8e0
[   50.879099]  </TASK>

I'll need to find out a way to make sget_fc() call without holding 
uuid_mutex instead.

Thanks,
Qu