2023-11-20 11:19:56

by Ritesh Harjani (IBM)

[permalink] [raw]
Subject: [PATCHv3 1/2] aio-dio-write-verify: Add sync and noverify option

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) <[email protected]>
---
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 dabbfacd..a7ca8307 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);
}

@@ -292,8 +295,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) {
@@ -308,6 +313,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]);
}
@@ -324,7 +335,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;
@@ -342,9 +353,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



2023-11-20 11:20:04

by Ritesh Harjani (IBM)

[permalink] [raw]
Subject: [PATCHv3 2/2] generic: Add integrity tests with synchronous directio

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/733 | 54 +++++++++++++++++++++++++++++++++++++++++++
tests/generic/733.out | 22 ++++++++++++++++++
2 files changed, 76 insertions(+)
create mode 100755 tests/generic/733
create mode 100644 tests/generic/733.out

diff --git a/tests/generic/733 b/tests/generic/733
new file mode 100755
index 00000000..18021e8a
--- /dev/null
+++ b/tests/generic/733
@@ -0,0 +1,54 @@
+#! /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.
+# Based on a testcase reported by Gao Xiang <[email protected]>
+#
+
+. ./common/preamble
+_begin_fstest auto quick shutdown aio
+
+# real QA test starts here
+_supported_fs generic
+_require_scratch
+_require_scratch_shutdown
+_require_aiodio aio-dio-write-verify
+
+_fixed_by_kernel_commit 91562895f803 \
+ "ext4: properly sync file size update after O_SYNC direct IO"
+
+_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/733.out b/tests/generic/733.out
new file mode 100644
index 00000000..e0536a4e
--- /dev/null
+++ b/tests/generic/733.out
@@ -0,0 +1,22 @@
+QA output created by 733
+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


2023-12-09 05:06:38

by Zorro Lang

[permalink] [raw]
Subject: Re: [PATCHv3 1/2] aio-dio-write-verify: Add sync and noverify option

On Mon, Nov 20, 2023 at 04:49:33PM +0530, Ritesh Harjani (IBM) wrote:
> 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) <[email protected]>
> ---

This version is good to me,
Reviewed-by: Zorro Lang <[email protected]>

> 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 dabbfacd..a7ca8307 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);
> }
>
> @@ -292,8 +295,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) {
> @@ -308,6 +313,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]);
> }
> @@ -324,7 +335,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;
> @@ -342,9 +353,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
>
>


2023-12-09 05:09:30

by Zorro Lang

[permalink] [raw]
Subject: Re: [PATCHv3 2/2] generic: Add integrity tests with synchronous directio

On Mon, Nov 20, 2023 at 04:49:34PM +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 <[email protected]>
> Signed-off-by: Ritesh Harjani (IBM) <[email protected]>
> ---
> tests/generic/733 | 54 +++++++++++++++++++++++++++++++++++++++++++
> tests/generic/733.out | 22 ++++++++++++++++++
> 2 files changed, 76 insertions(+)
> create mode 100755 tests/generic/733
> create mode 100644 tests/generic/733.out
>
> diff --git a/tests/generic/733 b/tests/generic/733
> new file mode 100755
> index 00000000..18021e8a
> --- /dev/null
> +++ b/tests/generic/733
> @@ -0,0 +1,54 @@
> +#! /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.
> +# Based on a testcase reported by Gao Xiang <[email protected]>
> +#
> +
> +. ./common/preamble
> +_begin_fstest auto quick shutdown aio
> +
> +# real QA test starts here
> +_supported_fs generic
> +_require_scratch
> +_require_scratch_shutdown
> +_require_aiodio aio-dio-write-verify
> +
> +_fixed_by_kernel_commit 91562895f803 \
> + "ext4: properly sync file size update after O_SYNC direct IO"

As this a generic test case, so:

[[ "$FSTYP" =~ ext* ]] && ...

Anyway, I'll change that when I merge it. Other looks good to me

Reviewed-by: Zorro Lang <[email protected]>

> +
> +_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/733.out b/tests/generic/733.out
> new file mode 100644
> index 00000000..e0536a4e
> --- /dev/null
> +++ b/tests/generic/733.out
> @@ -0,0 +1,22 @@
> +QA output created by 733
> +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
>
>