Re: [PATCH] fstests: add a dio-read-into-mmap and sync race test case

On Tue, Aug 04, 2026 at 09:11:24AM +0930, Qu Wenruo wrote:
> 
> 
> 在 2026/8/3 20:05, Zorro Lang 写道:
> > On Sat, Jul 25, 2026 at 08:37:24PM +0930, Qu Wenruo wrote:
> > > There is a report that on btrfs, if the following workload are running,
> > > btrfs can fail:
> > > 
> > > - A dio read into a mmaped range
> > >    Only the mmap range needs to be on btrfs.
> > >    The dio read source makes no difference.
> > > 
> > > - Sync_range on the mapped range
> > > 
> > > The btrfs errors include:
> > > 
> > > - Hang during data writeback
> > > - Filesystem flips RO
> > > 
> > > The mmap range is dirtied but written back by the sync_range process,
> > > then dio read finished and found that the folios are no longer dirty,
> > > so dio endio will mark those folios dirty again so that the fs can write
> > > them back again.
> > > 
> > > However for non-experimental btrfs with 4K block size and 4K page size,
> > > there is a regression in v7.2 that such case is no longer handled
> > > properly, due to the enablement of large folios and removal of cow
> > > fixup.
> > > And btrfs can never handle it for bs < ps from day 1.
> > > 
> > > Add a regression test for it.
> > > 
> > > Reported-by: Christian Borntraeger <[email protected]>
> > > Link: https://lore.kernel.org/linux-btrfs/[email protected]/
> > > Signed-off-by: Qu Wenruo <[email protected]>
> > > ---
> > 
> > Hi Wenruo,
> > 
> > Thanks for this new test!
> 
> Thanks for the review. Will update the patch to address all the comments and
> refresh the test number.
> 
> But one comment inlined below.
> 
> > > +
> > > +kill "$read_pid" "$sync_pid" &> /dev/null
> > 
> > How about:
> >    kill -TERM -"$read_pid" -"$sync_pid" &> /dev/null
> > ?
> > 
> > As the (man 1 kill) says:
> > 
> >    pid
> >      Each pid can be expressed in one of the following ways:
> >      ...
> >      -n
> >         where n is larger than 1. All processes in process group n are signaled.
> >         When an argument of the form '-n' is given, and it is meant to denote a
> >         process group, either a signal must be specified first, or the argument
> >         must be preceded by a '--' option, otherwise it will be taken as the
> >         signal to send.
> > 
> > I didn't give it a try, but I think this might help to kill the
> > dio-read-into-mmap and sync_range process too.
> 
> Unfortunately this doesn't seems to work.
> 
> If I removed the redirection, it shows the following error:
> 
> /home/adam/xfstests/tests/generic/801: line 76: kill: (-1271) - No such
> process
> /home/adam/xfstests/tests/generic/801: line 76: kill: (-1272) - No such
> process
> 
> Thus it will not really kill the children processes.
> 
> Despite this change, all other comments will be addressed.

Hi Wenruo,

Sorry for the confusion! I assumed that approach would work, but from the
error message, it seems running "read_workload &" doesn't create a proper
Process Group (PGID).

How about this approach instead?

1) kill -TERM "$read_pid" 2>/dev/null

Based on the trap in read_workload(), it should enter "wait" upon receiving
SIGTERM, preventing any new iterations of the loop. Then we can run:

2) pkill -P "$read_pid" 2>/dev/null

to clean up any remaining child processes, followed by a wait for the main
process to exit.

3) wait "$read_pid"
   unset read_pid

(We can wrap this logic in a function and invoke it inside _cleanup)

I hope this can prevent the script from waiting long time, and no orphaned
processes are left behind. Feel free to tell me if anyone has better idea :)

Thanks,
Zorro

> 
> Thanks,
> Qu
> 
> > 
> > (same in _cleanup)
> > 
> > > +unset "$read_pid" "$sync_pid"
> > 
> > unset read_pid sync_pid
> > 
> > > +wait
> > > +
> > > +echo "Silence is golden"
> > > +_exit 0
> > > diff --git a/tests/generic/799.out b/tests/generic/799.out
> > > new file mode 100644
> > > index 00000000..f3fd9fa2
> > > --- /dev/null
> > > +++ b/tests/generic/799.out
> > > @@ -0,0 +1,2 @@
> > > +QA output created by 799
> > > +Silence is golden
> > > -- 
> > > 2.51.2
> > > 
> > > 
> > 
> 
>