Re: [PATCH 18/22] xfs: use BIO_COMPLETE_IN_TASK for bounce buffered read I/Os

On 2026-07-23 16:49:43, Christoph Hellwig wrote:
> Stop using the xfs per-inode work struct for completing read bios, as
> unlike writes we don't want to serialize reads on a single inode as
> there is no exclusive resource contention for them.
> 
> Factor the code for kicking off a read that needs and ioend and the
> task context completion into a single helper so that it is split off
> the xfs_end_bio machinery, which is not only used for writes.
> 
> Signed-off-by: Christoph Hellwig <[email protected]>

I would probably need something like this for fsverity.

When using integrity checksums with fsverity together, both file data
ioends and fsverity metadata ioends get onto the inode queue
(without checksums there's no metadata ioends). This way worker could
self-deadlock if data ioend processed first in xfs_end_io(). It will
call verify_bio to read merkle pages which could be already waiting
in queue.

I initially considered changing xfs_end_io, for read ioends, to just
schedule them instead of adding to the queue, but decided just sort
metadata ioends first in the queue [1] as a bit simpler fix.

The difference is that work won't be scheduled on the high-priority
fsverity's workqueue. Not sure how critical this is as with
checksums all reads would be the same priority.

1: https://lore.kernel.org/fsverity/[email protected]/T/#u

-- 
- Andrey

> ---
>  fs/xfs/xfs_aops.c  | 10 ++++------
>  fs/xfs/xfs_file.c  |  9 +--------
>  fs/xfs/xfs_ioend.c | 32 +++++++++++++++++++++++++++-----
>  fs/xfs/xfs_ioend.h |  2 ++
>  4 files changed, 34 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 49d21d905cc3..76918bd15ca8 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -580,12 +580,10 @@ xfs_bio_submit_read(
>  	const struct iomap_iter		*iter,
>  	struct iomap_read_folio_ctx	*ctx)
>  {
> -	struct bio			*bio = ctx->read_ctx;
> -
> -	/* defer read completions to the ioend workqueue */
> -	iomap_init_ioend(iter->inode, bio, ctx->read_ctx_file_offset,
> -		iomap_ioend_flags(&iter->iomap));
> -	iomap_bio_submit_read_endio(iter, ctx, xfs_end_bio);
> +	xfs_ioend_submit_read(iter->inode, ctx->read_ctx,
> +			ctx->read_ctx_file_offset,
> +			iomap_ioend_flags(&iter->iomap));
> +	ctx->read_ctx = NULL;
>  }
>  
>  static const struct iomap_read_ops xfs_iomap_read_ops = {
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index c0c3a11e7ff2..d31a1dddcdc3 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -37,7 +37,6 @@
>  #include <linux/fadvise.h>
>  #include <linux/mount.h>
>  #include <linux/filelock.h>
> -#include <linux/bio-integrity.h>
>  
>  static const struct vm_operations_struct xfs_file_vm_ops;
>  
> @@ -236,14 +235,8 @@ xfs_dio_read_bounce_submit_io(
>  	struct bio		*bio,
>  	loff_t			file_offset)
>  {
> -	struct iomap_ioend	*ioend;
> -
> -	ioend = iomap_init_ioend(iter->inode, bio, file_offset,
> +	xfs_ioend_submit_read(iter->inode, bio, file_offset,
>  			iomap_ioend_flags(&iter->iomap) | IOMAP_IOEND_DIRECT);
> -	if (ioend->io_flags & IOMAP_IOEND_INTEGRITY)
> -		fs_bio_integrity_alloc(bio);
> -	bio->bi_end_io = xfs_end_bio;
> -	submit_bio(bio);
>  }
>  
>  static const struct iomap_dio_ops xfs_dio_read_bounce_ops = {
> diff --git a/fs/xfs/xfs_ioend.c b/fs/xfs/xfs_ioend.c
> index 40695d18dac0..37a3ae8066e9 100644
> --- a/fs/xfs/xfs_ioend.c
> +++ b/fs/xfs/xfs_ioend.c
> @@ -16,6 +16,32 @@
>  #include "xfs_reflink.h"
>  #include "xfs_zone_alloc.h"
>  #include "xfs_ioend.h"
> +#include <linux/bio-integrity.h>
> +
> +static void
> +xfs_end_io_read(
> +	struct bio		*bio)
> +{
> +	struct iomap_ioend	*ioend = iomap_ioend_from_bio(bio);
> +	int			error = blk_status_to_errno(bio->bi_status);
> +
> +	iomap_finish_ioends(ioend, error);
> +}
> +
> +void
> +xfs_ioend_submit_read(
> +	struct inode		*inode,
> +	struct bio		*bio,
> +	loff_t			file_offset,
> +	u16			ioend_flags)
> +{
> +	iomap_init_ioend(inode, bio, file_offset, ioend_flags);
> +	if (ioend_flags & IOMAP_IOEND_INTEGRITY)
> +		fs_bio_integrity_alloc(bio);
> +	bio->bi_end_io = xfs_end_io_read;
> +	bio_set_flag(bio, BIO_COMPLETE_IN_TASK);
> +	submit_bio(bio);
> +}
>  
>  static void
>  xfs_ioend_put_open_zones(
> @@ -148,11 +174,7 @@ xfs_end_io(
>  			io_list))) {
>  		list_del_init(&ioend->io_list);
>  		iomap_ioend_try_merge(ioend, &tmp);
> -		if (bio_op(&ioend->io_bio) == REQ_OP_READ)
> -			iomap_finish_ioends(ioend,
> -				blk_status_to_errno(ioend->io_bio.bi_status));
> -		else
> -			xfs_end_ioend_write(ioend);
> +		xfs_end_ioend_write(ioend);
>  		cond_resched();
>  	}
>  }
> diff --git a/fs/xfs/xfs_ioend.h b/fs/xfs/xfs_ioend.h
> index 525865767fca..7c2a1ea3e6ed 100644
> --- a/fs/xfs/xfs_ioend.h
> +++ b/fs/xfs/xfs_ioend.h
> @@ -12,5 +12,7 @@ static inline bool xfs_ioend_is_append(struct iomap_ioend *ioend)
>  }
>  
>  void xfs_end_bio(struct bio *bio);
> +void xfs_ioend_submit_read(struct inode *inode, struct bio *bio,
> +		loff_t file_offset, u16 ioend_flags);
>  
>  #endif /* __XFS_IOEND_H */
> -- 
> 2.53.0
> 
>