[PATCH] generic: Add integrity tests with synchronous directio

[PATCH] generic: Add integrity tests with synchronous directio

[Ritesh Harjani (IBM)]

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 <hsiangkao@linux.alibaba.com>
Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 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
+
+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
-- 
2.41.0

Re: [PATCH] generic: Add integrity tests with synchronous directio

[Gao Xiang]

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 <hsiangkao@linux.alibaba.com>
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> ---
>   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

Re: [PATCH] generic: Add integrity tests with synchronous directio

[Darrick J. Wong]

On Fri, Sep 22, 2023 at 05:40:36PM +0530, 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 <hsiangkao@linux.alibaba.com>
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> ---
>  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

$XFS_IO_PROG ?

Otherwise looks good to me...

--D

> +
> +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
> -- 
> 2.41.0
> 

Re: [PATCH] generic: Add integrity tests with synchronous directio

[Ritesh Harjani]

Gao Xiang <hsiangkao@linux.alibaba.com> 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 <hsiangkao@linux.alibaba.com>
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> ---
>>   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

Re: [PATCH] generic: Add integrity tests with synchronous directio

[Ritesh Harjani]

"Darrick J. Wong" <djwong@kernel.org> writes:

> On Fri, Sep 22, 2023 at 05:40:36PM +0530, 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 <hsiangkao@linux.alibaba.com>
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> ---
>>  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
>
> $XFS_IO_PROG ?

Thanks for pointing out. Will fix it in next rev.

>
> Otherwise looks good to me...

Thanks. I am planning to enhance this integrity test to also cover
synchronous dio (already added), buff-io and aiodio writes as well
in the next revision.

-ritesh

Re: [PATCH] generic: Add integrity tests with synchronous directio

[Zorro Lang]

On Fri, Sep 22, 2023 at 05:40:36PM +0530, 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.

Thanks for this patch, some review points as below.

Is there a bug ? If there is, please use _fixed_by_kernel_commit to point
out that.

> 
> 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 <hsiangkao@linux.alibaba.com>
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> ---
>  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.*
> +}

This _cleanup looks same ith the default one, so you don't need to do
this "override", just remove this _cleanup and use the default one.

> +
> +# Import common functions.
> +. ./common/filter

If you don't need any filter helpers, feel free to remove this line.

> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
     ^^^
If you'll send a v2, feel free to remove this comment line :)

> +_supported_fs generic
> +_require_scratch
> +_require_scratch_shutdown

_require_odirect ??

Or if you will add aio test in v2, please use _require_aiodio.
Also add "aio" test group (in the _begin_fstest line).

> +
> +_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

$XFS_IO_PROG

Thanks,
Zorro

> +
> +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
> -- 
> 2.41.0
> 

Re: [PATCH] generic: Add integrity tests with synchronous directio

[Ritesh Harjani]

Zorro Lang <zlang@redhat.com> writes:

> On Fri, Sep 22, 2023 at 05:40:36PM +0530, 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.
>
> Thanks for this patch, some review points as below.
>
> Is there a bug ? If there is, please use _fixed_by_kernel_commit to point
> out that.
>

It's still under discussion. So I am fine if you would like to wait
until the official fix is ready.

>> 
>> 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 <hsiangkao@linux.alibaba.com>
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> ---
>>  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.*
>> +}
>
> This _cleanup looks same ith the default one, so you don't need to do
> this "override", just remove this _cleanup and use the default one.
>

Ok, IIUC, we don't need to define _cleanup function, since
". ./common/preamble" does it for us.

>> +
>> +# Import common functions.
>> +. ./common/filter
>
> If you don't need any filter helpers, feel free to remove this line.
>

will remove it.

>> +
>> +# real QA test starts here
>> +
>> +# Modify as appropriate.
>      ^^^
> If you'll send a v2, feel free to remove this comment line :)
>

Will remove.


>> +_supported_fs generic
>> +_require_scratch
>> +_require_scratch_shutdown
>
> _require_odirect ??
>
> Or if you will add aio test in v2, please use _require_aiodio.
> Also add "aio" test group (in the _begin_fstest line).
>

Sure. Thanks for pointing out.

>> +
>> +_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
>
> $XFS_IO_PROG

done.
>
> Thanks,
> Zorro
>

Thanks for the review!
-ritesh

[PATCHv2 1/2] aio-dio-write-verify: Add sync and noverify option

[Ritesh Harjani (IBM)]

