Re: [PATCH 2/6] btrfs: store encoded read state in struct btrfs_encoded_read_private

[Pavel Begunkov <asml.silence@xxxxxxxxx>]

On 8/23/24 17:27, Mark Harmstone wrote:
> Move the various stack variables needed for encoded reads into struct
> btrfs_encoded_read_private, so that we can split it into several
> functions.
>
> Signed-off-by: Mark Harmstone <maharmstone@xxxxxx>
> ---
>   fs/btrfs/btrfs_inode.h |  20 ++++-
>   fs/btrfs/inode.c       | 170 +++++++++++++++++++++--------------------
>   fs/btrfs/ioctl.c       |  60 ++++++++-------
>   3 files changed, 135 insertions(+), 115 deletions(-)
>
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index affe70929234..5cd4308bd337 100644
...
>   
> -ssize_t btrfs_encoded_read(struct file *file, loff_t offset,
> -			   struct iov_iter *iter,
> -			   struct btrfs_ioctl_encoded_io_args *encoded)
> +ssize_t btrfs_encoded_read(struct btrfs_encoded_read_private *priv)
>   {
> -	struct btrfs_inode *inode = BTRFS_I(file_inode(file));
> +	struct btrfs_inode *inode = BTRFS_I(file_inode(priv->file));
>   	struct btrfs_fs_info *fs_info = inode->root->fs_info;
>   	struct extent_io_tree *io_tree = &inode->io_tree;
>   	ssize_t ret;
> -	size_t count = iov_iter_count(iter);
>   	u64 start, lockend, disk_bytenr, disk_io_size;
> -	struct extent_state *cached_state = NULL;
>   	struct extent_map *em;
>   	bool unlocked = false;
>   
> -	file_accessed(file);
> +	priv->count = iov_iter_count(&priv->iter);
> +
> +	file_accessed(priv->file);
>   
>   	btrfs_inode_lock(inode, BTRFS_ILOCK_SHARED);

Request submission should usually be short and not block
on IO or any locks that might wait for IO.

bool nowait = issue_flags & IO_URING_F_NONBLOCK;

btrfs_encoded_read(..., nowait) {
	f = BTRFS_ILOCK_SHARED;
	if (nowait)
		f |= BTRFS_ILOCK_TRY;
	if (btrfs_inode_lock(inode, f))
		return -EAGAIN; // io_uring will retry from a blocking context
}

so it might need sth like this here as well as for
filemap waiting and possibly other places.

>   
> -	if (offset >= inode->vfs_inode.i_size) {
> +	if (priv->args.offset >= inode->vfs_inode.i_size) {
>   		btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
>   		return 0;
>   	}
> -	start = ALIGN_DOWN(offset, fs_info->sectorsize);
> +	start = ALIGN_DOWN(priv->args.offset, fs_info->sectorsize);
>   	/*
> -	 * We don't know how long the extent containing offset is, but if
> -	 * it's compressed we know that it won't be longer than this.
> +	 * We don't know how long the extent containing priv->args.offset is,
> +	 * but if it's compressed we know that it won't be longer than this.
>   	 */
>   	lockend = start + BTRFS_MAX_UNCOMPRESSED - 1;
>   
...

--
Pavel Begunkov