[PATCH] xfs: punch COW fork delalloc on COW writeback failure

[PATCH] xfs: punch COW fork delalloc on COW writeback failure

[Ravi Singh]

When a COW writeback I/O fails, xfs_end_ioend_write() cancels the COW
fork extents and punches out any overlapping delalloc blocks.  Currently
the delalloc punch targets XFS_DATA_FORK, but on the error path
xfs_reflink_end_cow() is never called, so the COW fork extents have not
been moved to the data fork yet.  Any overlapping delalloc that needs to
be cleaned up is therefore still in the COW fork.

Punching the data fork is incorrect because it can remove delalloc
reservations that belong to a concurrent buffered write.  In particular,
the convert_delay path in xfs_buffered_write_iomap_begin() drops the
ILOCK before calling xfs_bmapi_convert_delalloc(), and a concurrent COW
writeback failure punching data fork delalloc during this window causes
xfs_bmapi_convert_one_delalloc() to find the extent missing and trigger
a WARN_ON_ONCE(whichfork != XFS_COW_FORK).

This is the same class of wrong-fork issue fixed by commit f6f91d290c8b
("xfs: punch delalloc extents from the COW fork for COW writes") for the
xfs_buffered_write_iomap_end() path.

Fix xfs_end_ioend_write() to punch delalloc from XFS_COW_FORK instead
of XFS_DATA_FORK for IOMAP_IOEND_SHARED ioends.

