Re: [PATCH 4/7] iomap: support ioends for reads

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Feb 03, 2025 at 10:43:08AM +0100, Christoph Hellwig wrote:
> Support using the ioend structure to defer I/O completion for
> reads in addition to writes.  This requires a check for the operation
> to not merge reads and writes, and for buffere I/O a call into the

                                         buffered

> buffered read I/O completion handler from iomap_finish_ioend.  For
> direct I/O the existing call into the direct I/O completion handler
> handles reads just fine already.

Otherwise everything looks ok to me.

--D

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/iomap/buffered-io.c | 23 ++++++++++++++++++-----
>  fs/iomap/internal.h    |  3 ++-
>  fs/iomap/ioend.c       |  6 +++++-
>  3 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index eaffa23eb8e4..06990e012884 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -306,14 +306,27 @@ static void iomap_finish_folio_read(struct folio *folio, size_t off,
>  		folio_end_read(folio, uptodate);
>  }
>  
> -static void iomap_read_end_io(struct bio *bio)
> +static u32 __iomap_read_end_io(struct bio *bio, int error)
>  {
> -	int error = blk_status_to_errno(bio->bi_status);
>  	struct folio_iter fi;
> +	u32 folio_count = 0;
>  
> -	bio_for_each_folio_all(fi, bio)
> +	bio_for_each_folio_all(fi, bio) {
>  		iomap_finish_folio_read(fi.folio, fi.offset, fi.length, error);
> +		folio_count++;
> +	}
>  	bio_put(bio);
> +	return folio_count;
> +}
> +
> +static void iomap_read_end_io(struct bio *bio)
> +{
> +	__iomap_read_end_io(bio, blk_status_to_errno(bio->bi_status));
> +}
> +
> +u32 iomap_finish_ioend_buffered_read(struct iomap_ioend *ioend)
> +{
> +	return __iomap_read_end_io(&ioend->io_bio, ioend->io_error);
>  }
>  
>  struct iomap_readpage_ctx {
> @@ -1568,7 +1581,7 @@ static void iomap_finish_folio_write(struct inode *inode, struct folio *folio,
>   * state, release holds on bios, and finally free up memory.  Do not use the
>   * ioend after this.
>   */
> -u32 iomap_finish_ioend_buffered(struct iomap_ioend *ioend)
> +u32 iomap_finish_ioend_buffered_write(struct iomap_ioend *ioend)
>  {
>  	struct inode *inode = ioend->io_inode;
>  	struct bio *bio = &ioend->io_bio;
> @@ -1600,7 +1613,7 @@ static void iomap_writepage_end_bio(struct bio *bio)
>  	struct iomap_ioend *ioend = iomap_ioend_from_bio(bio);
>  
>  	ioend->io_error = blk_status_to_errno(bio->bi_status);
> -	iomap_finish_ioend_buffered(ioend);
> +	iomap_finish_ioend_buffered_write(ioend);
>  }
>  
>  /*
> diff --git a/fs/iomap/internal.h b/fs/iomap/internal.h
> index f6992a3bf66a..c824e74a3526 100644
> --- a/fs/iomap/internal.h
> +++ b/fs/iomap/internal.h
> @@ -4,7 +4,8 @@
>  
>  #define IOEND_BATCH_SIZE	4096
>  
> -u32 iomap_finish_ioend_buffered(struct iomap_ioend *ioend);
> +u32 iomap_finish_ioend_buffered_read(struct iomap_ioend *ioend);
> +u32 iomap_finish_ioend_buffered_write(struct iomap_ioend *ioend);
>  u32 iomap_finish_ioend_direct(struct iomap_ioend *ioend);
>  
>  #endif /* _IOMAP_INTERNAL_H */
> diff --git a/fs/iomap/ioend.c b/fs/iomap/ioend.c
> index 18894ebba6db..2dd29403dc10 100644
> --- a/fs/iomap/ioend.c
> +++ b/fs/iomap/ioend.c
> @@ -44,7 +44,9 @@ static u32 iomap_finish_ioend(struct iomap_ioend *ioend, int error)
>  		return 0;
>  	if (ioend->io_flags & IOMAP_IOEND_DIRECT)
>  		return iomap_finish_ioend_direct(ioend);
> -	return iomap_finish_ioend_buffered(ioend);
> +	if (bio_op(&ioend->io_bio) == REQ_OP_READ)
> +		return iomap_finish_ioend_buffered_read(ioend);
> +	return iomap_finish_ioend_buffered_write(ioend);
>  }
>  
>  /*
> @@ -83,6 +85,8 @@ EXPORT_SYMBOL_GPL(iomap_finish_ioends);
>  static bool iomap_ioend_can_merge(struct iomap_ioend *ioend,
>  		struct iomap_ioend *next)
>  {
> +	if (bio_op(&ioend->io_bio) != bio_op(&next->io_bio))
> +		return false;
>  	if (ioend->io_bio.bi_status != next->io_bio.bi_status)
>  		return false;
>  	if (next->io_flags & IOMAP_IOEND_BOUNDARY)
> -- 
> 2.45.2
> 
>