Re: [PATCH v2 5/7] btrfs: unlock inode and extent in caller when uring read extent fails

[Qu Wenruo <wqu@suse.com>]

在 2026/8/6 18:20, Yang Xiuwei 写道:
> btrfs_uring_read_extent() runs only after btrfs_encoded_read() has
> taken the inode shared lock and the extent lock.  On failure it used to
> unlock in out_fail, and a pages-array allocation failure returned
> -ENOMEM without unlocking at all.
> 
> Unlock in the caller instead, matching the copy_to_user() error path.
> out_fail only frees the local priv/pages allocations, and the pages
> array failure joins that path.
> 
> Fixes: 34310c442e17 ("btrfs: add io_uring command for encoded reads (ENCODED_READ ioctl)")
> Suggested-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
> Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
> ---
>   fs/btrfs/ioctl.c | 21 ++++++++++++---------
>   1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 939c4a9d47b6..b576887469bb 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4552,7 +4552,7 @@ static void btrfs_uring_read_finished(struct io_tw_req tw_req, io_tw_token_t tw)
>   	size_t page_offset;
>   	ssize_t ret;
>   
> -	/* The inode lock has already been acquired in btrfs_uring_read_extent.  */
> +	/* The inode lock has already been acquired in btrfs_encoded_read(). */
>   	btrfs_lockdep_inode_acquire(inode, i_rwsem);
>   
>   	if (priv->err) {
> @@ -4618,7 +4618,6 @@ static int btrfs_uring_read_extent(struct kiocb *iocb, struct iov_iter *iter,
>   				   struct iovec *iov, struct io_uring_cmd *cmd)
>   {
>   	struct btrfs_inode *inode = BTRFS_I(file_inode(iocb->ki_filp));
> -	struct extent_io_tree *io_tree = &inode->io_tree;
>   	struct page **pages = NULL;
>   	struct btrfs_uring_priv *priv = NULL;
>   	unsigned long nr_pages;
> @@ -4626,8 +4625,10 @@ static int btrfs_uring_read_extent(struct kiocb *iocb, struct iov_iter *iter,
>   
>   	nr_pages = DIV_ROUND_UP(disk_io_size, PAGE_SIZE);
>   	pages = kzalloc_objs(struct page *, nr_pages, GFP_NOFS);
> -	if (!pages)
> -		return -ENOMEM;
> +	if (!pages) {
> +		ret = -ENOMEM;
> +		goto out_fail;

Why go to that label? We have nothing to release, the old return -ENOMEM 
works completely fine now.

Otherwise looks good to me.

> +	}
>   	ret = btrfs_alloc_page_array(nr_pages, pages, GFP_NOFS);
>   	if (ret) {
>   		ret = -ENOMEM;
> @@ -4674,12 +4675,12 @@ static int btrfs_uring_read_extent(struct kiocb *iocb, struct iov_iter *iter,
>   	return -EIOCBQUEUED;
>   
>   out_fail:
> -	btrfs_unlock_extent(io_tree, start, lockend, &cached_state);
> -	btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
>   	kfree(priv);
> -	for (int i = 0; i < nr_pages; i++) {
> -		if (pages[i])
> -			__free_page(pages[i]);
> +	if (pages) {
> +		for (int i = 0; i < nr_pages; i++) {
> +			if (pages[i])
> +				__free_page(pages[i]);
> +		}
>   	}
>   	kfree(pages);
>   	return ret;
> @@ -4819,6 +4820,8 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
>   					      data->iov, cmd);
>   		if (ret == -EIOCBQUEUED)
>   			goto out_acct;
> +		btrfs_unlock_extent(io_tree, start, lockend, &cached_state);
> +		btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
>   	}
>   
>   out_free: