| Message ID | 434beffaf18d39f898518ea9eb1cea4548e77c3a.1695383715.git.ritesh.list@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | generic: Add integrity tests with synchronous directio | expand |
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
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 >
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
"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
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 >
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
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
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