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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2026/8/6 00:25, Zorro Lang 写道:
> 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 <borntraeger@linux.ibm.com>
>>>> Link: https://lore.kernel.org/linux-btrfs/20260721191152.101118-1-borntraeger@linux.ibm.com/
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>
>>> 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.

Not sure if pkill is really needed, as the last wait will handle it well.


So in short, the existing

kill "$pid"
wait "$pid"
unset pid

is good enough, that's what I did in the v2.

Although the child processes still need to trap the corresponding 
signals, which is also done in v2.

Finally I have already tested the signal interruption for v2 several 
times, it all properly catch the signal and terminate the children 
processes and the main script without any delay.

So I think the one used in v2 is working properly.

Thanks,
Qu
> 
> 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
>>>>
>>>>
>>>
>>
>>
>