Fixes: 5ca5916b6bc9 ("xfs: punch out data fork delalloc blocks on COW writeback failure")
Signed-off-by: Ravi Singh <ravising@redhat.com>
---
 fs/xfs/xfs_aops.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 51293b6f331f..8268a3f80398 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -131,11 +131,11 @@ xfs_end_ioend_write(
 	}

 	/*
-	 * Clean up all COW blocks and underlying data fork delalloc blocks on
-	 * I/O error. The delalloc punch is required because this ioend was
-	 * mapped to blocks in the COW fork and the associated pages are no
-	 * longer dirty. If we don't remove delalloc blocks here, they become
-	 * stale and can corrupt free space accounting on unmount.
+	 * Clean up COW blocks and COW fork delalloc blocks on I/O error.
+	 * On the error path xfs_reflink_end_cow() is never called, so the
+	 * COW fork extents have not been moved to the data fork yet.  Any
+	 * overlapping delalloc that needs to be cleaned up is therefore
+	 * still in the COW fork.
 	 */
 	error = blk_status_to_errno(ioend->io_bio.bi_status);
 	if (unlikely(error)) {
@@ -153,7 +153,7 @@ xfs_end_ioend_write(
 		if (ioend->io_flags & IOMAP_IOEND_SHARED) {
 			ASSERT(!is_zoned);
 			xfs_reflink_cancel_cow_range(ip, offset, size, true);
-			xfs_bmap_punch_delalloc_range(ip, XFS_DATA_FORK, offset,
+			xfs_bmap_punch_delalloc_range(ip, XFS_COW_FORK, offset,
 					offset + size, NULL);
 		}
 		goto done;
--
2.39.3

Re: [PATCH] xfs: punch COW fork delalloc on COW writeback failure

[Dave Chinner]

On Thu, Jul 09, 2026 at 06:26:22AM +0000, Ravi Singh wrote:
> When a COW writeback I/O fails, xfs_end_ioend_write() cancels the COW
> fork extents and punches out any overlapping delalloc blocks.  Currently
> the delalloc punch targets XFS_DATA_FORK, but on the error path
> xfs_reflink_end_cow() is never called, so the COW fork extents have not
> been moved to the data fork yet.  Any overlapping delalloc that needs to
> be cleaned up is therefore still in the COW fork.
> 
> Punching the data fork is incorrect because it can remove delalloc
> reservations that belong to a concurrent buffered write.  In particular,
> the convert_delay path in xfs_buffered_write_iomap_begin() drops the
> ILOCK before calling xfs_bmapi_convert_delalloc(), and a concurrent COW
> writeback failure punching data fork delalloc during this window causes
> xfs_bmapi_convert_one_delalloc() to find the extent missing and trigger
> a WARN_ON_ONCE(whichfork != XFS_COW_FORK).
> 
> This is the same class of wrong-fork issue fixed by commit f6f91d290c8b
> ("xfs: punch delalloc extents from the COW fork for COW writes") for the
> xfs_buffered_write_iomap_end() path.

I know I suggested that punching the wrong fork -might- be the cause
of the bug you were trying to reproduce after noticing that commit
reference in your analysis, but I don't think my hypothesis was
correct.

I've had reason to look at xfs_reflink_cancel_cow_range() very
closely today, and I think you should, too. i.e. work out what the
COW fork should contain after it finishes running on the range the
IO spanned.

Then....

> Fix xfs_end_ioend_write() to punch delalloc from XFS_COW_FORK instead
> of XFS_DATA_FORK for IOMAP_IOEND_SHARED ioends.

... it should become clear that COW fork doesn't actually contain
any delalloc extents over the given range after the cancel operation
returns.

So even if punching the data fork is wrong or triggering some other
data fork extent manipulation race, replacing it with a cow fork
punch is not a fix for whatever the underlying issue might be.

Cheers,

Dave.
-- 
Dave Chinner
dgc@kernel.org

Re: [PATCH] xfs: punch COW fork delalloc on COW writeback failure

[Ravi Singh]

On Thu, Jul 9, 2026 at 1:28 PM Dave Chinner <dgc@kernel.org> wrote:
>
> On Thu, Jul 09, 2026 at 06:26:22AM +0000, Ravi Singh wrote:
> > When a COW writeback I/O fails, xfs_end_ioend_write() cancels the COW
> > fork extents and punches out any overlapping delalloc blocks.  Currently
> > the delalloc punch targets XFS_DATA_FORK, but on the error path
> > xfs_reflink_end_cow() is never called, so the COW fork extents have not
> > been moved to the data fork yet.  Any overlapping delalloc that needs to
> > be cleaned up is therefore still in the COW fork.
> >
> > Punching the data fork is incorrect because it can remove delalloc
> > reservations that belong to a concurrent buffered write.  In particular,
> > the convert_delay path in xfs_buffered_write_iomap_begin() drops the
> > ILOCK before calling xfs_bmapi_convert_delalloc(), and a concurrent COW
> > writeback failure punching data fork delalloc during this window causes
> > xfs_bmapi_convert_one_delalloc() to find the extent missing and trigger
> > a WARN_ON_ONCE(whichfork != XFS_COW_FORK).
> >
> > This is the same class of wrong-fork issue fixed by commit f6f91d290c8b
> > ("xfs: punch delalloc extents from the COW fork for COW writes") for the
> > xfs_buffered_write_iomap_end() path.
>
> I know I suggested that punching the wrong fork -might- be the cause
> of the bug you were trying to reproduce after noticing that commit
> reference in your analysis, but I don't think my hypothesis was
> correct.
>
> I've had reason to look at xfs_reflink_cancel_cow_range() very
> closely today, and I think you should, too. i.e. work out what the
> COW fork should contain after it finishes running on the range the
> IO spanned.
>
> Then....
>
> > Fix xfs_end_ioend_write() to punch delalloc from XFS_COW_FORK instead
> > of XFS_DATA_FORK for IOMAP_IOEND_SHARED ioends.
>
> ... it should become clear that COW fork doesn't actually contain
> any delalloc extents over the given range after the cancel operation
> returns.
>
> So even if punching the data fork is wrong or triggering some other
> data fork extent manipulation race, replacing it with a cow fork
> punch is not a fix for whatever the underlying issue might be.
>

Thanks Dave. You're right I should have looked at
xfs_reflink_cancel_cow_range() more carefully before posting.
cancel_cow_blocks() removes both delalloc and real extents from
the COW fork when cancel_real is true, so there's nothing left in
the COW fork to punch after it returns.

I'll go back and look at this more closely.

> Cheers,
>
> Dave.
> --
> Dave Chinner
> dgc@kernel.org
>