use the super_block as bdev holder

use the super_block as bdev holder

[Christoph Hellwig]

Hi all,

this series contains the btrfs parts of the "remove get_super" from June
that managed to get lost.

I've dropped all the reviews from back then as the rebase against the new
mount API conversion led to a lot of non-trivial conflicts.

Josef kindly ran it through the CI farm and provided a fixup based on that.

Diffstat:
 disk-io.c |    4 +--
 super.c   |   71 ++++++++++++++++++++++++++++++++++----------------------------
 volumes.c |   60 +++++++++++++++++++++++++++-------------------------
 volumes.h |    8 ++++--
 4 files changed, 78 insertions(+), 65 deletions(-)

[PATCH 1/5] btrfs: always open the device read-only in btrfs_scan_one_device

[Christoph Hellwig]

btrfs_scan_one_device opens the block device only to read the super
block.  Instead of passing a blk_mode_t argument to sometimes open
it for writing, just hard code BLK_OPEN_READ as it will never write
to the device or hand the block_device out to someone else.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/super.c   | 9 ++++-----
 fs/btrfs/volumes.c | 4 ++--
 fs/btrfs/volumes.h | 2 +-
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 3a677b808f0f75..ba16ade1d79aea 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -295,10 +295,9 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 		break;
 	case Opt_device: {
 		struct btrfs_device *device;
-		blk_mode_t mode = sb_open_mode(fc->sb_flags);
 
 		mutex_lock(&uuid_mutex);
-		device = btrfs_scan_one_device(param->string, mode, false);
+		device = btrfs_scan_one_device(param->string, false);
 		mutex_unlock(&uuid_mutex);
 		if (IS_ERR(device))
 			return PTR_ERR(device);
@@ -1796,7 +1795,7 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 	 * With 'true' passed to btrfs_scan_one_device() (mount time) we expect
 	 * either a valid device or an error.
 	 */
-	device = btrfs_scan_one_device(fc->source, mode, true);
+	device = btrfs_scan_one_device(fc->source, true);
 	ASSERT(device != NULL);
 	if (IS_ERR(device)) {
 		mutex_unlock(&uuid_mutex);
@@ -2198,7 +2197,7 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
 		 * Scanning outside of mount can return NULL which would turn
 		 * into 0 error code.
 		 */
-		device = btrfs_scan_one_device(vol->name, BLK_OPEN_READ, false);
+		device = btrfs_scan_one_device(vol->name, false);
 		ret = PTR_ERR_OR_ZERO(device);
 		mutex_unlock(&uuid_mutex);
 		break;
@@ -2216,7 +2215,7 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
 		 * Scanning outside of mount can return NULL which would turn
 		 * into 0 error code.
 		 */
-		device = btrfs_scan_one_device(vol->name, BLK_OPEN_READ, false);
+		device = btrfs_scan_one_device(vol->name, false);
 		if (IS_ERR_OR_NULL(device)) {
 			mutex_unlock(&uuid_mutex);
 			ret = PTR_ERR(device);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4c32497311d2ff..02b61757798366 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1310,7 +1310,7 @@ int btrfs_forget_devices(dev_t devt)
  * the device or return an error. Multi-device and seeding devices are registered
  * in both cases.
  */
-struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
+struct btrfs_device *btrfs_scan_one_device(const char *path,
 					   bool mount_arg_dev)
 {
 	struct btrfs_super_block *disk_super;
@@ -1339,7 +1339,7 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
 	 * values temporarily, as the device paths of the fsid are the only
 	 * required information for assembling the volume.
 	 */
-	bdev_handle = bdev_open_by_path(path, flags, NULL, NULL);
+	bdev_handle = bdev_open_by_path(path, BLK_OPEN_READ, NULL, NULL);
 	if (IS_ERR(bdev_handle))
 		return ERR_CAST(bdev_handle);
 
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 53f87f398da779..3d35c4b92efb94 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -642,7 +642,7 @@ struct btrfs_block_group *btrfs_create_chunk(struct btrfs_trans_handle *trans,
 void btrfs_mapping_tree_free(struct btrfs_fs_info *fs_info);
 int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 		       blk_mode_t flags, void *holder);
-struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
+struct btrfs_device *btrfs_scan_one_device(const char *path,
 					   bool mount_arg_dev);
 int btrfs_forget_devices(dev_t devt);
 void btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
-- 
2.39.2

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

[Christoph Hellwig]

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 <hch@lst.de>
---
 fs/btrfs/disk-io.c |  4 ++--
 fs/btrfs/super.c   | 27 ++++++++++++++-------------
 2 files changed, 16 insertions(+), 15 deletions(-)

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
-- 
2.39.2

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

[Christian Brauner]

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.

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

[Eric Biggers]

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 3/5] btrfs: split btrfs_fs_devices.opened

[Christoph Hellwig]

The btrfs_fs_devices.opened member mixes an in use counter for the
fs_devices structure that prevents it from being garbage collected with
a flag if the underlying devices were actually opened.  This not only
makes the code hard to follow, but also prevents btrfs from switching
to opening the block device only after super block creation.  Split it
into an in_use counter and an is_open boolean flag instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/volumes.c | 51 ++++++++++++++++++++++++++--------------------
 fs/btrfs/volumes.h |  6 ++++--
 2 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 02b61757798366..65d28c79402091 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -414,7 +414,8 @@ static void free_fs_devices(struct btrfs_fs_devices *fs_devices)
 {
 	struct btrfs_device *device;
 
-	WARN_ON(fs_devices->opened);
+	WARN_ON_ONCE(fs_devices->in_use);
+	WARN_ON_ONCE(fs_devices->is_open);
 	while (!list_empty(&fs_devices->devices)) {
 		device = list_entry(fs_devices->devices.next,
 				    struct btrfs_device, dev_list);
@@ -537,7 +538,7 @@ static int btrfs_free_stale_devices(dev_t devt, struct btrfs_device *skip_device
 				continue;
 			if (devt && devt != device->devt)
 				continue;
-			if (fs_devices->opened) {
+			if (fs_devices->in_use) {
 				if (devt)
 					ret = -EBUSY;
 				break;
@@ -609,7 +610,7 @@ static struct btrfs_fs_devices *find_fsid_by_device(
 	if (found_by_devt) {
 		/* Existing device. */
 		if (fsid_fs_devices == NULL) {
-			if (devt_fs_devices->opened == 0) {
+			if (devt_fs_devices->in_use == 0) {
 				/* Stale device. */
 				return NULL;
 			} else {
@@ -797,7 +798,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 	if (!device) {
 		unsigned int nofs_flag;
 
-		if (fs_devices->opened) {
+		if (fs_devices->in_use) {
 			btrfs_err(NULL,
 "device %s belongs to fsid %pU, and the fs is already mounted, scanned by %s (%d)",
 				  path, fs_devices->fsid, current->comm,
@@ -862,7 +863,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 		 * tracking a problem where systems fail mount by subvolume id
 		 * when we reject replacement on a mounted FS.
 		 */
-		if (!fs_devices->opened && found_transid < device->generation) {
+		if (!fs_devices->in_use && found_transid < device->generation) {
 			/*
 			 * That is if the FS is _not_ mounted and if you
 			 * are here, that means there is more than one
@@ -923,7 +924,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 	 * it back. We need it to pick the disk with largest generation
 	 * (as above).
 	 */
-	if (!fs_devices->opened) {
+	if (!fs_devices->in_use) {
 		device->generation = found_transid;
 		fs_devices->latest_generation = max_t(u64, found_transid,
 						fs_devices->latest_generation);
@@ -1122,15 +1123,19 @@ static void close_fs_devices(struct btrfs_fs_devices *fs_devices)
 
 	lockdep_assert_held(&uuid_mutex);
 
-	if (--fs_devices->opened > 0)
+	if (--fs_devices->in_use > 0)
 		return;
 
+	if (!fs_devices->is_open)
+		goto done;
+
 	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list)
 		btrfs_close_one_device(device);
 
 	WARN_ON(fs_devices->open_devices);
 	WARN_ON(fs_devices->rw_devices);
-	fs_devices->opened = 0;
+	fs_devices->is_open = false;
+done:
 	fs_devices->seeding = false;
 	fs_devices->fs_info = NULL;
 }
@@ -1142,7 +1147,7 @@ void btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 
 	mutex_lock(&uuid_mutex);
 	close_fs_devices(fs_devices);
-	if (!fs_devices->opened) {
+	if (!fs_devices->in_use) {
 		list_splice_init(&fs_devices->seed_list, &list);
 
 		/*
@@ -1190,7 +1195,7 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
 	if (fs_devices->open_devices == 0)
 		return -EINVAL;
 
-	fs_devices->opened = 1;
+	fs_devices->is_open = true;
 	fs_devices->latest_dev = latest_dev;
 	fs_devices->total_rw_bytes = 0;
 	fs_devices->chunk_alloc_policy = BTRFS_CHUNK_ALLOC_REGULAR;
@@ -1227,16 +1232,14 @@ 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->opened) {
-		fs_devices->opened++;
-		ret = 0;
-	} else {
+	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;
 	}
-
-	return ret;
+	fs_devices->in_use++;
+	return 0;
 }
 
 void btrfs_release_disk_super(struct btrfs_super_block *super)
@@ -2203,13 +2206,14 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info,
 	 * This can happen if cur_devices is the private seed devices list.  We
 	 * cannot call close_fs_devices() here because it expects the uuid_mutex
 	 * to be held, but in fact we don't need that for the private
-	 * seed_devices, we can simply decrement cur_devices->opened and then
+	 * seed_devices, we can simply decrement cur_devices->in_use and then
 	 * remove it from our list and free the fs_devices.
 	 */
 	if (cur_devices->num_devices == 0) {
 		list_del_init(&cur_devices->seed_list);
-		ASSERT(cur_devices->opened == 1);
-		cur_devices->opened--;
+		ASSERT(cur_devices->in_use == 1);
+		cur_devices->in_use--;
+		cur_devices->is_open = false;
 		free_fs_devices(cur_devices);
 	}
 
@@ -2439,7 +2443,8 @@ static struct btrfs_fs_devices *btrfs_init_sprout(struct btrfs_fs_info *fs_info)
 	list_add(&old_devices->fs_list, &fs_uuids);
 
 	memcpy(seed_devices, fs_devices, sizeof(*seed_devices));
-	seed_devices->opened = 1;
+	seed_devices->in_use = 1;
+	seed_devices->is_open = true;
 	INIT_LIST_HEAD(&seed_devices->devices);
 	INIT_LIST_HEAD(&seed_devices->alloc_list);
 	mutex_init(&seed_devices->device_list_mutex);
@@ -7109,7 +7114,8 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
 			return fs_devices;
 
 		fs_devices->seeding = true;
-		fs_devices->opened = 1;
+		fs_devices->in_use = 1;
+		fs_devices->is_open = true;
 		return fs_devices;
 	}
 
@@ -7126,6 +7132,7 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
 		free_fs_devices(fs_devices);
 		return ERR_PTR(ret);
 	}
+	fs_devices->in_use = 1;
 
 	if (!fs_devices->seeding) {
 		close_fs_devices(fs_devices);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 3d35c4b92efb94..a136205044898c 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -357,8 +357,10 @@ struct btrfs_fs_devices {
 
 	struct list_head seed_list;
 
-	/* Count fs-devices opened. */
-	int opened;
+	/* Count if fs_device is in used. */
+	unsigned int in_use;
+	/* True if the devices were opened. */
+	bool is_open;
 
 	/* Set when we find or add a device that doesn't have the nonrot flag set. */
 	bool rotating;
-- 
2.39.2

Re: [PATCH 3/5] btrfs: split btrfs_fs_devices.opened

[Johannes Thumshirn]

On 18.12.23 05:50, Christoph Hellwig wrote:
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 3d35c4b92efb94..a136205044898c 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -357,8 +357,10 @@ struct btrfs_fs_devices {
>   
>   	struct list_head seed_list;
>   
> -	/* Count fs-devices opened. */
> -	int opened;
> +	/* Count if fs_device is in used. */
> +	unsigned int in_use;

Can we make in_use a refcount_t? That will catch eventual miscounts. 
Also open/close devices isn't exactly a fastpath so refcount_t is good.

Re: [PATCH 3/5] btrfs: split btrfs_fs_devices.opened

[Christoph Hellwig]

On Mon, Dec 18, 2023 at 11:56:11AM +0000, Johannes Thumshirn wrote:
> >   	struct list_head seed_list;
> >   
> > -	/* Count fs-devices opened. */
> > -	int opened;
> > +	/* Count if fs_device is in used. */
> > +	unsigned int in_use;
> 
> Can we make in_use a refcount_t? That will catch eventual miscounts. 
> Also open/close devices isn't exactly a fastpath so refcount_t is good.

The refcount_t doesn't allow an increment to go from zero to 1, so
I don't think it fits here.

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

[Christoph Hellwig]

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>
---
 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 7d38f973e991f6..62a933b47e03c3 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1782,7 +1782,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 = sb_open_mode(fc->sb_flags);
@@ -1805,15 +1804,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.
@@ -1831,24 +1823,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 65d28c79402091..5ab1b7c2fa3967 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1222,8 +1222,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
@@ -1232,14 +1230,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.39.2

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

[Christoph Hellwig]

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.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/super.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 62a933b47e03c3..2dfa2274b19387 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1831,7 +1831,7 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 		struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
 
 		mutex_lock(&uuid_mutex);
-		ret = btrfs_open_devices(fs_devices, mode, &btrfs_fs_type);
+		ret = btrfs_open_devices(fs_devices, mode, sb);
 		mutex_unlock(&uuid_mutex);
 		if (ret)
 			goto error_deactivate;
@@ -1844,7 +1844,7 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 		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;
+		btrfs_sb(sb)->bdev_holder = sb;
 		ret = btrfs_fill_super(sb, fs_devices, NULL);
 		if (ret)
 			goto error_deactivate;
-- 
2.39.2

Re: [PATCH 5/5] btrfs: use the super_block as holder when mounting file systems

[Johannes Thumshirn]

On 18.12.23 05:50, Christoph Hellwig wrote:
> 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.
>

Small Nit:
ext4, f2fs and xfs use the super_block, erofs uses 'sb->s_type' as well 
here. Reiser uses the journal and so does jfs. So while these two might 
not be the best examples in the world, all other is an exaggeration.

I'd just kill the last sentence.

Re: [PATCH 5/5] btrfs: use the super_block as holder when mounting file systems

[Christoph Hellwig]

On Mon, Dec 18, 2023 at 12:14:35PM +0000, Johannes Thumshirn wrote:
> Small Nit:
> ext4, f2fs and xfs use the super_block, erofs uses 'sb->s_type' as well 
> here. Reiser uses the journal and so does jfs. So while these two might 
> not be the best examples in the world, all other is an exaggeration.

As of 6.8-rc every file system but btrfs should be using the superblock.

Re: [PATCH 5/5] btrfs: use the super_block as holder when mounting file systems

[Gao Xiang]

Hi Christoph,

On 2023/12/18 23:02, Christoph Hellwig wrote:
> On Mon, Dec 18, 2023 at 12:14:35PM +0000, Johannes Thumshirn wrote:
>> Small Nit:
>> ext4, f2fs and xfs use the super_block, erofs uses 'sb->s_type' as well
>> here. Reiser uses the journal and so does jfs. So while these two might
>> not be the best examples in the world, all other is an exaggeration.
> 
> As of 6.8-rc every file system but btrfs should be using the superblock.

Just saw this by chance.  Currently EROFS uses 'sb->s_type' to refer
external binary source devices (blobs) across different mounts.  Since
these devices are treated readonly so such external sources can be
shared between mounts as some shared data layer.

Thanks,
Gao Xiang

Re: [PATCH 5/5] btrfs: use the super_block as holder when mounting file systems

[Christoph Hellwig]

On Tue, Dec 19, 2023 at 01:33:54PM +0800, Gao Xiang wrote:
>>> ext4, f2fs and xfs use the super_block, erofs uses 'sb->s_type' as well
>>> here. Reiser uses the journal and so does jfs. So while these two might
>>> not be the best examples in the world, all other is an exaggeration.
>>
>> As of 6.8-rc every file system but btrfs should be using the superblock.
>
> Just saw this by chance.  Currently EROFS uses 'sb->s_type' to refer
> external binary source devices (blobs) across different mounts.  Since
> these devices are treated readonly so such external sources can be
> shared between mounts as some shared data layer.

Makes sense for that somewhat unusual use case.  Note that this means
you can't really use blk_holder_ops based notifications from the block
driver, but for read-only devices that's not all that important anyway.

Re: [PATCH 5/5] btrfs: use the super_block as holder when mounting file systems

[Gao Xiang]

On 2023/12/19 20:19, Christoph Hellwig wrote:
> On Tue, Dec 19, 2023 at 01:33:54PM +0800, Gao Xiang wrote:
>>>> ext4, f2fs and xfs use the super_block, erofs uses 'sb->s_type' as well
>>>> here. Reiser uses the journal and so does jfs. So while these two might
>>>> not be the best examples in the world, all other is an exaggeration.
>>>
>>> As of 6.8-rc every file system but btrfs should be using the superblock.
>>
>> Just saw this by chance.  Currently EROFS uses 'sb->s_type' to refer
>> external binary source devices (blobs) across different mounts.  Since
>> these devices are treated readonly so such external sources can be
>> shared between mounts as some shared data layer.
> 
> Makes sense for that somewhat unusual use case.  Note that this means
> you can't really use blk_holder_ops based notifications from the block
> driver, but for read-only devices that's not all that important anyway.

Yeah, anyway even if notifications is used in the future, I will think
more about this later.

Thanks,
Gao Xiang

Re: use the super_block as bdev holder

[Johannes Thumshirn]

On 18.12.23 05:49, Christoph Hellwig wrote:
> Hi all,
> 
> this series contains the btrfs parts of the "remove get_super" from June
> that managed to get lost.
> 
> I've dropped all the reviews from back then as the rebase against the new
> mount API conversion led to a lot of non-trivial conflicts.
> 
> Josef kindly ran it through the CI farm and provided a fixup based on that.
> 
> Diffstat:
>   disk-io.c |    4 +--
>   super.c   |   71 ++++++++++++++++++++++++++++++++++----------------------------
>   volumes.c |   60 +++++++++++++++++++++++++++-------------------------
>   volumes.h |    8 ++++--
>   4 files changed, 78 insertions(+), 65 deletions(-)
> 
> 

Apart from the nitpicks
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
for the series.

Can we maybe get sth like this in as well while we're at it:
Most free() type functions accept NULL pointers, so do it for 
btrfs_close_devices() as well. It's purely esthetic but the if on top of
btrfs_free_fs_info() hurts my eyes.

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index c2f57c986069..bbaf8a9b734c 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1265,8 +1265,7 @@ 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);
+       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);
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 2dfa2274b193..14874e266345 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1959,10 +1959,8 @@ static struct vfsmount 
*btrfs_reconfigure_for_mount(struct fs_context *fc)
          * 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;
-       }
+       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
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5ab1b7c2fa39..878389e9f6a4 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1145,6 +1145,9 @@ void btrfs_close_devices(struct btrfs_fs_devices 
*fs_devices)
         LIST_HEAD(list);
         struct btrfs_fs_devices *tmp;

+       if (!fs_devices)
+               return NULL;
+
         mutex_lock(&uuid_mutex);
         close_fs_devices(fs_devices);
         if (!fs_devices->in_use) {

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

[Johannes Thumshirn]

This is a series I've picked up from Christoph, it changes the
block_device's bdev holder from fs_type to the super block.

Here's the original cover letter:
Hi all,

this series contains the btrfs parts of the "remove get_super" from June
that managed to get lost.

I've dropped all the reviews from back then as the rebase against the new
mount API conversion led to a lot of non-trivial conflicts.

Josef kindly ran it through the CI farm and provided a fixup based on that.

---
Christoph Hellwig (5):
      btrfs: always open the device read-only in btrfs_scan_one_device
      btrfs: call btrfs_close_devices from ->kill_sb
      btrfs: split btrfs_fs_devices.opened
      btrfs: open block devices after superblock creation
      btrfs: use the super_block as holder when mounting file systems

 fs/btrfs/disk-io.c |  4 +--
 fs/btrfs/super.c   | 71 ++++++++++++++++++++++++++++++------------------------
 fs/btrfs/volumes.c | 60 +++++++++++++++++++++++----------------------
 fs/btrfs/volumes.h |  8 +++---
 4 files changed, 78 insertions(+), 65 deletions(-)
---
base-commit: a50d41606b333e4364844987deb1060e7ea6c038
change-id: 20240214-hch-device-open-309ef9c98c62

Best regards,
-- 
Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 3/5] btrfs: split btrfs_fs_devices.opened

[Johannes Thumshirn]

From: Christoph Hellwig <hch@lst.de>

The btrfs_fs_devices.opened member mixes an in use counter for the
fs_devices structure that prevents it from being garbage collected with
a flag if the underlying devices were actually opened.  This not only
makes the code hard to follow, but also prevents btrfs from switching
to opening the block device only after super block creation.  Split it
into an in_use counter and an is_open boolean flag instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/volumes.c | 51 +++++++++++++++++++++++++++++----------------------
 fs/btrfs/volumes.h |  6 ++++--
 2 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 44caf1a48d33..f27af155abf0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -412,7 +412,8 @@ static void free_fs_devices(struct btrfs_fs_devices *fs_devices)
 {
 	struct btrfs_device *device;
 
-	WARN_ON(fs_devices->opened);
+	WARN_ON_ONCE(fs_devices->in_use);
+	WARN_ON_ONCE(fs_devices->is_open);
 	while (!list_empty(&fs_devices->devices)) {
 		device = list_entry(fs_devices->devices.next,
 				    struct btrfs_device, dev_list);
@@ -535,7 +536,7 @@ static int btrfs_free_stale_devices(dev_t devt, struct btrfs_device *skip_device
 				continue;
 			if (devt && devt != device->devt)
 				continue;
-			if (fs_devices->opened) {
+			if (fs_devices->in_use) {
 				if (devt)
 					ret = -EBUSY;
 				break;
@@ -607,7 +608,7 @@ static struct btrfs_fs_devices *find_fsid_by_device(
 	if (found_by_devt) {
 		/* Existing device. */
 		if (fsid_fs_devices == NULL) {
-			if (devt_fs_devices->opened == 0) {
+			if (devt_fs_devices->in_use == 0) {
 				/* Stale device. */
 				return NULL;
 			} else {
@@ -795,7 +796,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 	if (!device) {
 		unsigned int nofs_flag;
 
-		if (fs_devices->opened) {
+		if (fs_devices->in_use) {
 			btrfs_err(NULL,
 "device %s belongs to fsid %pU, and the fs is already mounted, scanned by %s (%d)",
 				  path, fs_devices->fsid, current->comm,
@@ -860,7 +861,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 		 * tracking a problem where systems fail mount by subvolume id
 		 * when we reject replacement on a mounted FS.
 		 */
-		if (!fs_devices->opened && found_transid < device->generation) {
+		if (!fs_devices->in_use && found_transid < device->generation) {
 			/*
 			 * That is if the FS is _not_ mounted and if you
 			 * are here, that means there is more than one
@@ -921,7 +922,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 	 * it back. We need it to pick the disk with largest generation
 	 * (as above).
 	 */
-	if (!fs_devices->opened) {
+	if (!fs_devices->in_use) {
 		device->generation = found_transid;
 		fs_devices->latest_generation = max_t(u64, found_transid,
 						fs_devices->latest_generation);
@@ -1120,15 +1121,19 @@ static void close_fs_devices(struct btrfs_fs_devices *fs_devices)
 
 	lockdep_assert_held(&uuid_mutex);
 
-	if (--fs_devices->opened > 0)
+	if (--fs_devices->in_use > 0)
 		return;
 
+	if (!fs_devices->is_open)
+		goto done;
+
 	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list)
 		btrfs_close_one_device(device);
 
 	WARN_ON(fs_devices->open_devices);
 	WARN_ON(fs_devices->rw_devices);
-	fs_devices->opened = 0;
+	fs_devices->is_open = false;
+done:
 	fs_devices->seeding = false;
 	fs_devices->fs_info = NULL;
 }
@@ -1140,7 +1145,7 @@ void btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 
 	mutex_lock(&uuid_mutex);
 	close_fs_devices(fs_devices);
-	if (!fs_devices->opened) {
+	if (!fs_devices->in_use) {
 		list_splice_init(&fs_devices->seed_list, &list);
 
 		/*
@@ -1188,7 +1193,7 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
 	if (fs_devices->open_devices == 0)
 		return -EINVAL;
 
-	fs_devices->opened = 1;
+	fs_devices->is_open = true;
 	fs_devices->latest_dev = latest_dev;
 	fs_devices->total_rw_bytes = 0;
 	fs_devices->chunk_alloc_policy = BTRFS_CHUNK_ALLOC_REGULAR;
@@ -1225,16 +1230,14 @@ 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->opened) {
-		fs_devices->opened++;
-		ret = 0;
-	} else {
+	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;
 	}
-
-	return ret;
+	fs_devices->in_use++;
+	return 0;
 }
 
 void btrfs_release_disk_super(struct btrfs_super_block *super)
@@ -2201,13 +2204,14 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info,
 	 * This can happen if cur_devices is the private seed devices list.  We
 	 * cannot call close_fs_devices() here because it expects the uuid_mutex
 	 * to be held, but in fact we don't need that for the private
-	 * seed_devices, we can simply decrement cur_devices->opened and then
+	 * seed_devices, we can simply decrement cur_devices->in_use and then
 	 * remove it from our list and free the fs_devices.
 	 */
 	if (cur_devices->num_devices == 0) {
 		list_del_init(&cur_devices->seed_list);
-		ASSERT(cur_devices->opened == 1);
-		cur_devices->opened--;
+		ASSERT(cur_devices->in_use == 1);
+		cur_devices->in_use--;
+		cur_devices->is_open = false;
 		free_fs_devices(cur_devices);
 	}
 
@@ -2437,7 +2441,8 @@ static struct btrfs_fs_devices *btrfs_init_sprout(struct btrfs_fs_info *fs_info)
 	list_add(&old_devices->fs_list, &fs_uuids);
 
 	memcpy(seed_devices, fs_devices, sizeof(*seed_devices));
-	seed_devices->opened = 1;
+	seed_devices->in_use = 1;
+	seed_devices->is_open = true;
 	INIT_LIST_HEAD(&seed_devices->devices);
 	INIT_LIST_HEAD(&seed_devices->alloc_list);
 	mutex_init(&seed_devices->device_list_mutex);
@@ -7115,7 +7120,8 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
 			return fs_devices;
 
 		fs_devices->seeding = true;
-		fs_devices->opened = 1;
+		fs_devices->in_use = 1;
+		fs_devices->is_open = true;
 		return fs_devices;
 	}
 
@@ -7132,6 +7138,7 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
 		free_fs_devices(fs_devices);
 		return ERR_PTR(ret);
 	}
+	fs_devices->in_use = 1;
 
 	if (!fs_devices->seeding) {
 		close_fs_devices(fs_devices);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 97c7284e7565..d6dc41c62998 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -372,8 +372,10 @@ struct btrfs_fs_devices {
 
 	struct list_head seed_list;
 
-	/* Count fs-devices opened. */
-	int opened;
+	/* Count if fs_device is in used. */
+	unsigned int in_use;
+	/* True if the devices were opened. */
+	bool is_open;
 
 	/* Set when we find or add a device that doesn't have the nonrot flag set. */
 	bool rotating;

-- 
2.43.0