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

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: [email protected] # v5.1
> Assisted-by: Claude:unspecified
> Signed-off-by: Anthony Vardaro (Anthropic) <[email protected]>
> ---
> 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) <[email protected]>
> 
>