Zorro Lang <zlang@xxxxxxxxxx> writes:
> On Sat, Aug 02, 2025 at 01:59:18AM +0530, Ritesh Harjani wrote:
>> Zorro Lang <zlang@xxxxxxxxxx> 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@xxxxxxxxx>
>> >> ---
>> >> 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@xxxxxxxxxxxxxxxxx>
>> >> #
>> >>
>> >> @@ -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@xxxxxxxxxx>
>
Thanks!
-ritesh
> Thanks,
> Zorro
>
