xfs_buf_submit error handling fix v2

xfs_buf_submit error handling fix v2

[Christoph Hellwig]

Hi all,

currently xfs_buf_submit can execute the endio handling twice when failing
for a synchronous submission.  This series refactors the surrounding code
a bit to allow for a non-hacky fix and then applies that.

Changes since v1:
 - fix a commit message typo


Diffstat:
 xfs_buf.c      |   86 ++++++++++++++++++++++++++++-----------------------------
 xfs_buf.h      |    2 -
 xfs_buf_item.c |    3 -
 xfs_inode.c    |    3 -
 4 files changed, 46 insertions(+), 48 deletions(-)

[PATCH 1/6] xfs: open code xfs_buf_ioend_fail in xfs_buf_submit

[Christoph Hellwig]

This better integrates with the other failure handling in xfs_buf_submit,
and prepares for a better API in xfs_buf_ioend_fail.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_buf.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 2a7d696d394a..a108d31996f2 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1383,10 +1383,8 @@ xfs_buf_submit(
 	 * state here rather than mount state to avoid corrupting the log tail
 	 * on shutdown.
 	 */
-	if (bp->b_mount->m_log && xlog_is_shutdown(bp->b_mount->m_log)) {
-		xfs_buf_ioend_fail(bp);
-		return;
-	}
+	if (bp->b_mount->m_log && xlog_is_shutdown(bp->b_mount->m_log))
+		goto ioerror;
 
 	if (bp->b_flags & XBF_WRITE)
 		xfs_buf_wait_unpin(bp);
@@ -1399,17 +1397,22 @@ xfs_buf_submit(
 
 	if ((bp->b_flags & XBF_WRITE) && !xfs_buf_verify_write(bp)) {
 		xfs_force_shutdown(bp->b_mount, SHUTDOWN_CORRUPT_INCORE);
-		xfs_buf_ioend(bp);
-		return;
+		goto end_io;
 	}
 
 	/* In-memory targets are directly mapped, no I/O required. */
-	if (xfs_buftarg_is_mem(bp->b_target)) {
-		xfs_buf_ioend(bp);
-		return;
-	}
+	if (xfs_buftarg_is_mem(bp->b_target))
+		goto end_io;
 
 	xfs_buf_submit_bio(bp);
+	return;
+
+ioerror:
+	bp->b_flags &= ~XBF_DONE;
+	xfs_buf_stale(bp);
+	xfs_buf_ioerror(bp, -EIO);
+end_io:
+	xfs_buf_ioend(bp);
 }
 
 /*
-- 
2.53.0

[PATCH 2/6] xfs: also mark the buffer stale on verifier failure in xfs_buf_submit

[Christoph Hellwig]

We should treat the buffer that caused a shutdown the same as handling
buffers after a shutdown, so use the same stale && !DONE logic here.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_buf.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index a108d31996f2..5ce48d8062fa 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1383,8 +1383,10 @@ xfs_buf_submit(
 	 * state here rather than mount state to avoid corrupting the log tail
 	 * on shutdown.
 	 */
-	if (bp->b_mount->m_log && xlog_is_shutdown(bp->b_mount->m_log))
+	if (bp->b_mount->m_log && xlog_is_shutdown(bp->b_mount->m_log)) {
+		xfs_buf_ioerror(bp, -EIO);
 		goto ioerror;
+	}
 
 	if (bp->b_flags & XBF_WRITE)
 		xfs_buf_wait_unpin(bp);
@@ -1397,7 +1399,7 @@ xfs_buf_submit(
 
 	if ((bp->b_flags & XBF_WRITE) && !xfs_buf_verify_write(bp)) {
 		xfs_force_shutdown(bp->b_mount, SHUTDOWN_CORRUPT_INCORE);
-		goto end_io;
+		goto ioerror;
 	}
 
 	/* In-memory targets are directly mapped, no I/O required. */
@@ -1410,7 +1412,6 @@ xfs_buf_submit(
 ioerror:
 	bp->b_flags &= ~XBF_DONE;
 	xfs_buf_stale(bp);
-	xfs_buf_ioerror(bp, -EIO);
 end_io:
 	xfs_buf_ioend(bp);
 }
-- 
2.53.0

Re: [PATCH 2/6] xfs: also mark the buffer stale on verifier failure in xfs_buf_submit

[Darrick J. Wong]

On Thu, Jun 25, 2026 at 03:58:32PM +0200, Christoph Hellwig wrote:
> We should treat the buffer that caused a shutdown the same as handling
> buffers after a shutdown, so use the same stale && !DONE logic here.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_buf.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index a108d31996f2..5ce48d8062fa 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1383,8 +1383,10 @@ xfs_buf_submit(
>  	 * state here rather than mount state to avoid corrupting the log tail
>  	 * on shutdown.
>  	 */
> -	if (bp->b_mount->m_log && xlog_is_shutdown(bp->b_mount->m_log))
> +	if (bp->b_mount->m_log && xlog_is_shutdown(bp->b_mount->m_log)) {
> +		xfs_buf_ioerror(bp, -EIO);
>  		goto ioerror;

IDGI.  If the filesystem was already shut down, we set EIO on the
buffer, stale it, and complete the ioend...

> +	}
>  
>  	if (bp->b_flags & XBF_WRITE)
>  		xfs_buf_wait_unpin(bp);
> @@ -1397,7 +1399,7 @@ xfs_buf_submit(
>  
>  	if ((bp->b_flags & XBF_WRITE) && !xfs_buf_verify_write(bp)) {
>  		xfs_force_shutdown(bp->b_mount, SHUTDOWN_CORRUPT_INCORE);
> -		goto end_io;
> +		goto ioerror;

...but if bp itself causes the shutdown, we now leave b_error at 0, but
do the same stale-and-complete thing?  Shouldn't we set EIO on the
buffer in both cases?

--D

>  	}
>  
>  	/* In-memory targets are directly mapped, no I/O required. */
> @@ -1410,7 +1412,6 @@ xfs_buf_submit(
>  ioerror:
>  	bp->b_flags &= ~XBF_DONE;
>  	xfs_buf_stale(bp);
> -	xfs_buf_ioerror(bp, -EIO);
>  end_io:
>  	xfs_buf_ioend(bp);
>  }
> -- 
> 2.53.0
> 
> 

Re: [PATCH 2/6] xfs: also mark the buffer stale on verifier failure in xfs_buf_submit

[Christoph Hellwig]

On Thu, Jun 25, 2026 at 11:00:02AM -0700, Darrick J. Wong wrote:
> > -	if (bp->b_mount->m_log && xlog_is_shutdown(bp->b_mount->m_log))
> > +	if (bp->b_mount->m_log && xlog_is_shutdown(bp->b_mount->m_log)) {
> > +		xfs_buf_ioerror(bp, -EIO);
> >  		goto ioerror;
> 
> IDGI.  If the filesystem was already shut down, we set EIO on the
> buffer, stale it, and complete the ioend...
> 
> > +	}
> >  
> >  	if (bp->b_flags & XBF_WRITE)
> >  		xfs_buf_wait_unpin(bp);
> > @@ -1397,7 +1399,7 @@ xfs_buf_submit(
> >  
> >  	if ((bp->b_flags & XBF_WRITE) && !xfs_buf_verify_write(bp)) {
> >  		xfs_force_shutdown(bp->b_mount, SHUTDOWN_CORRUPT_INCORE);
> > -		goto end_io;
> > +		goto ioerror;
> 
> ...but if bp itself causes the shutdown, we now leave b_error at 0, but
> do the same stale-and-complete thing?  Shouldn't we set EIO on the
> buffer in both cases?

->verify_write is expected to set b_error when it fails (and yes,
the API in that area is awful).

Re: [PATCH 2/6] xfs: also mark the buffer stale on verifier failure in xfs_buf_submit

[Darrick J. Wong]

On Fri, Jun 26, 2026 at 06:35:16AM +0200, Christoph Hellwig wrote:
> On Thu, Jun 25, 2026 at 11:00:02AM -0700, Darrick J. Wong wrote:
> > > -	if (bp->b_mount->m_log && xlog_is_shutdown(bp->b_mount->m_log))
> > > +	if (bp->b_mount->m_log && xlog_is_shutdown(bp->b_mount->m_log)) {
> > > +		xfs_buf_ioerror(bp, -EIO);
> > >  		goto ioerror;
> > 
> > IDGI.  If the filesystem was already shut down, we set EIO on the
> > buffer, stale it, and complete the ioend...
> > 
> > > +	}
> > >  
> > >  	if (bp->b_flags & XBF_WRITE)
> > >  		xfs_buf_wait_unpin(bp);
> > > @@ -1397,7 +1399,7 @@ xfs_buf_submit(
> > >  
> > >  	if ((bp->b_flags & XBF_WRITE) && !xfs_buf_verify_write(bp)) {
> > >  		xfs_force_shutdown(bp->b_mount, SHUTDOWN_CORRUPT_INCORE);
> > > -		goto end_io;
> > > +		goto ioerror;
> > 
> > ...but if bp itself causes the shutdown, we now leave b_error at 0, but
> > do the same stale-and-complete thing?  Shouldn't we set EIO on the
> > buffer in both cases?
> 
> ->verify_write is expected to set b_error when it fails (and yes,
> the API in that area is awful).

Would you mind leaving a comment here, then?

	if ((bp->b_flags & XBF_WRITE) && !xfs_buf_verify_write(bp)) {
		/* ->verify_write should have set b_error already */
		xfs_force_shutdown(...);
		goto out_ioerror;
	}

--D

Re: [PATCH 2/6] xfs: also mark the buffer stale on verifier failure in xfs_buf_submit

[Christoph Hellwig]

On Thu, Jun 25, 2026 at 11:21:46PM -0700, Darrick J. Wong wrote:
> > ->verify_write is expected to set b_error when it fails (and yes,
> > the API in that area is awful).
> 
> Would you mind leaving a comment here, then?
> 
> 	if ((bp->b_flags & XBF_WRITE) && !xfs_buf_verify_write(bp)) {
> 		/* ->verify_write should have set b_error already */
> 		xfs_force_shutdown(...);
> 		goto out_ioerror;
> 	}

Sure.

Re: [PATCH 2/6] xfs: also mark the buffer stale on verifier failure in xfs_buf_submit

[Darrick J. Wong]

On Fri, Jun 26, 2026 at 08:23:26AM +0200, Christoph Hellwig wrote:
> On Thu, Jun 25, 2026 at 11:21:46PM -0700, Darrick J. Wong wrote:
> > > ->verify_write is expected to set b_error when it fails (and yes,
> > > the API in that area is awful).
> > 
> > Would you mind leaving a comment here, then?
> > 
> > 	if ((bp->b_flags & XBF_WRITE) && !xfs_buf_verify_write(bp)) {
> > 		/* ->verify_write should have set b_error already */
> > 		xfs_force_shutdown(...);
> > 		goto out_ioerror;
> > 	}
> 
> Sure.

With the comment added,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

[PATCH 3/6] xfs: improve the xfs_buf_ioend_fail calling convention

[Christoph Hellwig]

Move setting the ASYNC flag into xfs_buf_ioend_fail, assert that the
buffer is locked as expected, and drop the confusing _ioend in the
name.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_buf.c      | 13 +++++++------
 fs/xfs/xfs_buf.h      |  2 +-
 fs/xfs/xfs_buf_item.c |  3 +--
 fs/xfs/xfs_inode.c    |  3 +--
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 5ce48d8062fa..83c74b5b7e8e 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1192,17 +1192,18 @@ xfs_buf_ioerror_alert(
 }
 
 /*
- * To simulate an I/O failure, the buffer must be locked and held with at least
- * two references.
+ * Fail a locked and referenced buffer outside the I/O path.
  *
- * The buf item reference is dropped via ioend processing. The second reference
- * is owned by the caller and is dropped on I/O completion if the buffer is
- * XBF_ASYNC.
+ * The caller transfers a reference which will be released after processing the
+ * error.
  */
 void
-xfs_buf_ioend_fail(
+xfs_buf_fail(
 	struct xfs_buf	*bp)
 {
+	ASSERT(xfs_buf_islocked(bp));
+
+	bp->b_flags |= XBF_ASYNC;
 	bp->b_flags &= ~XBF_DONE;
 	xfs_buf_stale(bp);
 	xfs_buf_ioerror(bp, -EIO);
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index b3cd1c7029f1..79cc9c3f0254 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -290,7 +290,7 @@ extern void __xfs_buf_ioerror(struct xfs_buf *bp, int error,
 		xfs_failaddr_t failaddr);
 #define xfs_buf_ioerror(bp, err) __xfs_buf_ioerror((bp), (err), __this_address)
 extern void xfs_buf_ioerror_alert(struct xfs_buf *bp, xfs_failaddr_t fa);
-void xfs_buf_ioend_fail(struct xfs_buf *);
+void xfs_buf_fail(struct xfs_buf *bp);
 void __xfs_buf_mark_corrupt(struct xfs_buf *bp, xfs_failaddr_t fa);
 #define xfs_buf_mark_corrupt(bp) __xfs_buf_mark_corrupt((bp), __this_address)
 
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 8487635579e5..1f055cd6732e 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -549,8 +549,7 @@ xfs_buf_item_unpin(
 		 * wait for the lock and then run the IO failure completion.
 		 */
 		xfs_buf_lock(bp);
-		bp->b_flags |= XBF_ASYNC;
-		xfs_buf_ioend_fail(bp);
+		xfs_buf_fail(bp);
 		return;
 	}
 
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 317eb57f989f..15279d22a894 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2646,8 +2646,7 @@ xfs_iflush_cluster(
 		 * inode cluster buffers.
 		 */
 		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
-		bp->b_flags |= XBF_ASYNC;
-		xfs_buf_ioend_fail(bp);
+		xfs_buf_fail(bp);
 		return error;
 	}
 
-- 
2.53.0

Re: [PATCH 3/6] xfs: improve the xfs_buf_ioend_fail calling convention

[Darrick J. Wong]

On Thu, Jun 25, 2026 at 03:58:33PM +0200, Christoph Hellwig wrote:
> Move setting the ASYNC flag into xfs_buf_ioend_fail, assert that the
> buffer is locked as expected, and drop the confusing _ioend in the
> name.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

Looks straightforward to me...
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_buf.c      | 13 +++++++------
>  fs/xfs/xfs_buf.h      |  2 +-
>  fs/xfs/xfs_buf_item.c |  3 +--
>  fs/xfs/xfs_inode.c    |  3 +--
>  4 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 5ce48d8062fa..83c74b5b7e8e 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1192,17 +1192,18 @@ xfs_buf_ioerror_alert(
>  }
>  
>  /*
> - * To simulate an I/O failure, the buffer must be locked and held with at least
> - * two references.
> + * Fail a locked and referenced buffer outside the I/O path.
>   *
> - * The buf item reference is dropped via ioend processing. The second reference
> - * is owned by the caller and is dropped on I/O completion if the buffer is
> - * XBF_ASYNC.
> + * The caller transfers a reference which will be released after processing the
> + * error.
>   */
>  void
> -xfs_buf_ioend_fail(
> +xfs_buf_fail(
>  	struct xfs_buf	*bp)
>  {
> +	ASSERT(xfs_buf_islocked(bp));
> +
> +	bp->b_flags |= XBF_ASYNC;
>  	bp->b_flags &= ~XBF_DONE;
>  	xfs_buf_stale(bp);
>  	xfs_buf_ioerror(bp, -EIO);
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index b3cd1c7029f1..79cc9c3f0254 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -290,7 +290,7 @@ extern void __xfs_buf_ioerror(struct xfs_buf *bp, int error,
>  		xfs_failaddr_t failaddr);
>  #define xfs_buf_ioerror(bp, err) __xfs_buf_ioerror((bp), (err), __this_address)
>  extern void xfs_buf_ioerror_alert(struct xfs_buf *bp, xfs_failaddr_t fa);
> -void xfs_buf_ioend_fail(struct xfs_buf *);
> +void xfs_buf_fail(struct xfs_buf *bp);
>  void __xfs_buf_mark_corrupt(struct xfs_buf *bp, xfs_failaddr_t fa);
>  #define xfs_buf_mark_corrupt(bp) __xfs_buf_mark_corrupt((bp), __this_address)
>  
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 8487635579e5..1f055cd6732e 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -549,8 +549,7 @@ xfs_buf_item_unpin(
>  		 * wait for the lock and then run the IO failure completion.
>  		 */
>  		xfs_buf_lock(bp);
> -		bp->b_flags |= XBF_ASYNC;
> -		xfs_buf_ioend_fail(bp);
> +		xfs_buf_fail(bp);
>  		return;
>  	}
>  
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 317eb57f989f..15279d22a894 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2646,8 +2646,7 @@ xfs_iflush_cluster(
>  		 * inode cluster buffers.
>  		 */
>  		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> -		bp->b_flags |= XBF_ASYNC;
> -		xfs_buf_ioend_fail(bp);
> +		xfs_buf_fail(bp);
>  		return error;
>  	}
>  
> -- 
> 2.53.0
> 
> 

[PATCH 4/6] xfs: remove xfs_buf_ioend

[Christoph Hellwig]

There are two callers of xfs_buf_ioend, one of which always has the
XBF_ASYNC flag set.  Open code the logic in both callers to prepare for a
bug fix.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_buf.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 83c74b5b7e8e..3eea9d9ea829 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1146,18 +1146,6 @@ __xfs_buf_ioend(
 	return true;
 }
 
-static void
-xfs_buf_ioend(
-	struct xfs_buf	*bp)
-{
-	if (!__xfs_buf_ioend(bp))
-		return;
-	if (bp->b_flags & XBF_ASYNC)
-		xfs_buf_relse(bp);
-	else
-		complete(&bp->b_iowait);
-}
-
 static void
 xfs_buf_ioend_work(
 	struct work_struct	*work)
@@ -1207,7 +1195,8 @@ xfs_buf_fail(
 	bp->b_flags &= ~XBF_DONE;
 	xfs_buf_stale(bp);
 	xfs_buf_ioerror(bp, -EIO);
-	xfs_buf_ioend(bp);
+	if (__xfs_buf_ioend(bp))
+		xfs_buf_relse(bp);
 }
 
 int
@@ -1414,7 +1403,12 @@ xfs_buf_submit(
 	bp->b_flags &= ~XBF_DONE;
 	xfs_buf_stale(bp);
 end_io:
-	xfs_buf_ioend(bp);
+	if (!__xfs_buf_ioend(bp))
+		return;
+	if (bp->b_flags & XBF_ASYNC)
+		xfs_buf_relse(bp);
+	else
+		complete(&bp->b_iowait);
 }
 
 /*
-- 
2.53.0

Re: [PATCH 4/6] xfs: remove xfs_buf_ioend

[Darrick J. Wong]

On Thu, Jun 25, 2026 at 03:58:34PM +0200, Christoph Hellwig wrote:
> There are two callers of xfs_buf_ioend, one of which always has the
> XBF_ASYNC flag set.  Open code the logic in both callers to prepare for a
> bug fix.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

Seems fine to me,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_buf.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 83c74b5b7e8e..3eea9d9ea829 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1146,18 +1146,6 @@ __xfs_buf_ioend(
>  	return true;
>  }
>  
> -static void
> -xfs_buf_ioend(
> -	struct xfs_buf	*bp)
> -{
> -	if (!__xfs_buf_ioend(bp))
> -		return;
> -	if (bp->b_flags & XBF_ASYNC)
> -		xfs_buf_relse(bp);
> -	else
> -		complete(&bp->b_iowait);
> -}
> -
>  static void
>  xfs_buf_ioend_work(
>  	struct work_struct	*work)
> @@ -1207,7 +1195,8 @@ xfs_buf_fail(
>  	bp->b_flags &= ~XBF_DONE;
>  	xfs_buf_stale(bp);
>  	xfs_buf_ioerror(bp, -EIO);
> -	xfs_buf_ioend(bp);
> +	if (__xfs_buf_ioend(bp))
> +		xfs_buf_relse(bp);
>  }
>  
>  int
> @@ -1414,7 +1403,12 @@ xfs_buf_submit(
>  	bp->b_flags &= ~XBF_DONE;
>  	xfs_buf_stale(bp);
>  end_io:
> -	xfs_buf_ioend(bp);
> +	if (!__xfs_buf_ioend(bp))
> +		return;
> +	if (bp->b_flags & XBF_ASYNC)
> +		xfs_buf_relse(bp);
> +	else
> +		complete(&bp->b_iowait);
>  }
>  
>  /*
> -- 
> 2.53.0
> 
> 

[PATCH 5/6] xfs: fix handling of synchronous errors in xfs_buf_submit

[Christoph Hellwig]

Synchronous readers and writers already run __xfs_buf_ioend from
xfs_buf_iowait after being woken through bp->b_iowait, so we
should not call it here, which can lead to double completions.

Fixes: 4b90de5bc0f5 ("xfs: reduce context switches for synchronous buffered I/O")
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_buf.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 3eea9d9ea829..83549573e2cc 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1403,12 +1403,12 @@ xfs_buf_submit(
 	bp->b_flags &= ~XBF_DONE;
 	xfs_buf_stale(bp);
 end_io:
-	if (!__xfs_buf_ioend(bp))
-		return;
-	if (bp->b_flags & XBF_ASYNC)
-		xfs_buf_relse(bp);
-	else
+	if (bp->b_flags & XBF_ASYNC) {
+		if (__xfs_buf_ioend(bp))
+			xfs_buf_relse(bp);
+	} else {
 		complete(&bp->b_iowait);
+	}
 }
 
 /*
-- 
2.53.0

Re: [PATCH 5/6] xfs: fix handling of synchronous errors in xfs_buf_submit

[Darrick J. Wong]

On Thu, Jun 25, 2026 at 03:58:35PM +0200, Christoph Hellwig wrote:
> Synchronous readers and writers already run __xfs_buf_ioend from
> xfs_buf_iowait after being woken through bp->b_iowait, so we
> should not call it here, which can lead to double completions.

How hard is it to trip the double completion?  I guess all you need is
an IO failure (or a verifier failure) on a synchronous read?

> Fixes: 4b90de5bc0f5 ("xfs: reduce context switches for synchronous buffered I/O")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

If so, then

Cc: <stable@vger.kernel.org> # v6.14
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_buf.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 3eea9d9ea829..83549573e2cc 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1403,12 +1403,12 @@ xfs_buf_submit(
>  	bp->b_flags &= ~XBF_DONE;
>  	xfs_buf_stale(bp);
>  end_io:
> -	if (!__xfs_buf_ioend(bp))
> -		return;
> -	if (bp->b_flags & XBF_ASYNC)
> -		xfs_buf_relse(bp);
> -	else
> +	if (bp->b_flags & XBF_ASYNC) {
> +		if (__xfs_buf_ioend(bp))
> +			xfs_buf_relse(bp);
> +	} else {
>  		complete(&bp->b_iowait);
> +	}
>  }
>  
>  /*
> -- 
> 2.53.0
> 
> 

Re: [PATCH 5/6] xfs: fix handling of synchronous errors in xfs_buf_submit

[Christoph Hellwig]

On Thu, Jun 25, 2026 at 11:13:03AM -0700, Darrick J. Wong wrote:
> On Thu, Jun 25, 2026 at 03:58:35PM +0200, Christoph Hellwig wrote:
> > Synchronous readers and writers already run __xfs_buf_ioend from
> > xfs_buf_iowait after being woken through bp->b_iowait, so we
> > should not call it here, which can lead to double completions.
> 
> How hard is it to trip the double completion?  I guess all you need is
> an IO failure (or a verifier failure) on a synchronous read?

Yes.  Or synchronous write for that matter.  Note that due to what the
completion handling does, the double completion might not be that
harmful for synchronous I/O, but it sure as hell is wrong..

[PATCH 6/6] xfs: simplify __xfs_buf_ioend

[Christoph Hellwig]

__xfs_buf_ioend can only resubmit the buffer for asynchronous
writes, which means the retry handling xfs_buf_iowait is not needed.

Because of this can stop returning a value from __xfs_buf_ioend and
just release the buffer for async I/O that does not require retries.

Also drop the __-prefix now that the semantics are straight forward.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_buf.c | 51 ++++++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 25 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 83549573e2cc..f8511c11e017 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1098,11 +1098,17 @@ xfs_buf_ioend_handle_error(
 	return false;
 }
 
-/* returns false if the caller needs to resubmit the I/O, else true */
-static bool
-__xfs_buf_ioend(
+/*
+ * Complete a buffer read or write.
+ *
+ * Releases the buffer if the I/O was asynchronous.
+ */
+static void
+xfs_buf_ioend(
 	struct xfs_buf	*bp)
 {
+	bool		async = bp->b_flags & XBF_ASYNC;
+
 	trace_xfs_buf_iodone(bp, _RET_IP_);
 
 	if (bp->b_flags & XBF_READ) {
@@ -1116,14 +1122,16 @@ __xfs_buf_ioend(
 		if (bp->b_flags & XBF_READ_AHEAD)
 			percpu_counter_dec(&bp->b_target->bt_readahead_count);
 	} else {
-		if (!bp->b_error) {
+		if (unlikely(bp->b_error)) {
+			if (xfs_buf_ioend_handle_error(bp)) {
+				ASSERT(async);
+				return;
+			}
+		} else {
 			bp->b_flags &= ~XBF_WRITE_FAIL;
 			bp->b_flags |= XBF_DONE;
 		}
 
-		if (unlikely(bp->b_error) && xfs_buf_ioend_handle_error(bp))
-			return false;
-
 		/* clear the retry state */
 		bp->b_last_error = 0;
 		bp->b_retries = 0;
@@ -1143,18 +1151,15 @@ __xfs_buf_ioend(
 
 	bp->b_flags &= ~(XBF_READ | XBF_WRITE | XBF_READ_AHEAD |
 			 _XBF_LOGRECOVERY);
-	return true;
+	if (async)
+		xfs_buf_relse(bp);
 }
 
 static void
 xfs_buf_ioend_work(
 	struct work_struct	*work)
 {
-	struct xfs_buf		*bp =
-		container_of(work, struct xfs_buf, b_ioend_work);
-
-	if (__xfs_buf_ioend(bp))
-		xfs_buf_relse(bp);
+	xfs_buf_ioend(container_of(work, struct xfs_buf, b_ioend_work));
 }
 
 void
@@ -1195,8 +1200,7 @@ xfs_buf_fail(
 	bp->b_flags &= ~XBF_DONE;
 	xfs_buf_stale(bp);
 	xfs_buf_ioerror(bp, -EIO);
-	if (__xfs_buf_ioend(bp))
-		xfs_buf_relse(bp);
+	xfs_buf_ioend(bp);
 }
 
 int
@@ -1305,12 +1309,11 @@ xfs_buf_iowait(
 {
 	ASSERT(!(bp->b_flags & XBF_ASYNC));
 
-	do {
-		trace_xfs_buf_iowait(bp, _RET_IP_);
-		wait_for_completion(&bp->b_iowait);
-		trace_xfs_buf_iowait_done(bp, _RET_IP_);
-	} while (!__xfs_buf_ioend(bp));
+	trace_xfs_buf_iowait(bp, _RET_IP_);
+	wait_for_completion(&bp->b_iowait);
+	trace_xfs_buf_iowait_done(bp, _RET_IP_);
 
+	xfs_buf_ioend(bp);
 	return bp->b_error;
 }
 
@@ -1403,12 +1406,10 @@ xfs_buf_submit(
 	bp->b_flags &= ~XBF_DONE;
 	xfs_buf_stale(bp);
 end_io:
-	if (bp->b_flags & XBF_ASYNC) {
-		if (__xfs_buf_ioend(bp))
-			xfs_buf_relse(bp);
-	} else {
+	if (bp->b_flags & XBF_ASYNC)
+		xfs_buf_ioend(bp);
+	else
 		complete(&bp->b_iowait);
-	}
 }
 
 /*
-- 
2.53.0

Re: [PATCH 6/6] xfs: simplify __xfs_buf_ioend

[Darrick J. Wong]

On Thu, Jun 25, 2026 at 03:58:36PM +0200, Christoph Hellwig wrote:
> __xfs_buf_ioend can only resubmit the buffer for asynchronous
> writes, which means the retry handling xfs_buf_iowait is not needed.
> 
> Because of this can stop returning a value from __xfs_buf_ioend and
> just release the buffer for async I/O that does not require retries.
> 
> Also drop the __-prefix now that the semantics are straight forward.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

This makes sense to me -- only async writes (e.g. logged buffers written
by the ail) get to go through the write retry mechanism; the synchronous
writers are either regular threads and can report errors, or they're
dquot/inode items and a cluster flush just marks the log item as failed.

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_buf.c | 51 ++++++++++++++++++++++++------------------------
>  1 file changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 83549573e2cc..f8511c11e017 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1098,11 +1098,17 @@ xfs_buf_ioend_handle_error(
>  	return false;
>  }
>  
> -/* returns false if the caller needs to resubmit the I/O, else true */
> -static bool
> -__xfs_buf_ioend(
> +/*
> + * Complete a buffer read or write.
> + *
> + * Releases the buffer if the I/O was asynchronous.
> + */
> +static void
> +xfs_buf_ioend(
>  	struct xfs_buf	*bp)
>  {
> +	bool		async = bp->b_flags & XBF_ASYNC;
> +
>  	trace_xfs_buf_iodone(bp, _RET_IP_);
>  
>  	if (bp->b_flags & XBF_READ) {
> @@ -1116,14 +1122,16 @@ __xfs_buf_ioend(
>  		if (bp->b_flags & XBF_READ_AHEAD)
>  			percpu_counter_dec(&bp->b_target->bt_readahead_count);
>  	} else {
> -		if (!bp->b_error) {
> +		if (unlikely(bp->b_error)) {
> +			if (xfs_buf_ioend_handle_error(bp)) {
> +				ASSERT(async);
> +				return;
> +			}
> +		} else {
>  			bp->b_flags &= ~XBF_WRITE_FAIL;
>  			bp->b_flags |= XBF_DONE;
>  		}
>  
> -		if (unlikely(bp->b_error) && xfs_buf_ioend_handle_error(bp))
> -			return false;
> -
>  		/* clear the retry state */
>  		bp->b_last_error = 0;
>  		bp->b_retries = 0;
> @@ -1143,18 +1151,15 @@ __xfs_buf_ioend(
>  
>  	bp->b_flags &= ~(XBF_READ | XBF_WRITE | XBF_READ_AHEAD |
>  			 _XBF_LOGRECOVERY);
> -	return true;
> +	if (async)
> +		xfs_buf_relse(bp);
>  }
>  
>  static void
>  xfs_buf_ioend_work(
>  	struct work_struct	*work)
>  {
> -	struct xfs_buf		*bp =
> -		container_of(work, struct xfs_buf, b_ioend_work);
> -
> -	if (__xfs_buf_ioend(bp))
> -		xfs_buf_relse(bp);
> +	xfs_buf_ioend(container_of(work, struct xfs_buf, b_ioend_work));
>  }
>  
>  void
> @@ -1195,8 +1200,7 @@ xfs_buf_fail(
>  	bp->b_flags &= ~XBF_DONE;
>  	xfs_buf_stale(bp);
>  	xfs_buf_ioerror(bp, -EIO);
> -	if (__xfs_buf_ioend(bp))
> -		xfs_buf_relse(bp);
> +	xfs_buf_ioend(bp);
>  }
>  
>  int
> @@ -1305,12 +1309,11 @@ xfs_buf_iowait(
>  {
>  	ASSERT(!(bp->b_flags & XBF_ASYNC));
>  
> -	do {
> -		trace_xfs_buf_iowait(bp, _RET_IP_);
> -		wait_for_completion(&bp->b_iowait);
> -		trace_xfs_buf_iowait_done(bp, _RET_IP_);
> -	} while (!__xfs_buf_ioend(bp));
> +	trace_xfs_buf_iowait(bp, _RET_IP_);
> +	wait_for_completion(&bp->b_iowait);
> +	trace_xfs_buf_iowait_done(bp, _RET_IP_);
>  
> +	xfs_buf_ioend(bp);
>  	return bp->b_error;
>  }
>  
> @@ -1403,12 +1406,10 @@ xfs_buf_submit(
>  	bp->b_flags &= ~XBF_DONE;
>  	xfs_buf_stale(bp);
>  end_io:
> -	if (bp->b_flags & XBF_ASYNC) {
> -		if (__xfs_buf_ioend(bp))
> -			xfs_buf_relse(bp);
> -	} else {
> +	if (bp->b_flags & XBF_ASYNC)
> +		xfs_buf_ioend(bp);
> +	else
>  		complete(&bp->b_iowait);
> -	}
>  }
>  
>  /*
> -- 
> 2.53.0
> 
> 

xfs_buf_submit error handling fix

[Christoph Hellwig]

Hi all,

currently xfs_buf_submit can execute the endio handling twice when failing
for a synchronous submission.  This series refactors the surrounding code
a bit to allow for a non-hacky fix and then applies that.

Diffstat:
 xfs_buf.c      |   86 ++++++++++++++++++++++++++++-----------------------------
 xfs_buf.h      |    2 -
 xfs_buf_item.c |    3 -
 xfs_inode.c    |    3 -
 4 files changed, 46 insertions(+), 48 deletions(-)

[PATCH 2/6] xfs: also mark the buffer stale on verifier failure in xfs_buf_submit

[Christoph Hellwig]

We should treat the buffer that caused a shutdown the same as handling
buffers after a shutdown, so use the same stale && !DONE logic here.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 9aaa8b87fc33..60ef53acebd2 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1383,8 +1383,10 @@ xfs_buf_submit(
 	 * state here rather than mount state to avoid corrupting the log tail
 	 * on shutdown.
 	 */
-	if (bp->b_mount->m_log && xlog_is_shutdown(bp->b_mount->m_log))
+	if (bp->b_mount->m_log && xlog_is_shutdown(bp->b_mount->m_log)) {
+		xfs_buf_ioerror(bp, -EIO);
 		goto ioerror;
+	}
 
 	if (bp->b_flags & XBF_WRITE)
 		xfs_buf_wait_unpin(bp);
@@ -1397,7 +1399,7 @@ xfs_buf_submit(
 
 	if ((bp->b_flags & XBF_WRITE) && !xfs_buf_verify_write(bp)) {
 		xfs_force_shutdown(bp->b_mount, SHUTDOWN_CORRUPT_INCORE);
-		goto end_io;
+		goto ioerror;
 	}
 
 	/* In-memory targets are directly mapped, no I/O required. */
@@ -1410,7 +1412,6 @@ xfs_buf_submit(
 ioerror:
 	bp->b_flags &= ~XBF_DONE;
 	xfs_buf_stale(bp);
-	xfs_buf_ioerror(bp, -EIO);
 end_io:
 	xfs_buf_ioend(bp);
 }
-- 
2.53.0

Re: [PATCH 2/6] xfs: also mark the buffer stale on verifier failure in xfs_buf_submit

[Carlos Maiolino]

On Mon, Jun 22, 2026 at 09:30:01AM +0200, Christoph Hellwig wrote:
> We should treat the buffer that caused a shutdown the same as handling
> buffers after a shutdown, so use the same stale && !DONE logic here.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> ---
>  fs/xfs/xfs_buf.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 9aaa8b87fc33..60ef53acebd2 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1383,8 +1383,10 @@ xfs_buf_submit(
>  	 * state here rather than mount state to avoid corrupting the log tail
>  	 * on shutdown.
>  	 */
> -	if (bp->b_mount->m_log && xlog_is_shutdown(bp->b_mount->m_log))
> +	if (bp->b_mount->m_log && xlog_is_shutdown(bp->b_mount->m_log)) {
> +		xfs_buf_ioerror(bp, -EIO);
>  		goto ioerror;
> +	}
>  
>  	if (bp->b_flags & XBF_WRITE)
>  		xfs_buf_wait_unpin(bp);
> @@ -1397,7 +1399,7 @@ xfs_buf_submit(
>  
>  	if ((bp->b_flags & XBF_WRITE) && !xfs_buf_verify_write(bp)) {
>  		xfs_force_shutdown(bp->b_mount, SHUTDOWN_CORRUPT_INCORE);
> -		goto end_io;
> +		goto ioerror;
>  	}
>  
>  	/* In-memory targets are directly mapped, no I/O required. */
> @@ -1410,7 +1412,6 @@ xfs_buf_submit(
>  ioerror:
>  	bp->b_flags &= ~XBF_DONE;
>  	xfs_buf_stale(bp);
> -	xfs_buf_ioerror(bp, -EIO);
>  end_io:
>  	xfs_buf_ioend(bp);
>  }
> -- 
> 2.53.0
> 
>