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

[David Sterba <dsterba@suse.cz>]

On Fri, Aug 23, 2024 at 05:27:44PM +0100, 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.

Moving local variables makes sense in some cases but I don't see reason
to move all of them.

> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
> ---
>  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
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -605,9 +605,23 @@ int btrfs_encoded_read_regular_fill_pages(struct btrfs_inode *inode,
>  					  u64 file_offset, u64 disk_bytenr,
>  					  u64 disk_io_size,
>  					  struct page **pages);
> -ssize_t btrfs_encoded_read(struct file *file, loff_t offset,
> -			   struct iov_iter *iter,
> -			   struct btrfs_ioctl_encoded_io_args *encoded);
> +
> +struct btrfs_encoded_read_private {
> +	wait_queue_head_t wait;
> +	atomic_t pending;
> +	blk_status_t status;
> +	unsigned long nr_pages;
> +	struct page **pages;
> +	struct extent_state *cached_state;

The cached state is used as a local variable that is not reused by other
functions that also take the private structure, so what's the reason to
store it here?

> +	size_t count;
> +	struct iovec iovstack[UIO_FASTIOV];
> +	struct iovec *iov;

Same, this is used in the leaf functions and not passed around.

> +	struct iov_iter iter;
> +	struct btrfs_ioctl_encoded_io_args args;
> +	struct file *file;
> +};

As a coding pattern, the structure should store data that would be
otherwise passed as parameters or repeatedly derived from the
parameters.