[PATCH] xfs: revalidate cached COW fork mappings during writeback

[PATCH] xfs: revalidate cached COW fork mappings during writeback

[Anthony Vardaro (Anthropic)]

Writeback can write a folio through a cached COW fork mapping after the
blocks behind it have been freed. Since commit d9252d526ba6 ("xfs:
validate writeback mapping using data fork seq counter") a cached data
fork mapping is dropped when the fork changes, but a COW fork mapping is
accepted on range alone, and since commit 3b3508980730 ("xfs: remove
superfluous writeback mapping eof trimming") nothing trims it to EOF. So
when close() or truncate frees the post-EOF COW blocks and the file is
then appended, the same writeback pass writes the new folio into blocks
the inode no longer owns. fsync() returns 0 and the range reads back as
zeroes, or the data lands in another file.

Check cow_seq against the COW fork if_seq for COW mappings as well, and
only sample cow_seq where the mapping is built so a failed conversion
cannot pair a stale mapping with a fresh sequence number.

This costs one extent lookup and one cancelled transaction per
invalidation: about 5% more fsync time on random 4k overwrites of a
reflinked file, nothing measurable on sequential writeback.

Fixes: d9252d526ba6 ("xfs: validate writeback mapping using data fork seq counter")
Cc: stable@vger.kernel.org # v5.1
Assisted-by: Claude:unspecified
Signed-off-by: Anthony Vardaro (Anthropic) <me@anthonyvardaro.com>
---
An fstests case for this, using the wb_delay_ms error injection knob,
follows separately.

Backport note: kernels before v6.2 do not have
trace_xfs_wb_cow_iomap_invalid() (added by commit c2beff99eb03), so
drop that call there. Kernels before v5.5 test wpc->fork ==
XFS_COW_FORK instead of IOMAP_F_SHARED and have no XFS_WPC().
---
 fs/xfs/xfs_aops.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 2a0c54256..c043e5bad 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -304,12 +304,22 @@ xfs_imap_valid(
 	    offset >= wpc->iomap.offset + wpc->iomap.length)
 		return false;
 	/*
-	 * If this is a COW mapping, it is sufficient to check that the mapping
-	 * covers the offset. Be careful to check this first because the caller
-	 * can revalidate a COW mapping without updating the data seqno.
+	 * A COW mapping is only valid while the COW fork is unchanged. After a
+	 * change, the blocks behind the mapping can already be freed, for
+	 * example by the post-EOF trim on close. Do this check before the
+	 * data fork check, because the caller can revalidate a COW mapping
+	 * without updating the data seqno.
 	 */
-	if (wpc->iomap.flags & IOMAP_F_SHARED)
+	if (wpc->iomap.flags & IOMAP_F_SHARED) {
+		if (!ip->i_cowfp)
+			return false;
+		if (XFS_WPC(wpc)->cow_seq != READ_ONCE(ip->i_cowfp->if_seq)) {
+			trace_xfs_wb_cow_iomap_invalid(ip, &wpc->iomap,
+					XFS_WPC(wpc)->cow_seq, XFS_COW_FORK);
+			return false;
+		}
 		return true;
+	}
 
 	/*
 	 * This is not a COW mapping. Check the sequence number of the data fork
@@ -359,9 +369,8 @@ xfs_map_blocks(
 	/*
 	 * COW fork blocks can overlap data fork blocks even if the blocks
 	 * aren't shared.  COW I/O always takes precedent, so we must always
-	 * check for overlap on reflink inodes unless the mapping is already a
-	 * COW one, or the COW fork hasn't changed from the last time we looked
-	 * at it.
+	 * check for overlap on reflink inodes unless the COW fork hasn't
+	 * changed from the last time we looked at it.
 	 *
 	 * It's safe to check the COW fork if_seq here without the ILOCK because
 	 * we've indirectly protected against concurrent updates: writeback has
@@ -394,16 +403,14 @@ xfs_map_blocks(
 	    xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb, &icur, &imap))
 		cow_fsb = imap.br_startoff;
 	if (cow_fsb != NULLFILEOFF && cow_fsb <= offset_fsb) {
-		XFS_WPC(wpc)->cow_seq = READ_ONCE(ip->i_cowfp->if_seq);
 		xfs_iunlock(ip, XFS_ILOCK_SHARED);
-
 		whichfork = XFS_COW_FORK;
 		goto allocate_blocks;
 	}
 
 	/*
-	 * No COW extent overlap. Revalidate now that we may have updated
-	 * ->cow_seq. If the data mapping is still valid, we're done.
+	 * No COW extent overlap. If the data mapping is still valid, we're
+	 * done.
 	 */
 	if (xfs_imap_valid(wpc, ip, offset)) {
 		xfs_iunlock(ip, XFS_ILOCK_SHARED);

---
base-commit: 0877338ade31b825884a744e03f27c8de300f101
change-id: 20260813-b4-xfs-cow-wb-revalidate-0427d1fb36d7

Best regards,
--  
Anthony Vardaro (Anthropic) <me@anthonyvardaro.com>

Re: [PATCH] xfs: revalidate cached COW fork mappings during writeback

[Darrick J. Wong]

On Thu, Aug 20, 2026 at 12:21:38AM +0000, Anthony Vardaro (Anthropic) wrote:
> Writeback can write a folio through a cached COW fork mapping after the
> blocks behind it have been freed. Since commit d9252d526ba6 ("xfs:
> validate writeback mapping using data fork seq counter") a cached data
> fork mapping is dropped when the fork changes, but a COW fork mapping is
> accepted on range alone, and since commit 3b3508980730 ("xfs: remove
> superfluous writeback mapping eof trimming") nothing trims it to EOF. So
> when close() or truncate frees the post-EOF COW blocks and the file is
> then appended, the same writeback pass writes the new folio into blocks
> the inode no longer owns. fsync() returns 0 and the range reads back as
> zeroes, or the data lands in another file.
> 
> Check cow_seq against the COW fork if_seq for COW mappings as well, and
> only sample cow_seq where the mapping is built so a failed conversion
> cannot pair a stale mapping with a fresh sequence number.
> 
> This costs one extent lookup and one cancelled transaction per
> invalidation: about 5% more fsync time on random 4k overwrites of a
> reflinked file, nothing measurable on sequential writeback.
> 
> Fixes: d9252d526ba6 ("xfs: validate writeback mapping using data fork seq counter")
> Cc: stable@vger.kernel.org # v5.1
> Assisted-by: Claude:unspecified
> Signed-off-by: Anthony Vardaro (Anthropic) <me@anthonyvardaro.com>
> ---
> An fstests case for this, using the wb_delay_ms error injection knob,
> follows separately.
> 
> Backport note: kernels before v6.2 do not have
> trace_xfs_wb_cow_iomap_invalid() (added by commit c2beff99eb03), so
> drop that call there. Kernels before v5.5 test wpc->fork ==
> XFS_COW_FORK instead of IOMAP_F_SHARED and have no XFS_WPC().
> ---
>  fs/xfs/xfs_aops.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 2a0c54256..c043e5bad 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -304,12 +304,22 @@ xfs_imap_valid(
>  	    offset >= wpc->iomap.offset + wpc->iomap.length)
>  		return false;
>  	/*
> -	 * If this is a COW mapping, it is sufficient to check that the mapping
> -	 * covers the offset. Be careful to check this first because the caller
> -	 * can revalidate a COW mapping without updating the data seqno.
> +	 * A COW mapping is only valid while the COW fork is unchanged. After a
> +	 * change, the blocks behind the mapping can already be freed, for
> +	 * example by the post-EOF trim on close. Do this check before the
> +	 * data fork check, because the caller can revalidate a COW mapping
> +	 * without updating the data seqno.
>  	 */
> -	if (wpc->iomap.flags & IOMAP_F_SHARED)
> +	if (wpc->iomap.flags & IOMAP_F_SHARED) {
> +		if (!ip->i_cowfp)
> +			return false;
> +		if (XFS_WPC(wpc)->cow_seq != READ_ONCE(ip->i_cowfp->if_seq)) {

The buffered write path has similar data/cow fork sequence counter
revalidation code, so would it be a better idea to adapt the writeback
path to sample the sequence counter via xfs_iomap_inode_sequence in
xfs_map_blocks, and re-check that in xfs_imap_valid()?

I weakly hinted at this a few years ago when we were adapting the
buffered write path, see [1].

--D

[1] https://lore.kernel.org/linux-xfs/Y2mcOCpKiDb4nf1X@magnolia/

> +			trace_xfs_wb_cow_iomap_invalid(ip, &wpc->iomap,
> +					XFS_WPC(wpc)->cow_seq, XFS_COW_FORK);
> +			return false;
> +		}
>  		return true;
> +	}
>  
>  	/*
>  	 * This is not a COW mapping. Check the sequence number of the data fork
> @@ -359,9 +369,8 @@ xfs_map_blocks(
>  	/*
>  	 * COW fork blocks can overlap data fork blocks even if the blocks
>  	 * aren't shared.  COW I/O always takes precedent, so we must always
> -	 * check for overlap on reflink inodes unless the mapping is already a
> -	 * COW one, or the COW fork hasn't changed from the last time we looked
> -	 * at it.
> +	 * check for overlap on reflink inodes unless the COW fork hasn't
> +	 * changed from the last time we looked at it.
>  	 *
>  	 * It's safe to check the COW fork if_seq here without the ILOCK because
>  	 * we've indirectly protected against concurrent updates: writeback has
> @@ -394,16 +403,14 @@ xfs_map_blocks(
>  	    xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb, &icur, &imap))
>  		cow_fsb = imap.br_startoff;
>  	if (cow_fsb != NULLFILEOFF && cow_fsb <= offset_fsb) {
> -		XFS_WPC(wpc)->cow_seq = READ_ONCE(ip->i_cowfp->if_seq);
>  		xfs_iunlock(ip, XFS_ILOCK_SHARED);
> -
>  		whichfork = XFS_COW_FORK;
>  		goto allocate_blocks;
>  	}
>  
>  	/*
> -	 * No COW extent overlap. Revalidate now that we may have updated
> -	 * ->cow_seq. If the data mapping is still valid, we're done.
> +	 * No COW extent overlap. If the data mapping is still valid, we're
> +	 * done.
>  	 */
>  	if (xfs_imap_valid(wpc, ip, offset)) {
>  		xfs_iunlock(ip, XFS_ILOCK_SHARED);
> 
> ---
> base-commit: 0877338ade31b825884a744e03f27c8de300f101
> change-id: 20260813-b4-xfs-cow-wb-revalidate-0427d1fb36d7
> 
> Best regards,
> --  
> Anthony Vardaro (Anthropic) <me@anthonyvardaro.com>
> 
> 

Re: [PATCH] xfs: revalidate cached COW fork mappings during writeback

[Anthony Vardaro (Anthropic)]

On Thu, Aug 20, 2026 at 09:11:23AM -0700, Darrick J. Wong wrote:
> The buffered write path has similar data/cow fork sequence counter
> revalidation code, so would it be a better idea to adapt the writeback
> path to sample the sequence counter via xfs_iomap_inode_sequence in
> xfs_map_blocks, and re-check that in xfs_imap_valid()?
>
> I weakly hinted at this a few years ago when we were adapting the
> buffered write path, see [1].
>
> [1] https://lore.kernel.org/linux-xfs/Y2mcOCpKiDb4nf1X@magnolia/

I think that's reasonable. It's mostly deletion since
xfs_bmapi_convert_one_delalloc() already fills
wpc->iomap.validity_cookie. I'd still like 1/2 to stay the small
private cow_seq check so it can go to older stable trees, which don't
have validity_cookie or xfs_iomap_inode_sequence().

So for v2 I'll send two patches: 1/2 for this fix, Cc stable; 2/2
samples xfs_iomap_inode_sequence() in xfs_map_blocks(), compares the
cookie in xfs_imap_valid(), and removes data_seq/cow_seq, with the cost
measured again.

Anthony

Re: [PATCH] xfs: revalidate cached COW fork mappings during writeback

[Dave Chinner]

On Thu, Aug 20, 2026 at 09:11:23AM -0700, Darrick J. Wong wrote:
> On Thu, Aug 20, 2026 at 12:21:38AM +0000, Anthony Vardaro (Anthropic) wrote:
> > Writeback can write a folio through a cached COW fork mapping after the
> > blocks behind it have been freed. Since commit d9252d526ba6 ("xfs:
> > validate writeback mapping using data fork seq counter") a cached data
> > fork mapping is dropped when the fork changes, but a COW fork mapping is
> > accepted on range alone, and since commit 3b3508980730 ("xfs: remove
> > superfluous writeback mapping eof trimming") nothing trims it to EOF. So
> > when close() or truncate frees the post-EOF COW blocks and the file is
> > then appended, the same writeback pass writes the new folio into blocks
> > the inode no longer owns. fsync() returns 0 and the range reads back as
> > zeroes, or the data lands in another file.

Have you reproduced this and tested that it the change actually
fixes the supposed bug?

> > Check cow_seq against the COW fork if_seq for COW mappings as well, and
> > only sample cow_seq where the mapping is built so a failed conversion
> > cannot pair a stale mapping with a fresh sequence number.
> > 
> > This costs one extent lookup and one cancelled transaction per
> > invalidation: about 5% more fsync time on random 4k overwrites of a
> > reflinked file, nothing measurable on sequential writeback.
> > 
> > Fixes: d9252d526ba6 ("xfs: validate writeback mapping using data fork seq counter")
> > Cc: stable@vger.kernel.org # v5.1
> > Assisted-by: Claude:unspecified
> > Signed-off-by: Anthony Vardaro (Anthropic) <me@anthonyvardaro.com>
> > ---
> > An fstests case for this, using the wb_delay_ms error injection knob,
> > follows separately.
> > 
> > Backport note: kernels before v6.2 do not have
> > trace_xfs_wb_cow_iomap_invalid() (added by commit c2beff99eb03), so
> > drop that call there. Kernels before v5.5 test wpc->fork ==
> > XFS_COW_FORK instead of IOMAP_F_SHARED and have no XFS_WPC().
> > ---
> >  fs/xfs/xfs_aops.c | 29 ++++++++++++++++++-----------
> >  1 file changed, 18 insertions(+), 11 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> > index 2a0c54256..c043e5bad 100644
> > --- a/fs/xfs/xfs_aops.c
> > +++ b/fs/xfs/xfs_aops.c
> > @@ -304,12 +304,22 @@ xfs_imap_valid(
> >  	    offset >= wpc->iomap.offset + wpc->iomap.length)
> >  		return false;
> >  	/*
> > -	 * If this is a COW mapping, it is sufficient to check that the mapping
> > -	 * covers the offset. Be careful to check this first because the caller
> > -	 * can revalidate a COW mapping without updating the data seqno.
> > +	 * A COW mapping is only valid while the COW fork is unchanged. After a
> > +	 * change, the blocks behind the mapping can already be freed, for
> > +	 * example by the post-EOF trim on close. Do this check before the
> > +	 * data fork check, because the caller can revalidate a COW mapping
> > +	 * without updating the data seqno.
> >  	 */
> > -	if (wpc->iomap.flags & IOMAP_F_SHARED)
> > +	if (wpc->iomap.flags & IOMAP_F_SHARED) {
> > +		if (!ip->i_cowfp)
> > +			return false;
> > +		if (XFS_WPC(wpc)->cow_seq != READ_ONCE(ip->i_cowfp->if_seq)) {
> 
> The buffered write path has similar data/cow fork sequence counter
> revalidation code, so would it be a better idea to adapt the writeback
> path to sample the sequence counter via xfs_iomap_inode_sequence in
> xfs_map_blocks, and re-check that in xfs_imap_valid()?

Hmmmm - looking at the rest of the function, I think that using
xfs_iomap_inode_sequence() will potentially introduce a new bug...

The code 10 lines below for non-shared iomap validity
unconditionally checks the COW fork sequence number  if
xfs_inode_has_cow_data() returns true.

The code above is essentially makes it:

	if (IOMAP_F_SHARED) {
		if (!xfs_inode_has_cow_data())
			return false;
		/* check cow sequence */
		return ....
	}

	/* check data sequence */

	if (xfs_inode_has_cow_data())
		/* check cow sequence */

	return ...

IOWs, adding the seqeunce check to the SHARED iomap means we
-always- check the COW_FORK sequence number now if
xfs_inode_has_cow_data() returns true. i.e.

	if (xfs_inode_has_cow_data()) {
		/* check cow sequence */
	}
	if (IOMAP_F_SHARED)
		return false;

	/* check data sequence */

And with this, it should be obvious now why using I suspect
xfs_iomap_inode_sequence() could introduce new problems - it only
encodes the cow fork sequence number if IOMAP_F_SHARED is set.

However, looking at the reworked logic above, I think this uncovers
another bug, this one in xfs_map_blocks(). That is, xfs_map_blocks()
never samples the COW fork sequence number on pure data fork
writeback on xfs_inode_has_cow_data() inodes. Hence the "always
check the cow-fork sequence" on pure data overwrites -always- fails
on inodes with mixed data/cow overwrites, even when the cached
extent is still valid.....

So, before a fix is made, we need to decide what the correct
behaviour is for writeback on mixed mode inodes. Given the imapct of
getting this wrong, I think that should be unconditionally tossing
the cached iomap if either the cow fork or data fork changes. That
makes for simple logic, and it covers all cases where a racing
change could potentially cause an issue....

-Dave.
-- 
Dave Chinner
dgc@kernel.org

Re: [PATCH] xfs: revalidate cached COW fork mappings during writeback

[Anthony Vardaro (Anthropic)]

On Sat, Aug 22, 2026 at 08:20:05AM +1000, Dave Chinner wrote:
> Have you reproduced this and tested that it the change actually
> fixes the supposed bug?

Thank you for the feedback. I have, on 6.18.y and on current for-next
(412f89fb3988). The reproducer is a small C program that makes a reflink
clone whose size isn't cowextsize aligned, dirty a few dozen blocks so
the COW reservation gets rounded out past EOF, then start one background
pass with sync_file_range(). Once the pass has converted the first folio
and cached the mapping, close the first writable fd (or truncate to the
current size), which trims the post-EOF COW blocks, and append one
block. The pass gets to the new folio, xfs_imap_valid() is happy with
the cached mapping on range alone, and the append lands in the extent
that was just freed. After fsync and FADV_DONTNEED the block reads back
as zeroes and GETBMAPX still shows delalloc there.

With the wb_delay_ms errortag widening the gap between
xfs_map_blocks() calls it hits 25/25; in a tight loop with no
injection, 200/200; those two I ran on both trees. On 6.18.y I also
ran it with nothing but the periodic flusher, about 2%, and a variant
that lets a second file pick up the freed block first, which ends with
that file holding my appended data 4/4. With the patch every one of
those is 0/N, and xfs_wb_cow_iomap_invalid fires where the hit used
to be. I'll put the program in the v2 cover letter and turn it into
an fstests case next to xfs/558.

> So, before a fix is made, we need to decide what the correct
> behaviour is for writeback on mixed mode inodes. Given the imapct of
> getting this wrong, I think that should be unconditionally tossing
> the cached iomap if either the cow fork or data fork changes.

That works for me. I also agree xfs_iomap_inode_sequence() as it
stands would quietly drop the COW check on data fork mappings, and
you're right that the data fork path never samples cow_seq, so that
check can only ever fail today.

So for v2, xfs_map_blocks() samples both if_seq values under the
ILOCK_SHARED it already takes for the lookups, and only stores them
alongside the mapping they were taken for; xfs_imap_valid() throws out
any cached mapping, shared or not, if either one has moved. I'd keep
the private data_seq/cow_seq for now so it backports cleanly. A cookie
could replace them later but it would have to carry both forks
unconditionally.

Since that makes COW mappings revalidate a lot more often, I'd like to
add a second patch that maps an already-real COW extent right there
under ILOCK_SHARED rather than bouncing through
xfs_bmapi_convert_delalloc() and cancelling a transaction, which is
more or less what xfs_map_cow() did before the writeback rework
removed it. Does that line up with what you had in mind?

Anthony