Re: [RFC 2/2] generic: Add integrity tests for O_DSYNC and RWF_DSYNC writes

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

Zorro Lang <zlang@redhat.com> writes:

> On Sat, Aug 02, 2025 at 01:59:18AM +0530, Ritesh Harjani wrote:
>> Zorro Lang <zlang@redhat.com> writes:
>> 
>> > On Thu, Jul 31, 2025 at 06:05:55PM +0530, Ritesh Harjani (IBM) wrote:
>> >> This test verifies the data & required metadata (e.g. inode i_size for extending
>> >> writes) integrity when using O_DSYNC and RWF_DSYNC during writes operations,
>> >> across buffered-io, aio-dio and dio, in the event of a sudden filesystem
>> >> shutdown after write completion.
>> >> 
>> >> Man page of open says that -
>> >> O_DSYNC provides synchronized I/O data integrity completion, meaning
>> >> write operations will flush data to the underlying hardware, but will
>> >> only flush metadata updates that are required to allow a subsequent read
>> >> operation to complete successfully.
>> >> 
>> >> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> >> ---
>> >>  tests/generic/737     | 30 +++++++++++++++++++++++++++++-
>> >>  tests/generic/737.out | 21 +++++++++++++++++++++
>
> BTW, as you change a testcase with known case number, your subject can be
> "generic/737: ..." (instead of "generic: ...").
>

Make sense. I will fix in v2.

>> >>  2 files changed, 50 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/tests/generic/737 b/tests/generic/737
>> >> index 99ca1f39..0f27c82b 100755
>> >> --- a/tests/generic/737
>> >> +++ b/tests/generic/737
>> >> @@ -4,7 +4,8 @@
>> >>  #
>> >>  # FS QA Test No. 737
>> >>  #
>> >> -# Integrity test for O_SYNC with buff-io, dio, aio-dio with sudden shutdown.
>> >> +# Integrity test for O_[D]SYNC and/or RWF_DSYNC with buff-io, dio, aio-dio with
>> >> +# sudden shutdown.
>> >>  # Based on a testcase reported by Gao Xiang <hsiangkao@linux.alibaba.com>
>> >>  #
>> >> 
>> >> @@ -21,6 +22,15 @@ _require_aiodio aio-dio-write-verify
>> >>  _scratch_mkfs > $seqres.full 2>&1
>> >>  _scratch_mount
>> >> 
>> >> +echo "T-0: Create a 1M file using buff-io & RWF_DSYNC"
>> >> +$XFS_IO_PROG -f -c "pwrite -V 1 -D -S 0x5a 0 1M" $SCRATCH_MNT/testfile.t1 > /dev/null 2>&1
>> >> +echo "T-0: Shutdown the fs suddenly"
>> >> +_scratch_shutdown
>> >> +echo "T-0: Cycle mount"
>> >> +_scratch_cycle_mount
>> >> +echo "T-0: File contents after cycle mount"
>> >> +_hexdump $SCRATCH_MNT/testfile.t1
>> >> +
>> >>  echo "T-1: Create a 1M file using buff-io & O_SYNC"
>> >>  $XFS_IO_PROG -fs -c "pwrite -S 0x5a 0 1M" $SCRATCH_MNT/testfile.t1 > /dev/null 2>&1
>> >>  echo "T-1: Shutdown the fs suddenly"
>> >> @@ -48,5 +58,23 @@ _scratch_cycle_mount
>> >>  echo "T-3: File contents after cycle mount"
>> >>  _hexdump $SCRATCH_MNT/testfile.t3
>> >> 
>> >> +echo "T-4: Create a 1M file using DIO & RWF_DSYNC"
>> >> +$XFS_IO_PROG -fdc "pwrite -V 1 -S 0x5a -D 0 1M" $SCRATCH_MNT/testfile.t4 > /dev/null 2>&1
>> >> +echo "T-4: Shutdown the fs suddenly"
>> >> +_scratch_shutdown
>> >> +echo "T-4: Cycle mount"
>> >> +_scratch_cycle_mount
>> >> +echo "T-4: File contents after cycle mount"
>> >> +_hexdump $SCRATCH_MNT/testfile.t4
>> >> +
>> >> +echo "T-5: Create a 1M file using AIO-DIO & O_DSYNC"
>> >> +$AIO_TEST -a size=1048576 -D -N $SCRATCH_MNT/testfile.t5 > /dev/null 2>&1
>> >> +echo "T-5: Shutdown the fs suddenly"
>> >> +_scratch_shutdown
>> >> +echo "T-5: Cycle mount"
>> >> +_scratch_cycle_mount
>> >> +echo "T-5: File contents after cycle mount"
>> >> +_hexdump $SCRATCH_MNT/testfile.t5
>> >
>> > I always hit "No such file or directory" [1], is this an expected test failure
>> > which you hope to uncover?
>> 
>> Yes, we will need this fix [1] from Jan. Sorry I missed to add that in the commit
>> message. Could you please give it a try with the fix maybe?
>
> Sure, with this patch, this test passed on my side:
>   FSTYP         -- xfs (debug)
>   PLATFORM      -- Linux/x86_64 dell-per750-41 6.16.0-mainline+ #9 SMP PREEMPT_DYNAMIC Sat Aug  2 16:01:22 CST 2025
>   MKFS_OPTIONS  -- -f /dev/mapper/testvg-scratch--devA
>   MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/mapper/testvg-scratch--devA /mnt/scratch
>
>   generic/737 4s ...  9s
>   Ran: generic/737
>   Passed all 1 tests
>
> So this "No such file or directory" failure is a known bug, and this patch trys to
> uncover it. You didn't metion that, so I thought you just try to write a
> new integrity test (rather than a regression test) :-D

Your understanding is correct. Sorry, I missed to add the fix details.
I posted the test before the fix got merged. But yes, I will add the
necessary details in v2.

> If this change uncover a known fix, please feel free to add _fixed_by_kernel_commit.

The commit isn't yet showing up in the VFS tree. Once it gets in I will
share the v2 with the following changes.

diff --git a/tests/generic/737 b/tests/generic/737
index 0f27c82b..a51e9623 100755
--- a/tests/generic/737
+++ b/tests/generic/737
@@ -18,6 +18,9 @@ _require_aiodio aio-dio-write-verify

 [[ "$FSTYP" =~ ext[0-9]+ ]] && _fixed_by_kernel_commit 91562895f803 \
        "ext4: properly sync file size update after O_SYNC direct IO"
+# which is further fixed by
+_fixed_by_kernel_commit 16f206eebbf8 \
+       "iomap: Fix broken data integrity guarantees for O_SYNC writes"

 _scratch_mkfs > $seqres.full 2>&1
 _scratch_mount


> BTW, I saw you mark this patchset with "RFC", do you need to change it more?
> Generally I won't merge RFC patches directly, although this patch looks good to
> me. So when you think you've gotten enough review points, please feel free to
> remove the "RFC" label and resend it :) Then ...
>

Right. I will drop RFC and will resend it with above mentioned points
(once the fix appears in VFS tree).

> Reviewed-by: Zorro Lang <zlang@redhat.com>
>

Thanks!

-ritesh

> Thanks,
> Zorro
>