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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2024/2/15 03:12, Johannes Thumshirn 写道:
> From: Christoph Hellwig <hch@lst.de>
> 
> 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>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>   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 8ab185182c30..4aa67e2a48f6 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1266,6 +1266,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);
> @@ -3609,7 +3611,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;
>   }
> @@ -4389,7 +4390,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 b6cadf4f21b8..51b8fd272b15 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1822,10 +1822,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;
>   
> @@ -1839,15 +1837,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 {
> @@ -1866,10 +1861,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;
>   }
>   
>   /*
> @@ -1962,10 +1953,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;
> +	}
> +

This changed quite some after commit 951a3f59d268 ("btrfs: fix mount 
failure due to remount races") and "btrfs: open code fc_mount() to avoid 
releasing s_umount rw_sempahore" (only in for-next branch).

This part will need some refresh.

Thanks,
Qu>   	/*
>   	 * 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
>