Hi Ritesh,
On 2023/9/22 20:10, Ritesh Harjani (IBM) wrote:
> This test covers data & metadata integrity check with directio with
> o_sync flag and checks the file contents & size after sudden fileystem
> shutdown once the directio write is completed. ext4 directio after iomap
> conversion was broken in the sense that if the FS crashes after
> synchronous directio write, it's file size is not properly updated.
> This test adds a testcase to cover such scenario.
>
> Man page of open says that -
> O_SYNC provides synchronized I/O file integrity completion, meaning write
> operations will flush data and all associated metadata to the underlying
> hardware
>
> Reported-by: Gao Xiang <[email protected]>
> Signed-off-by: Ritesh Harjani (IBM) <[email protected]>
> ---
> tests/generic/471 | 45 +++++++++++++++++++++++++++++++++++++++++++
> tests/generic/471.out | 8 ++++++++
> 2 files changed, 53 insertions(+)
> create mode 100755 tests/generic/471
> create mode 100644 tests/generic/471.out
>
> diff --git a/tests/generic/471 b/tests/generic/471
> new file mode 100755
> index 00000000..6c31cff8
> --- /dev/null
> +++ b/tests/generic/471
> @@ -0,0 +1,45 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2023 IBM Corporation. All Rights Reserved.
> +#
> +# FS QA Test 471
> +#
> +# Integrity test with DIRECT_IO & O_SYNC with sudden shutdown
> +#
> +. ./common/preamble
> +_begin_fstest auto quick shutdown
> +
> +# Override the default cleanup function.
> +_cleanup()
> +{
> + cd /
> + rm -r -f $tmp.*
> +}
> +
> +# Import common functions.
> +. ./common/filter
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs generic
> +_require_scratch
> +_require_scratch_shutdown
> +
> +_scratch_mkfs > $seqres.full 2>&1
> +_scratch_mount
> +
> +echo "Create a 1M file using O_DIRECT & O_SYNC"
> +xfs_io -fsd -c "pwrite -S 0x5a 0 1M" $SCRATCH_MNT/testfile > /dev/null 2>&1
Thanks for the time on this.
I'm fine with this as it's the exact regression test to
my report.
Although the original issue from our guest real workload
is actually aio + O_SYNC, but that doesn't matter for
ext4 since it will serialize the whole process of DIO
write beyond i_size with inode lock.
Yet if my understanding is correct, some other fses (e.g.
XFS) seem to be more relaxed than this, see
xfs_file_dio_write_aligned() and xfs_file_write_checks(),
so I'm not sure if we need to cover AIO cases as well,
anyway.
Thanks,
Gao Xiang
> +
> +echo "Shutdown the fs suddenly"
> +_scratch_shutdown
> +
> +echo "Cycle mount"
> +_scratch_cycle_mount
> +
> +echo "File contents after cycle mount"
> +_hexdump $SCRATCH_MNT/testfile
> +
> +status=0
> +exit
> diff --git a/tests/generic/471.out b/tests/generic/471.out
> new file mode 100644
> index 00000000..ae279b79
> --- /dev/null
> +++ b/tests/generic/471.out
> @@ -0,0 +1,8 @@
> +QA output created by 471
> +Create a 1M file using O_DIRECT & O_SYNC
> +Shutdown the fs suddenly
> +Cycle mount
> +File contents after cycle mount
> +000000 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a >ZZZZZZZZZZZZZZZZ<
> +*
> +100000
Gao Xiang <[email protected]> writes:
> Hi Ritesh,
>
> On 2023/9/22 20:10, Ritesh Harjani (IBM) wrote:
>> This test covers data & metadata integrity check with directio with
>> o_sync flag and checks the file contents & size after sudden fileystem
>> shutdown once the directio write is completed. ext4 directio after iomap
>> conversion was broken in the sense that if the FS crashes after
>> synchronous directio write, it's file size is not properly updated.
>> This test adds a testcase to cover such scenario.
>>
>> Man page of open says that -
>> O_SYNC provides synchronized I/O file integrity completion, meaning write
>> operations will flush data and all associated metadata to the underlying
>> hardware
>>
>> Reported-by: Gao Xiang <[email protected]>
>> Signed-off-by: Ritesh Harjani (IBM) <[email protected]>
>> ---
>> tests/generic/471 | 45 +++++++++++++++++++++++++++++++++++++++++++
>> tests/generic/471.out | 8 ++++++++
>> 2 files changed, 53 insertions(+)
>> create mode 100755 tests/generic/471
>> create mode 100644 tests/generic/471.out
>>
>> diff --git a/tests/generic/471 b/tests/generic/471
>> new file mode 100755
>> index 00000000..6c31cff8
>> --- /dev/null
>> +++ b/tests/generic/471
>> @@ -0,0 +1,45 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2023 IBM Corporation. All Rights Reserved.
>> +#
>> +# FS QA Test 471
>> +#
>> +# Integrity test with DIRECT_IO & O_SYNC with sudden shutdown
>> +#
>> +. ./common/preamble
>> +_begin_fstest auto quick shutdown
>> +
>> +# Override the default cleanup function.
>> +_cleanup()
>> +{
>> + cd /
>> + rm -r -f $tmp.*
>> +}
>> +
>> +# Import common functions.
>> +. ./common/filter
>> +
>> +# real QA test starts here
>> +
>> +# Modify as appropriate.
>> +_supported_fs generic
>> +_require_scratch
>> +_require_scratch_shutdown
>> +
>> +_scratch_mkfs > $seqres.full 2>&1
>> +_scratch_mount
>> +
>> +echo "Create a 1M file using O_DIRECT & O_SYNC"
>> +xfs_io -fsd -c "pwrite -S 0x5a 0 1M" $SCRATCH_MNT/testfile > /dev/null 2>&1
>
> Thanks for the time on this.
>
> I'm fine with this as it's the exact regression test to
> my report.
>
> Although the original issue from our guest real workload
> is actually aio + O_SYNC, but that doesn't matter for
> ext4 since it will serialize the whole process of DIO
> write beyond i_size with inode lock.
Yes, even if we do AIO but since it is an extending write we will pass
IOMAP_DIO_FORCE_WAIT to iomap which means it will wait for the
completion anyway.
But on second thoughts, we can still add both synchronous direct-io
writes and buffered-io writes to this test. The man page of "open" tells
about O_SYNC flag, so the test should make sure that "data" and
"metadata" gets written to disk for both buffered-io and direct-io.
I will enhance that in second revision to cover buffered-io case as well.
>
> Yet if my understanding is correct, some other fses (e.g.
> XFS) seem to be more relaxed than this, see
> xfs_file_dio_write_aligned() and xfs_file_write_checks(),
> so I'm not sure if we need to cover AIO cases as well,
> anyway.
It's O_SYNC flag of open which mandates both data and metadata
integrity after "write" (or similar) completion.
So, I think it will be better to cover AIO case as well.
There are a bunch of AIO tests present. I will see if either of it can
be enhanced to do basic aiodio writes. If not, I will add a basic
integrity test.
Thanks
-ritesh
>
> Thanks,
> Gao Xiang
>
>> +
>> +echo "Shutdown the fs suddenly"
>> +_scratch_shutdown
>> +
>> +echo "Cycle mount"
>> +_scratch_cycle_mount
>> +
>> +echo "File contents after cycle mount"
>> +_hexdump $SCRATCH_MNT/testfile
>> +
>> +status=0
>> +exit
>> diff --git a/tests/generic/471.out b/tests/generic/471.out
>> new file mode 100644
>> index 00000000..ae279b79
>> --- /dev/null
>> +++ b/tests/generic/471.out
>> @@ -0,0 +1,8 @@
>> +QA output created by 471
>> +Create a 1M file using O_DIRECT & O_SYNC
>> +Shutdown the fs suddenly
>> +Cycle mount
>> +File contents after cycle mount
>> +000000 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a >ZZZZZZZZZZZZZZZZ<
>> +*
>> +100000