diff mbox series

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

Message ID 3c21207848460ffe8aab734b32c1c2464049296c.1695469920.git.ritesh.list@gmail.com
State Not Applicable
Headers show
Series [PATCHv2,1/2] aio-dio-write-verify: Add sync and noverify option | expand

Commit Message

Ritesh Harjani (IBM) Sept. 23, 2023, noon UTC 
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

Comments

Zorro Lang Sept. 28, 2023, 3:42 a.m. UTC | #1 
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
>
Ritesh Harjani (IBM) Sept. 28, 2023, 4:51 a.m. UTC | #2 
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
diff mbox series

Patch

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