This patch adds -S for O_SYNC and -N for noverify option to
aio-dio-write-verify test. We will use this for integrity
verification test for aio-dio.

Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 src/aio-dio-regress/aio-dio-write-verify.c | 29 ++++++++++++++++------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/src/aio-dio-regress/aio-dio-write-verify.c b/src/aio-dio-regress/aio-dio-write-verify.c
index 302b8fe4..61519f6e 100644
--- a/src/aio-dio-regress/aio-dio-write-verify.c
+++ b/src/aio-dio-regress/aio-dio-write-verify.c
@@ -34,13 +34,16 @@

 void usage(char *progname)
 {
-	fprintf(stderr, "usage: %s [-t truncsize ] <-a size=N,off=M [-a ...]>  filename\n"
+	fprintf(stderr, "usage: %s [-t truncsize ] <-a size=N,off=M [-a ...]>  [-S] [-N] filename\n"
 	        "\t-t truncsize: truncate the file to a special size before AIO wirte\n"
 	        "\t-a: specify once AIO write size and startoff, this option can be specified many times, but less than 128\n"
 	        "\t\tsize=N: AIO write size\n"
 	        "\t\toff=M:  AIO write startoff\n"
-	        "e.g: %s -t 4608 -a size=4096,off=512 -a size=4096,off=4608 filename\n",
-	        progname, progname);
+			"\t-S: uses O_SYNC flag for open. By default O_SYNC is not used\n"
+			"\t-N: no_verify: means no write verification. By default noverify is false\n"
+	        "e.g: %s -t 4608 -a size=4096,off=512 -a size=4096,off=4608 filename\n"
+	        "e.g: %s -t 1048576 -a size=1048576 -S -N filename\n",
+	        progname, progname, progname);
 	exit(1);
 }

@@ -281,8 +284,10 @@ int main(int argc, char *argv[])
 	char *filename = NULL;
 	int num_events = 0;
 	off_t tsize = 0;
+	int o_sync = 0;
+	int no_verify = 0;

