> On Jun 11, 2025, at 11:59 PM, Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx> wrote:
>
> On Wed, Jun 11, 2025 at 06:57:07PM -0700, Catherine Hoang wrote:
>> Simple tests of various atomic write requests and a (simulated) hardware
>> device.
>>
>> The first test performs basic multi-block atomic writes on a scsi_debug device
>> with atomic writes enabled. We test all advertised sizes between the atomic
>> write unit min and max. We also ensure that the write fails when expected, such
>> as when attempting buffered io or unaligned directio.
>>
>> The second test is similar to the one above, except that it verifies multi-block
>> atomic writes on actual hardware instead of simulated hardware. The device used
>> in this test is not required to support atomic writes.
>>
>> The final two tests ensure multi-block atomic writes can be performed on various
>> interweaved mappings, including written, mapped, hole, and unwritten. We also
>> test large atomic writes on a heavily fragmented filesystem. These tests are
>> separated into reflink (shared) and non-reflink tests.
>>
>> Signed-off-by: "Darrick J. Wong" <djwong@xxxxxxxxxx>
>> Signed-off-by: Catherine Hoang <catherine.hoang@xxxxxxxxxx>
>
> Hi Catherine, Darrick,
>
> The tests looks mostly okay. Just a few minor comments I've added below:
>
>> ---
>> common/atomicwrites | 10 ++++
>> tests/generic/1222 | 89 ++++++++++++++++++++++++++++
>> tests/generic/1222.out | 10 ++++
>> tests/generic/1223 | 67 +++++++++++++++++++++
>> tests/generic/1223.out | 9 +++
>> tests/generic/1224 | 86 +++++++++++++++++++++++++++
>> tests/generic/1224.out | 16 ++++++
>> tests/generic/1225 | 128 +++++++++++++++++++++++++++++++++++++++++
>> tests/generic/1225.out | 21 +++++++
>> 9 files changed, 436 insertions(+)
>> create mode 100755 tests/generic/1222
>> create mode 100644 tests/generic/1222.out
>> create mode 100755 tests/generic/1223
>> create mode 100644 tests/generic/1223.out
>> create mode 100755 tests/generic/1224
>> create mode 100644 tests/generic/1224.out
>> create mode 100755 tests/generic/1225
>> create mode 100644 tests/generic/1225.out
>>
>> diff --git a/common/atomicwrites b/common/atomicwrites
>> index 88f49a1a..4ba945ec 100644
>> --- a/common/atomicwrites
>> +++ b/common/atomicwrites
>> @@ -136,3 +136,13 @@ _test_atomic_file_writes()
>> $XFS_IO_PROG -dc "pwrite -A -D -V1 -b $bsize 1 $bsize" $testfile 2>> $seqres.full && \
>> echo "atomic write requires offset to be aligned to bsize"
>> }
>> +
>> +_simple_atomic_write() {
>> + local pos=$1
>> + local count=$2
>> + local file=$3
>> + local directio=$4
>> +
>> + echo "testing pos=$pos count=$count file=$file directio=$directio" >> $seqres.full
>> + $XFS_IO_PROG $directio -c "pwrite -b $count -V 1 -A -D $pos $count" $file >> $seqres.full
>> +}
>> diff --git a/tests/generic/1222 b/tests/generic/1222
>> new file mode 100755
>> index 00000000..d3665d0b
>> --- /dev/null
>> +++ b/tests/generic/1222
>> @@ -0,0 +1,89 @@
>
> <snip>
>
>> +
>> +$XFS_IO_PROG -f -c "falloc 0 $((max_awu * 2))" -c fsync $testfile
>> +
>> +# try outside the advertised sizes
>> +echo "two EINVAL for unsupported sizes"
>> +min_i=$((min_awu / 2))
>> +_simple_atomic_write $min_i $min_i $testfile -d
>> +max_i=$((max_awu * 2))
>> +_simple_atomic_write $max_i $max_i $testfile -d
>> +
>> +# try all of the advertised sizes
>> +echo "all should work"
>> +for ((i = min_awu; i <= max_awu; i *= 2)); do
>> + $XFS_IO_PROG -f -c "falloc 0 $((max_awu * 2))" -c fsync $testfile
>> + _test_atomic_file_writes $i $testfile
>> + _simple_atomic_write $i $i $testfile -d
>
> I'm still not sure what extra thing this _simple_atomic_write is testing
> here that is not already tested via _test_atomic_file_writes? (same
> question for g/1223 as well)
I think you’re right, the only difference I see is that _simple_atomic_write
accepts a pos argument while _test_atomic_file_writes doesn’t. But
since _test_atomic_file_writes already does a broader range of tests I
don’t think _simple_atomic_write is necessary, so I can remove that.
>
>> +done
>> +
>> +# does not support buffered io
>> +echo "one EOPNOTSUPP for buffered atomic"
>> +_simple_atomic_write 0 $min_awu $testfile
>> +
>> +# does not support unaligned directio
>> +echo "one EINVAL for unaligned directio"
>> +_simple_atomic_write $sector_size $min_awu $testfile -d
>> +
>
> <snip>
>
>> new file mode 100755
>> index 00000000..f2dea804
>> --- /dev/null
>> +++ b/tests/generic/1225
>> @@ -0,0 +1,128 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2025 Oracle. All Rights Reserved.
>> +#
>> +# FS QA Test 1225
>> +#
>> +# basic tests for large atomic writes with mixed mappings
>> +#
>> +. ./common/preamble
>> +_begin_fstest auto quick rw atomicwrites
>> +
>> +. ./common/atomicwrites
>> +. ./common/filter
>> +. ./common/reflink
>
> I think we are not using reflink based helpers here so this can be
> dropped.
>
>
> <snip>
>
>> +
>> +echo "atomic write unwritten+mapped+unwritten"
>> +dd if=/dev/zero of=$file1 bs=1M count=10 conv=fsync >>$seqres.full 2>&1
>> +$XFS_IO_PROG -fc "falloc 0 4096000" $file1 >>$seqres.full 2>&1
>> +$XFS_IO_PROG -dc "pwrite -D -V1 4096 4096" $file1 >>$seqres.full 2>&1
>> +$XFS_IO_PROG -dc "pwrite -A -D -V1 0 65536" $file1 >>$seqres.full 2>&1
>> +md5sum $file1 | _filter_scratch
>> +
>> +echo "atomic write adjacent mapped+unwritten and unwritten+mapped"
>> +dd if=/dev/zero of=$file1 bs=1M count=10 conv=fsync >>$seqres.full 2>&1
>> +$XFS_IO_PROG -fc "falloc 0 4096000" $file1 >>$seqres.full 2>&1
>> +$XFS_IO_PROG -dc "pwrite -D -V1 0 4096" $file1 >>$seqres.full 2>&1
>> +$XFS_IO_PROG -dc "pwrite -D -V1 61440 4096" $file1 >>$seqres.full 2>&1
>> +$XFS_IO_PROG -dc "pwrite -A -D -V1 0 32768" $file1 >>$seqres.full 2>&1
>> +$XFS_IO_PROG -dc "pwrite -A -D -V1 32768 32768" $file1 >>$seqres.full 2>&1
>> +md5sum $file1 | _filter_scratch
>> +
>> +echo "atomic write mapped+unwritten+mapped"
>> +dd if=/dev/zero of=$file1 bs=1M count=10 conv=fsync >>$seqres.full 2>&1
>> +$XFS_IO_PROG -fc "falloc 0 4096000" $file1 >>$seqres.full 2>&1
>> +$XFS_IO_PROG -dc "pwrite -D -V1 0 4096" $file1 >>$seqres.full 2>&1
>> +$XFS_IO_PROG -dc "pwrite -D -V1 61440 4096" $file1 >>$seqres.full 2>&1
>> +$XFS_IO_PROG -dc "pwrite -A -D -V1 0 65536" $file1 >>$seqres.full 2>&1
>> +md5sum $file1 | _filter_scratch
>> +
>> +echo "atomic write interweaved hole+unwritten+written"
>> +dd if=/dev/zero of=$file1 bs=1M count=10 conv=fsync >>$seqres.full 2>&1
>> +blksz=4096
>> +nr=32
>> +_weave_file_rainbow $blksz $nr $file1 >>$seqres.full 2>&1
>> +$XFS_IO_PROG -dc "pwrite -A -D -V1 0 65536" $file1 >>$seqres.full 2>&1
>> +md5sum $file1 | _filter_scratch
>
> Thanks for adding more unwritten based combinations :)
>
> Rest everything looks okay to me.
>
> Regards,
> ojaswin
>
> <snip>
