Re: [PATCH 1/3] iomap: use bio_complete_in_task for buffered read failures

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

On Mon, Sep 28, 2026 at 11:11:07AM +0200, Christoph Hellwig wrote:
> Use bio_complete_in_task to defer the bio completion handler to task
> context instead of the homegrown deferral.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/iomap/bio.c | 46 ++--------------------------------------------
>  1 file changed, 2 insertions(+), 44 deletions(-)
> 
> diff --git a/fs/iomap/bio.c b/fs/iomap/bio.c
> index 48100c614431..e83524c46838 100644
> --- a/fs/iomap/bio.c
> +++ b/fs/iomap/bio.c
> @@ -9,9 +9,6 @@
>  #include "internal.h"
>  #include "trace.h"
>  
> -static DEFINE_SPINLOCK(failed_read_lock);
> -static struct bio_list failed_read_list = BIO_EMPTY_LIST;
> -
>  static u32 __iomap_read_end_io(struct bio *bio, int error)
>  {
>  	struct folio_iter fi;
> @@ -27,50 +24,11 @@ static u32 __iomap_read_end_io(struct bio *bio, int error)
>  	return folio_count;
>  }
>  
> -static void
> -iomap_fail_reads(
> -	struct work_struct	*work)
> -{
> -	struct bio		*bio;
> -	struct bio_list		tmp = BIO_EMPTY_LIST;
> -	unsigned long		flags;
> -
> -	spin_lock_irqsave(&failed_read_lock, flags);
> -	bio_list_merge_init(&tmp, &failed_read_list);
> -	spin_unlock_irqrestore(&failed_read_lock, flags);
> -
> -	while ((bio = bio_list_pop(&tmp)) != NULL) {
> -		__iomap_read_end_io(bio, blk_status_to_errno(bio->bi_status));
> -		cond_resched();
> -	}
> -}
> -
> -static DECLARE_WORK(failed_read_work, iomap_fail_reads);
> -
> -static void iomap_fail_buffered_read(struct bio *bio)
> -{
> -	unsigned long flags;
> -
> -	/*
> -	 * Bounce I/O errors to a workqueue to avoid nested i_lock acquisitions
> -	 * in the fserror code.  The caller no longer owns the bio reference
> -	 * after the spinlock drops.
> -	 */
> -	spin_lock_irqsave(&failed_read_lock, flags);
> -	if (bio_list_empty(&failed_read_list))
> -		WARN_ON_ONCE(!schedule_work(&failed_read_work));
> -	bio_list_add(&failed_read_list, bio);
> -	spin_unlock_irqrestore(&failed_read_lock, flags);
> -}
> -
>  static void iomap_read_end_io(struct bio *bio)
>  {
> -	if (bio->bi_status) {
> -		iomap_fail_buffered_read(bio);
> +	if (bio->bi_status && bio_complete_in_task(bio))

Hmm.

If bio failed, we call bio_complete_in_task to do something with the
bio.  If the bio wasn't flagged COMPLETE_IN_TASK and we're in atomic
context, then __bio_complete_in_task will queue the bio to a per-cpu
completion batch, (maybe) schedule a worker to deal with the batch,
and we're done.

If the bio was already a COMPLETE_IN_TASK bio or we're not in atomic
context, the function returns false and we fall through to the
__iomap_read_end_io call below.

That looks like a correct conversion of the iomap_fail_buffered_read
code, with only the slight change that we're using a per-cpu batch
instead of a single static list.  Right?

If yes, then
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D


>  		return;
> -	}
> -
> -	__iomap_read_end_io(bio, 0);
> +	__iomap_read_end_io(bio, blk_status_to_errno(bio->bi_status));
>  }
>  
>  u32 iomap_finish_ioend_buffered_read(struct iomap_ioend *ioend)
> -- 
> 2.53.0
>