-	while ((c = getopt(argc, argv, "a:t:")) != -1) {
+	while ((c = getopt(argc, argv, "a:t:SN")) != -1) {
 		char *endp;

 		switch (c) {
@@ -297,6 +302,12 @@ int main(int argc, char *argv[])
 		case 't':
 			tsize = strtoul(optarg, &endp, 0);
 			break;
+		case 'S':
+			o_sync = O_SYNC;
+			break;
+		case 'N':
+			no_verify = 1;
+			break;
 		default:
 			usage(argv[0]);
 		}
@@ -313,7 +324,7 @@ int main(int argc, char *argv[])
 	else
 		usage(argv[0]);

-	fd = open(filename, O_DIRECT | O_CREAT | O_TRUNC | O_RDWR, 0600);
+	fd = open(filename, O_DIRECT | O_CREAT | O_TRUNC | O_RDWR | o_sync, 0600);
 	if (fd == -1) {
 		perror("open");
 		return 1;
@@ -331,9 +342,11 @@ int main(int argc, char *argv[])
 		return 1;
 	}

-	if (io_verify(fd) != 0) {
-		fprintf(stderr, "Data verification fails\n");
-		return 1;
+	if (no_verify == 0) {
+		if (io_verify(fd) != 0) {
+			fprintf(stderr, "Data verification fails\n");
+			return 1;
+		}
 	}

 	close(fd);
--
2.41.0

[PATCHv2 2/2] generic: Add integrity tests with synchronous directio

[Ritesh Harjani (IBM)]

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 <hsiangkao@linux.alibaba.com>
Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 tests/generic/471     | 50 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/471.out | 22 +++++++++++++++++++
 2 files changed, 72 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..218e6676
--- /dev/null
+++ b/tests/generic/471
@@ -0,0 +1,50 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2023 IBM Corporation.  All Rights Reserved.
+#
+# FS QA Test 471
+#
+# Integrity test for O_SYNC with buff-io, dio, aio-dio with sudden shutdown
+#
+. ./common/preamble
+_begin_fstest auto quick shutdown aio
+
+# real QA test starts here
+_supported_fs generic
+_require_scratch
+_require_scratch_shutdown
+_require_odirect
+_require_aiodio aio-dio-write-verify
+
+_scratch_mkfs > $seqres.full 2>&1
+_scratch_mount
+
+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"
+_scratch_shutdown
+echo "T-1: Cycle mount"
+_scratch_cycle_mount
+echo "T-1: File contents after cycle mount"
+_hexdump $SCRATCH_MNT/testfile.t1
+
+echo "T-2: Create a 1M file using O_DIRECT & O_SYNC"
+$XFS_IO_PROG -fsd -c "pwrite -S 0x5a 0 1M" $SCRATCH_MNT/testfile.t2 > /dev/null 2>&1
+echo "T-2: Shutdown the fs suddenly"
+_scratch_shutdown
+echo "T-2: Cycle mount"
+_scratch_cycle_mount
+echo "T-2: File contents after cycle mount"
+_hexdump $SCRATCH_MNT/testfile.t2
+
+echo "T-3: Create a 1M file using AIO-DIO & O_SYNC"
+$AIO_TEST -a size=1048576 -S -N $SCRATCH_MNT/testfile.t3 > /dev/null 2>&1
+echo "T-3: Shutdown the fs suddenly"
+_scratch_shutdown
+echo "T-3: Cycle mount"
+_scratch_cycle_mount
+echo "T-3: File contents after cycle mount"
+_hexdump $SCRATCH_MNT/testfile.t3
+
+status=0
+exit
diff --git a/tests/generic/471.out b/tests/generic/471.out
new file mode 100644
index 00000000..2bfb033d
--- /dev/null
+++ b/tests/generic/471.out
@@ -0,0 +1,22 @@
+QA output created by 471
+T-1: Create a 1M file using buff-io & O_SYNC
+T-1: Shutdown the fs suddenly
+T-1: Cycle mount
+T-1: File contents after cycle mount
+000000 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a  >ZZZZZZZZZZZZZZZZ<
+*
+100000
+T-2: Create a 1M file using O_DIRECT & O_SYNC
+T-2: Shutdown the fs suddenly
+T-2: Cycle mount
+T-2: File contents after cycle mount
+000000 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a  >ZZZZZZZZZZZZZZZZ<
+*
+100000
+T-3: Create a 1M file using AIO-DIO & O_SYNC
+T-3: Shutdown the fs suddenly
+T-3: Cycle mount
+T-3: File contents after cycle mount
+000000 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a  >ZZZZZZZZZZZZZZZZ<
+*
+100000
-- 
2.41.0

Re: [PATCHv2 2/2] generic: Add integrity tests with synchronous directio

[Zorro Lang]

On Sat, Sep 23, 2023 at 05:30:24PM +0530, 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 <hsiangkao@linux.alibaba.com>
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> ---
>  tests/generic/471     | 50 +++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/471.out | 22 +++++++++++++++++++
>  2 files changed, 72 insertions(+)
>  create mode 100755 tests/generic/471
>  create mode 100644 tests/generic/471.out
> 
> diff --git a/tests/generic/471 b/tests/generic/471

The generic/471 has been taken last week, you can choose another number.
Or simply use generic/999, then I'll change the 999 to a proper number.

> new file mode 100755
> index 00000000..218e6676
> --- /dev/null
> +++ b/tests/generic/471
> @@ -0,0 +1,50 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2023 IBM Corporation.  All Rights Reserved.
> +#
> +# FS QA Test 471
> +#
> +# Integrity test for O_SYNC with buff-io, dio, aio-dio with sudden shutdown
> +#
> +. ./common/preamble
> +_begin_fstest auto quick shutdown aio
> +
> +# real QA test starts here
> +_supported_fs generic

Is the bug fix be reviewed and acked now? If it is, please use _fixed_by_kernel_commit
at here. The commit id can be "xxxxxxxxxxxx" if it's not merged by acked.

> +_require_scratch
> +_require_scratch_shutdown
> +_require_odirect

Due to you add aio test in v2, so this line should be: _require_aiodio

> +_require_aiodio aio-dio-write-verify
> +
> +_scratch_mkfs > $seqres.full 2>&1
> +_scratch_mount
> +
> +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"
> +_scratch_shutdown
> +echo "T-1: Cycle mount"
> +_scratch_cycle_mount
> +echo "T-1: File contents after cycle mount"
> +_hexdump $SCRATCH_MNT/testfile.t1
> +
> +echo "T-2: Create a 1M file using O_DIRECT & O_SYNC"
> +$XFS_IO_PROG -fsd -c "pwrite -S 0x5a 0 1M" $SCRATCH_MNT/testfile.t2 > /dev/null 2>&1
> +echo "T-2: Shutdown the fs suddenly"
> +_scratch_shutdown
> +echo "T-2: Cycle mount"
> +_scratch_cycle_mount
> +echo "T-2: File contents after cycle mount"
> +_hexdump $SCRATCH_MNT/testfile.t2
> +
> +echo "T-3: Create a 1M file using AIO-DIO & O_SYNC"
> +$AIO_TEST -a size=1048576 -S -N $SCRATCH_MNT/testfile.t3 > /dev/null 2>&1

So you just need aio-dio-write-verify.c to do aio write. Maybe we can have aio
read and write support in xfs_io in one day:)

Thanks,
Zorro


> +echo "T-3: Shutdown the fs suddenly"
> +_scratch_shutdown
> +echo "T-3: Cycle mount"
> +_scratch_cycle_mount
> +echo "T-3: File contents after cycle mount"
> +_hexdump $SCRATCH_MNT/testfile.t3
> +
> +status=0
> +exit
> diff --git a/tests/generic/471.out b/tests/generic/471.out
> new file mode 100644
> index 00000000..2bfb033d
> --- /dev/null
> +++ b/tests/generic/471.out
> @@ -0,0 +1,22 @@
> +QA output created by 471
> +T-1: Create a 1M file using buff-io & O_SYNC
> +T-1: Shutdown the fs suddenly
> +T-1: Cycle mount
> +T-1: File contents after cycle mount
> +000000 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a  >ZZZZZZZZZZZZZZZZ<
> +*
> +100000
> +T-2: Create a 1M file using O_DIRECT & O_SYNC
> +T-2: Shutdown the fs suddenly
> +T-2: Cycle mount
> +T-2: File contents after cycle mount
> +000000 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a  >ZZZZZZZZZZZZZZZZ<
> +*
> +100000
> +T-3: Create a 1M file using AIO-DIO & O_SYNC
> +T-3: Shutdown the fs suddenly
> +T-3: Cycle mount
> +T-3: File contents after cycle mount
> +000000 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a  >ZZZZZZZZZZZZZZZZ<
> +*
> +100000
> -- 
> 2.41.0
> 

Re: [PATCHv2 2/2] generic: Add integrity tests with synchronous directio

[Ritesh Harjani]

Zorro Lang <zlang@redhat.com> writes:

> On Sat, Sep 23, 2023 at 05:30:24PM +0530, 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 <hsiangkao@linux.alibaba.com>
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> ---
>>  tests/generic/471     | 50 +++++++++++++++++++++++++++++++++++++++++++
>>  tests/generic/471.out | 22 +++++++++++++++++++
>>  2 files changed, 72 insertions(+)
>>  create mode 100755 tests/generic/471
>>  create mode 100644 tests/generic/471.out
>> 
>> diff --git a/tests/generic/471 b/tests/generic/471
>
> The generic/471 has been taken last week, you can choose another number.
> Or simply use generic/999, then I'll change the 999 to a proper number.
>
>> new file mode 100755
>> index 00000000..218e6676
>> --- /dev/null
>> +++ b/tests/generic/471
>> @@ -0,0 +1,50 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2023 IBM Corporation.  All Rights Reserved.
>> +#
>> +# FS QA Test 471
>> +#
>> +# Integrity test for O_SYNC with buff-io, dio, aio-dio with sudden shutdown
>> +#
>> +. ./common/preamble
>> +_begin_fstest auto quick shutdown aio
>> +
>> +# real QA test starts here
>> +_supported_fs generic
>
> Is the bug fix be reviewed and acked now? If it is, please use _fixed_by_kernel_commit
> at here. The commit id can be "xxxxxxxxxxxx" if it's not merged by acked.
>

Hi Zorro,

Yes, the patch is still being worked on. I think we can wait till then
to not spook the distro CI testing to start reporting multiple bug
reports using this testcase :P 


>> +_require_scratch
>> +_require_scratch_shutdown
>> +_require_odirect
>
> Due to you add aio test in v2, so this line should be: _require_aiodio
>

Ok, I guess I can just remove this line "_require_odirect". Because in the
next line I anyway include _require_aiodio.

>> +_require_aiodio aio-dio-write-verify

here ^^

>> +
>> +_scratch_mkfs > $seqres.full 2>&1
>> +_scratch_mount
>> +
>> +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"
>> +_scratch_shutdown
>> +echo "T-1: Cycle mount"
>> +_scratch_cycle_mount
>> +echo "T-1: File contents after cycle mount"
>> +_hexdump $SCRATCH_MNT/testfile.t1
>> +
>> +echo "T-2: Create a 1M file using O_DIRECT & O_SYNC"
>> +$XFS_IO_PROG -fsd -c "pwrite -S 0x5a 0 1M" $SCRATCH_MNT/testfile.t2 > /dev/null 2>&1
>> +echo "T-2: Shutdown the fs suddenly"
>> +_scratch_shutdown
>> +echo "T-2: Cycle mount"
>> +_scratch_cycle_mount
>> +echo "T-2: File contents after cycle mount"
>> +_hexdump $SCRATCH_MNT/testfile.t2
>> +
>> +echo "T-3: Create a 1M file using AIO-DIO & O_SYNC"
>> +$AIO_TEST -a size=1048576 -S -N $SCRATCH_MNT/testfile.t3 > /dev/null 2>&1
>
> So you just need aio-dio-write-verify.c to do aio write. Maybe we can have aio
> read and write support in xfs_io in one day:)

Yes, someday maybe :)
But I am not sure what plan Darrick has about it. Was there any
history behind no aio-dio support in xfs_io? Just curious since the
discussion came up.


Thanks for the review!
-ritesh