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

["Anthony Vardaro (Anthropic)" <me@anthonyvardaro.com>]

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