Re: [PATCH v2] xfs: exercise a failed CoW conversion during writeback

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Sep 28, 2026 at 09:53:29PM +0800, Zorro Lang wrote:
> On Mon, Sep 28, 2026 at 10:50:46AM +0200, Andrea Parri wrote:
> > iomap_add_to_ioend() submits the pending ioend through
> > ->writeback_submit() before allocating a new one for the current
> > range.  For XFS, xfs_writeback_submit() converts the ioend's CoW
> > extents via xfs_reflink_convert_cow() before submitting the bio; if
> > that conversion fails, ->writeback_submit() completes the ioend with
> > an error and returns it.  A kernel bug left the stale ioend behind in
> > wpc->wb_ctx, which iomap_writepages() then submitted a second time,
> > corrupting XFS's ip->i_ioend_list.
> > 
> > Exercise this path with the wb_cow_convert_error error tag: reflink a
> > file, dirty several widely separated ranges of the shared extent so
> > that a single writepages() call has to build and submit more than one
> > ioend, inject the error, and let writeback run.  On a kernel without
> > the fix this reliably hits a "list_add double add" WARN from
> > xfs_end_bio(); on a fixed kernel writeback just fails cleanly.
> > 
> > This requires the wb_cow_convert_error XFS error tag; the test cleanly
> > not-runs on kernels without it.
> > 
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
> > ---
> > The kernel fix and the wb_cow_convert_error error tag this test relies
> > on are part of the series at:
> > 
> >   https://lore.kernel.org/all/cover.1790342457.git.parri.andrea@gmail.com/
> > 
> > Changes since v1:
> >  - Cite the kernel fix with _fixed_by_kernel_commit (Darrick).
> >  - Don't reset the error tag before the remount; the unmount clears it
> >    anyway (Darrick).
> >  - Picked up Christoph's Reviewed-by.
> > 
> > v1: https://lore.kernel.org/all/20260924091338.198407-1-parri.andrea@gmail.com/
> 
> Overall the patch looks good to me, but I have one minor comment below ...
> 
> > 
> >  tests/xfs/842     | 71 +++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/xfs/842.out |  7 +++++
> >  2 files changed, 78 insertions(+)
> >  create mode 100755 tests/xfs/842
> >  create mode 100644 tests/xfs/842.out
> > 
> > diff --git a/tests/xfs/842 b/tests/xfs/842
> > new file mode 100755
> > index 0000000000000..4495e9bdfa530
> > --- /dev/null
> > +++ b/tests/xfs/842
> > @@ -0,0 +1,71 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2026 Andrea Parri.  All Rights Reserved.
> > +#
> > +# FS QA Test No. 842
> > +#
> > +# Regression test for a failed ->writeback_submit() call leaving a stale
> > +# wpc->wb_ctx behind.  iomap_writepages() then resubmits whatever
> > +# wpc->wb_ctx points to, i.e. the ioend that ->writeback_submit() already
> > +# completed with an error.  For XFS the second bio_endio() lands back in
> > +# xfs_end_bio(), which list_add_tail()s the already-linked ioend into
> > +# ip->i_ioend_list a second time, corrupting the list.
> > +#
> > +# The only in-tree way for XFS's ->writeback_submit() to fail is a
> > +# failing xfs_reflink_convert_cow(), so this uses the
> > +# wb_cow_convert_error error tag to force that on a reflinked file whose
> > +# CoW extents are dirtied in several widely separated ranges, so that a
> > +# single writepages() call has to submit more than one ioend.
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick clone
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +. ./common/reflink
> > +. ./common/inject
> > +
> > +_fixed_by_kernel_commit XXXXXXXXXXXX \
> > +	"iomap: don't resubmit an ioend after ->writeback_submit() failed"
> > +
> > +_require_cp_reflink
> > +_require_scratch_reflink
> > +_require_xfs_io_error_injection "wb_cow_convert_error"
> > +_require_kernel_config CONFIG_LIST_HARDENED
> > +
> > +blksz=65536
> > +nr=8
> > +sz=$((blksz * nr * 2))
> > +
> > +echo "Format and mount"
> > +_scratch_mkfs >> $seqres.full 2>&1
> > +_scratch_mount
> > +
> > +echo "Create reflinked file with several CoW extents"
> > +_pwrite_byte 0x58 0 $sz $SCRATCH_MNT/file1 >> $seqres.full
> > +_scratch_sync
> > +_cp_reflink $SCRATCH_MNT/file1 $SCRATCH_MNT/file2
> > +
> > +# Dirty several widely separated ranges of file2's CoW extents so that a
> > +# single writepages() call has to submit more than one ioend.
> > +seq=0
> 
> In fstests, $seq is a core global variable representing the test sequence
> number. Overwriting it here might lead to unexpected side effects with
> framework helpers or cleanup logic. Could we rename this loop variable to
> something else to avoid trouble? If you need, I can help that when I merge
> it.
> 
> Thanks,
> Zorro
> 
> > +while [ $seq -lt $nr ]; do
> > +	off=$((seq * blksz * 2))
> > +	_pwrite_byte 0x59 $off $blksz $SCRATCH_MNT/file2 >> $seqres.full
> > +	seq=$((seq + 1))
> > +done

for ((off = 0; off < (blksz * nr * 2); off += (blksz * 2))); do
	_pwrite_byte 0x59 $off $blksz $SCRATCH_MNT/file2 >> $seqres.full
done

--D

> > +
> > +echo "Inject wb_cow_convert_error"
> > +_scratch_inject_error "wb_cow_convert_error"
> > +
> > +echo "Trigger writeback with CoW conversion forced to fail"
> > +_scratch_sync
> > +
> > +echo "Remount"
> > +_scratch_cycle_mount
> > +
> > +echo "Silence is golden"
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/xfs/842.out b/tests/xfs/842.out
> > new file mode 100644
> > index 0000000000000..72c9c67e7d5fb
> > --- /dev/null
> > +++ b/tests/xfs/842.out
> > @@ -0,0 +1,7 @@
> > +QA output created by 842
> > +Format and mount
> > +Create reflinked file with several CoW extents
> > +Inject wb_cow_convert_error
> > +Trigger writeback with CoW conversion forced to fail
> > +Remount
> > +Silence is golden
> > -- 
> > 2.53.0
> > 
>