Re: [PATCH 2/5] btrfs: change btrfs_encoded_read_regular_fill_pages to take a callback

David Sterba
2024-10-21T13:21:55Z

On Mon, Oct 14, 2024 at 06:18:24PM +0100, Mark Harmstone wrote:
> Change btrfs_encoded_read_regular_fill_pages so that it takes a callback
> rather than waiting, and add new helper function btrfs_encoded_read_wait_cb
> to match the existing behaviour.
>
> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
> ---
> fs/btrfs/btrfs_inode.h | 13 +++++++-
> fs/btrfs/inode.c | 70 ++++++++++++++++++++++++++++++++----------
> fs/btrfs/send.c | 15 ++++++++-
> 3 files changed, 79 insertions(+), 19 deletions(-)
>
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index 3056c8aed8ef..6aea5bedc968 100644
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -601,10 +601,21 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
> int btrfs_writepage_cow_fixup(struct page *page);
> int btrfs_encoded_io_compression_from_extent(struct btrfs_fs_info *fs_info,
> int compress_type);
> +typedef void (btrfs_encoded_read_cb_t)(void *, int);
> +
> +struct btrfs_encoded_read_wait_ctx {
> + wait_queue_head_t wait;
> + bool done;
> + int err;

Please reorder that so it does not waste the bytes after 'done' to align
'err'

> +};
> +
> +void btrfs_encoded_read_wait_cb(void *ctx, int err);
> int btrfs_encoded_read_regular_fill_pages(struct btrfs_inode *inode,
> u64 file_offset, u64 disk_bytenr,
> u64 disk_io_size,
> - struct page **pages);
> + struct page **pages,
> + btrfs_encoded_read_cb_t cb,
> + void *cb_ctx);
> ssize_t btrfs_encoded_read(struct kiocb *iocb, struct iov_iter *iter,
> struct btrfs_ioctl_encoded_io_args *encoded);
> ssize_t btrfs_do_encoded_write(struct kiocb *iocb, struct iov_iter *from,
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index b024ebc3dcd6..b5abe98f3af4 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -9080,9 +9080,10 @@ static ssize_t btrfs_encoded_read_inline(
> }
>
> struct btrfs_encoded_read_private {
> - wait_queue_head_t wait;
> atomic_t pending;
> blk_status_t status;
> + btrfs_encoded_read_cb_t *cb;
> + void *cb_ctx;

Final version of this structure could be also reordered so it does not
leave unnecessary holes, I think status is u8 so it now fills the hole
after pending but I'm not sure now if other patches make more changes.

> };
>
> static void btrfs_encoded_read_endio(struct btrfs_bio *bbio)
> @@ -9100,26 +9101,33 @@ static void btrfs_encoded_read_endio(struct btrfs_bio *bbio)
> */
> WRITE_ONCE(priv->status, bbio->bio.bi_status);
> }
> - if (!atomic_dec_return(&priv->pending))
> - wake_up(&priv->wait);
> + if (!atomic_dec_return(&priv->pending)) {

Though it's in the original code, please rewrite the condition so it
reads as an arithmetic condition, "== 0".

> + priv->cb(priv->cb_ctx,
> + blk_status_to_errno(READ_ONCE(priv->status)));
> + kfree(priv);
> + }
> bio_put(&bbio->bio);
> }