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

[Qu Wenruo <wqu@suse.com>]

在 2025/6/11 19:33, 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>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/volumes.c | 53 ++++++++++++++++++++++++++--------------------
>   fs/btrfs/volumes.h |  6 ++++--
>   2 files changed, 34 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 1ebfc69012a2..00b64f98e3bd 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -413,7 +413,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_first_entry(&fs_devices->devices,
>   					  struct btrfs_device, dev_list);
> @@ -541,7 +542,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;
> @@ -613,7 +614,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 {
> @@ -848,7 +849,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 (%d:%d) belongs to fsid %pU, and the fs is already mounted, scanned by %s (%d)",
>   				  path, MAJOR(path_devt), MINOR(path_devt),
> @@ -916,7 +917,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
> @@ -977,7 +978,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);
> @@ -1177,15 +1178,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;
>   }
> @@ -1197,7 +1202,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);
>   
>   		/*
> @@ -1253,7 +1258,7 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
>   		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;
> @@ -1306,16 +1311,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)
> @@ -1407,7 +1410,7 @@ static bool btrfs_skip_registration(struct btrfs_super_block *disk_super,
>   
>   		mutex_lock(&fs_devices->device_list_mutex);
>   
> -		if (!fs_devices->opened) {
> +		if (!fs_devices->is_open) {
>   			mutex_unlock(&fs_devices->device_list_mutex);
>   			continue;
>   		}
> @@ -2314,13 +2317,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, "opened=%d", cur_devices->opened);
> -		cur_devices->opened--;
> +		ASSERT(cur_devices->in_use == 1, "opened=%d", cur_devices->in_use);
> +		cur_devices->in_use--;
> +		cur_devices->is_open = false;
>   		free_fs_devices(cur_devices);
>   	}
>   
> @@ -2549,7 +2553,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);
> @@ -7162,7 +7167,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;
>   	}
>   
> @@ -7179,6 +7185,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 afa71d315c46..06cf8c99befe 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -419,8 +419,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;