Message ID | 20210818084126.4167799-1-bxue@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | ext4: regression test for "tune2fs -l" after ext4 shutdown | expand |
On Wed, Aug 18, 2021 at 04:40:56PM +0800, bxue@redhat.com wrote: > From: Boyang Xue <bxue@redhat.com> > > Regression test for: > > ext4: Fix tune2fs checksum failure for mounted filesystem Better to specify the commit id number. I saw Ted has applied that patch: https://lore.kernel.org/linux-ext4/162895105421.460437.8931255765382647790.b4-ty@mit.edu/ And maybe you can describe *a little* more in commit log. > > Signed-off-by: Boyang Xue <bxue@redhat.com> > --- > Hi, > > This is a new regression test for the patch > > ``` > ext4: Fix tune2fs checksum failure for mounted filesystem > > Commit 81414b4dd48 ("ext4: remove redundant sb checksum recomputation") > removed checksum recalculation after updating superblock free space / > inode counters in ext4_fill_super() based on the fact that we will > recalculate the checksum on superblock writeout. That is correct > assumption but until the writeout happens (which can take a long time) > the checksum is incorrect in the buffer cache and if tune2fs is called > in that time window it will complain. So return back the checksum > recalculation and add a comment explaining the tune2fs peculiarity. > > Fixes: 81414b4dd48f ("ext4: remove redundant sb checksum recomputation") > Reported-by: Boyang Xue <bxue@xxxxxxxxxx> > Signed-off-by: Jan Kara <jack@xxxxxxx> > ``` > > It's expected to fail on kernels from the kernel-5.11-rc1 to the latest > version, where tune2fs fails with: > > ``` > tune2fs 1.46.2 (28-Feb-2021) > tune2fs: Superblock checksum does not match superblock while trying to > open /dev/loop0 > Couldn't find valid filesystem superblock. > ``` > > Please help review this test, Thanks! > > -Boyang > > tests/ext4/309 | 42 ++++++++++++++++++++++++++++++++++++++++++ > tests/ext4/309.out | 2 ++ > 2 files changed, 44 insertions(+) > create mode 100755 tests/ext4/309 > create mode 100644 tests/ext4/309.out > > diff --git a/tests/ext4/309 b/tests/ext4/309 > new file mode 100755 > index 00000000..ae335617 > --- /dev/null > +++ b/tests/ext4/309 > @@ -0,0 +1,42 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2021 YOUR NAME HERE. All Rights Reserved. ^^^^^^^^^^^^^^ Write your copyright > +# > +# FS QA Test 309 > +# > +# Test that tune2fs doesn't fail after ext4 shutdown > +# Regression test for commit: > +# ext4: Fix tune2fs checksum failure for mounted filesystem > +# > +. ./common/preamble > +_begin_fstest auto rw quick > + > +_cleanup() > +{ > + _scratch_unmount > +} I think the umount isn't necessary, so the specific _cleanup isn't needed either. > + > +# Import common functions. > +. ./common/filter Do you use any filter helpers below? > + > +# real QA test starts here > +_supported_fs ext4 I'm wondering if this case can be a generic case, there's nothing ext4 specified operations, except this line: "$TUNE2FS_PROG -l $SCRATCH_DEV" Hmm... if we can change this line to something likes _get_fs_super(), it might help to make this test to be a generic test. > +_require_scratch > +_require_scratch_shutdown > +_require_command "$TUNE2FS_PROG" tune2fs > + > +echo "Silence is golden" > + > +_scratch_mkfs >/dev/null 2>&1 > +_scratch_mount > +echo "ext4/309" > $SCRATCH_MNT/309.tmp It's sure this case will be "ext4/309", although you use "309" won't affect anything. > +_scratch_shutdown > +_scratch_cycle_mount > +$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 > +if [ $? -eq 0 ]; then > + status=0 > +else > + status=1 > +fi Don't need to change the status value, how about write as: $TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null The error output will break the golden image directly. ( cc ext4 mailist, to get more review) Thanks, Zorro > + > +exit > diff --git a/tests/ext4/309.out b/tests/ext4/309.out > new file mode 100644 > index 00000000..56330d65 > --- /dev/null > +++ b/tests/ext4/309.out > @@ -0,0 +1,2 @@ > +QA output created by 309 > +Silence is golden > -- > 2.27.0 >
Hi Zorro, On Wed, Aug 18, 2021 at 7:32 PM Zorro Lang <zlang@redhat.com> wrote: > > On Wed, Aug 18, 2021 at 04:40:56PM +0800, bxue@redhat.com wrote: > > From: Boyang Xue <bxue@redhat.com> > > > > Regression test for: > > > > ext4: Fix tune2fs checksum failure for mounted filesystem > > Better to specify the commit id number. I saw Ted has applied that patch: > > https://lore.kernel.org/linux-ext4/162895105421.460437.8931255765382647790.b4-ty@mit.edu/ Thanks. I see the commit id e905fbe3fd0fdb90052f6efdf88f50a78833cfe7 in the above URL. I didn't add it since I'm not sure if this id will be the final id when the commit is finally merged to the mainline kernel (Linus tree)? > > And maybe you can describe *a little* more in commit log. Yes I can add a few words in the commit log, but actually I expect the reader of this test reads the commit message of the mentioned commit "ext4: Fix tune2fs checksum failure for mounted filesystem", which I think is more precise. > > > > > Signed-off-by: Boyang Xue <bxue@redhat.com> > > --- > > Hi, > > > > This is a new regression test for the patch > > > > ``` > > ext4: Fix tune2fs checksum failure for mounted filesystem > > > > Commit 81414b4dd48 ("ext4: remove redundant sb checksum recomputation") > > removed checksum recalculation after updating superblock free space / > > inode counters in ext4_fill_super() based on the fact that we will > > recalculate the checksum on superblock writeout. That is correct > > assumption but until the writeout happens (which can take a long time) > > the checksum is incorrect in the buffer cache and if tune2fs is called > > in that time window it will complain. So return back the checksum > > recalculation and add a comment explaining the tune2fs peculiarity. > > > > Fixes: 81414b4dd48f ("ext4: remove redundant sb checksum recomputation") > > Reported-by: Boyang Xue <bxue@xxxxxxxxxx> > > Signed-off-by: Jan Kara <jack@xxxxxxx> > > ``` > > > > It's expected to fail on kernels from the kernel-5.11-rc1 to the latest > > version, where tune2fs fails with: > > > > ``` > > tune2fs 1.46.2 (28-Feb-2021) > > tune2fs: Superblock checksum does not match superblock while trying to > > open /dev/loop0 > > Couldn't find valid filesystem superblock. > > ``` > > > > Please help review this test, Thanks! > > > > -Boyang > > > > tests/ext4/309 | 42 ++++++++++++++++++++++++++++++++++++++++++ > > tests/ext4/309.out | 2 ++ > > 2 files changed, 44 insertions(+) > > create mode 100755 tests/ext4/309 > > create mode 100644 tests/ext4/309.out > > > > diff --git a/tests/ext4/309 b/tests/ext4/309 > > new file mode 100755 > > index 00000000..ae335617 > > --- /dev/null > > +++ b/tests/ext4/309 > > @@ -0,0 +1,42 @@ > > +#! /bin/bash > > +# SPDX-License-Identifier: GPL-2.0 > > +# Copyright (c) 2021 YOUR NAME HERE. All Rights Reserved. > ^^^^^^^^^^^^^^ > Write your copyright I will correct it in the next version. Thanks. > > > +# > > +# FS QA Test 309 > > +# > > +# Test that tune2fs doesn't fail after ext4 shutdown > > +# Regression test for commit: > > +# ext4: Fix tune2fs checksum failure for mounted filesystem > > +# > > +. ./common/preamble > > +_begin_fstest auto rw quick > > + > > +_cleanup() > > +{ > > + _scratch_unmount > > +} > > I think the umount isn't necessary, so the specific _cleanup isn't > needed either. The $SCRATCH_DEV was still mounted before this _cleanup(), so I'm wondering why we shouldn't do _scratch_unmount here? And I see at least another similar structured test ext4/306 do _scratch_unmount in _cleanup(). > > > + > > +# Import common functions. > > +. ./common/filter > > Do you use any filter helpers below? No. I will remove this line in my next version. > > > + > > +# real QA test starts here > > +_supported_fs ext4 > > I'm wondering if this case can be a generic case, there's nothing > ext4 specified operations, except this line: > > "$TUNE2FS_PROG -l $SCRATCH_DEV" > > Hmm... if we can change this line to something likes _get_fs_super(), > it might help to make this test to be a generic test. I think this bug is heavily related to "tune2fs", ext4 only. So I guess an ext4 only test is enough? > > > +_require_scratch > > +_require_scratch_shutdown > > +_require_command "$TUNE2FS_PROG" tune2fs > > + > > +echo "Silence is golden" > > + > > +_scratch_mkfs >/dev/null 2>&1 > > +_scratch_mount > > +echo "ext4/309" > $SCRATCH_MNT/309.tmp > > It's sure this case will be "ext4/309", although you use "309" won't > affect anything. Yes I can rename it to something like ext4-309.tmp if it looks better. > > > +_scratch_shutdown > > +_scratch_cycle_mount > > +$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 > > +if [ $? -eq 0 ]; then > > + status=0 > > +else > > + status=1 > > +fi > > Don't need to change the status value, how about write as: > > $TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null > > The error output will break the golden image directly. How did you test that? The error output didn't break the "golden image" in my test. > > ( cc ext4 mailist, to get more review) > > Thanks, > Zorro Thanks for review! -Boyang > > > + > > +exit > > diff --git a/tests/ext4/309.out b/tests/ext4/309.out > > new file mode 100644 > > index 00000000..56330d65 > > --- /dev/null > > +++ b/tests/ext4/309.out > > @@ -0,0 +1,2 @@ > > +QA output created by 309 > > +Silence is golden > > -- > > 2.27.0 > > >
On Wed 18-08-21 21:20:44, Boyang Xue wrote: > > > + > > > +# real QA test starts here > > > +_supported_fs ext4 > > > > I'm wondering if this case can be a generic case, there's nothing > > ext4 specified operations, except this line: > > > > "$TUNE2FS_PROG -l $SCRATCH_DEV" > > > > Hmm... if we can change this line to something likes _get_fs_super(), > > it might help to make this test to be a generic test. > > I think this bug is heavily related to "tune2fs", ext4 only. So I > guess an ext4 only test is enough? FWIW I agree with Boyang here. For this test to make sense for any other filesystem other the filesystem would need to read fs metadata through buffer cache in _get_fs_super(). Furthermore it is somewhat ext2/3/4 specific (due to historical reasons) that reading superblock from the buffer cache of a mounted filesystem is expected to result in something sensible. Usually this is plain unsupported use... Honza
On Wed, Aug 18, 2021 at 09:20:44PM +0800, Boyang Xue wrote: > Hi Zorro, > > On Wed, Aug 18, 2021 at 7:32 PM Zorro Lang <zlang@redhat.com> wrote: > > > > On Wed, Aug 18, 2021 at 04:40:56PM +0800, bxue@redhat.com wrote: > > > From: Boyang Xue <bxue@redhat.com> > > > > > > Regression test for: > > > > > > ext4: Fix tune2fs checksum failure for mounted filesystem > > > > Better to specify the commit id number. I saw Ted has applied that patch: > > > > https://lore.kernel.org/linux-ext4/162895105421.460437.8931255765382647790.b4-ty@mit.edu/ > > Thanks. I see the commit id e905fbe3fd0fdb90052f6efdf88f50a78833cfe7 > in the above URL. I didn't add it since I'm not sure if this id will > be the final id when the commit is finally merged to the mainline > kernel (Linus tree)? > > > > > And maybe you can describe *a little* more in commit log. > > Yes I can add a few words in the commit log, but actually I expect the > reader of this test reads the commit message of the mentioned commit > "ext4: Fix tune2fs checksum failure for mounted filesystem", which I > think is more precise. > > > > > > > > > Signed-off-by: Boyang Xue <bxue@redhat.com> > > > --- > > > Hi, > > > > > > This is a new regression test for the patch > > > > > > ``` > > > ext4: Fix tune2fs checksum failure for mounted filesystem > > > > > > Commit 81414b4dd48 ("ext4: remove redundant sb checksum recomputation") > > > removed checksum recalculation after updating superblock free space / > > > inode counters in ext4_fill_super() based on the fact that we will > > > recalculate the checksum on superblock writeout. That is correct > > > assumption but until the writeout happens (which can take a long time) > > > the checksum is incorrect in the buffer cache and if tune2fs is called > > > in that time window it will complain. So return back the checksum > > > recalculation and add a comment explaining the tune2fs peculiarity. > > > > > > Fixes: 81414b4dd48f ("ext4: remove redundant sb checksum recomputation") > > > Reported-by: Boyang Xue <bxue@xxxxxxxxxx> > > > Signed-off-by: Jan Kara <jack@xxxxxxx> > > > ``` > > > > > > It's expected to fail on kernels from the kernel-5.11-rc1 to the latest > > > version, where tune2fs fails with: > > > > > > ``` > > > tune2fs 1.46.2 (28-Feb-2021) > > > tune2fs: Superblock checksum does not match superblock while trying to > > > open /dev/loop0 > > > Couldn't find valid filesystem superblock. > > > ``` > > > > > > Please help review this test, Thanks! > > > > > > -Boyang > > > > > > tests/ext4/309 | 42 ++++++++++++++++++++++++++++++++++++++++++ > > > tests/ext4/309.out | 2 ++ > > > 2 files changed, 44 insertions(+) > > > create mode 100755 tests/ext4/309 > > > create mode 100644 tests/ext4/309.out > > > > > > diff --git a/tests/ext4/309 b/tests/ext4/309 > > > new file mode 100755 > > > index 00000000..ae335617 > > > --- /dev/null > > > +++ b/tests/ext4/309 > > > @@ -0,0 +1,42 @@ > > > +#! /bin/bash > > > +# SPDX-License-Identifier: GPL-2.0 > > > +# Copyright (c) 2021 YOUR NAME HERE. All Rights Reserved. > > ^^^^^^^^^^^^^^ > > Write your copyright > > I will correct it in the next version. Thanks. > > > > > > +# > > > +# FS QA Test 309 > > > +# > > > +# Test that tune2fs doesn't fail after ext4 shutdown > > > +# Regression test for commit: > > > +# ext4: Fix tune2fs checksum failure for mounted filesystem > > > +# > > > +. ./common/preamble > > > +_begin_fstest auto rw quick > > > + > > > +_cleanup() > > > +{ > > > + _scratch_unmount > > > +} > > > > I think the umount isn't necessary, so the specific _cleanup isn't > > needed either. > > The $SCRATCH_DEV was still mounted before this _cleanup(), so I'm > wondering why we shouldn't do _scratch_unmount here? And I see at > least another similar structured test ext4/306 do _scratch_unmount in > _cleanup(). The SCRATCH_DEV will be umounted, don't need to do that in the end of each test cases, except you need to do something on the unmounted SCRATCH_DEV in the end. I don't know why ext4/306 has that, maybe due to old reason, or we didn't notice/care that when we reviewed it? And it's not worth sending a patch just for removing this "not wrong but just redundant" line now. Except a patch trys to cleanup all _cleanup(). > > > > > > + > > > +# Import common functions. > > > +. ./common/filter > > > > Do you use any filter helpers below? > > No. I will remove this line in my next version. > > > > > > + > > > +# real QA test starts here > > > +_supported_fs ext4 > > > > I'm wondering if this case can be a generic case, there's nothing > > ext4 specified operations, except this line: > > > > "$TUNE2FS_PROG -l $SCRATCH_DEV" > > > > Hmm... if we can change this line to something likes _get_fs_super(), > > it might help to make this test to be a generic test. > > I think this bug is heavily related to "tune2fs", ext4 only. So I > guess an ext4 only test is enough? Just checking. That's fine for me to keep this case as an ext4 only case. > > > > > > +_require_scratch > > > +_require_scratch_shutdown > > > +_require_command "$TUNE2FS_PROG" tune2fs > > > + > > > +echo "Silence is golden" > > > + > > > +_scratch_mkfs >/dev/null 2>&1 > > > +_scratch_mount > > > +echo "ext4/309" > $SCRATCH_MNT/309.tmp > > > > It's sure this case will be "ext4/309", although you use "309" won't > > affect anything. > > Yes I can rename it to something like ext4-309.tmp if it looks better. I think something likes: echo "This is a test" > $SCRATCH_MNT/testfile is good enough, don't need the "ext4" or "309" things. > > > > > > +_scratch_shutdown > > > +_scratch_cycle_mount > > > +$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 > > > +if [ $? -eq 0 ]; then > > > + status=0 > > > +else > > > + status=1 > > > +fi > > > > Don't need to change the status value, how about write as: > > > > $TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null > > > > The error output will break the golden image directly. > > How did you test that? The error output didn't break the "golden > image" in my test. [root@xx-xxxx-xx xfstests-dev]# ./check ext4/309 FSTYP -- ext4 PLATFORM -- Linux/x86_64 xx-xxxx-xx 5.14.0-rc4-xfs #14 SMP Thu Aug 12 00:56:07 CST 2021 MKFS_OPTIONS -- /dev/mapper/testvg-scratchdev MOUNT_OPTIONS -- -o acl,user_xattr -o context=system_u:object_r:root_t:s0 /dev/mapper/testvg-scratchdev /mnt/scratch ext4/309 5s ... - output mismatch (see /root/git/xfstests-dev/results//ext4/309.out.bad) --- tests/ext4/309.out 2021-08-18 18:17:15.925427714 +0800 +++ /root/git/xfstests-dev/results//ext4/309.out.bad 2021-08-19 00:17:57.001648868 +0800 @@ -1,2 +1,4 @@ QA output created by 309 Silence is golden +/usr/sbin/tune2fs: Superblock checksum does not match superblock while trying to open /dev/mapper/testvg-scratchdev +Couldn't find valid filesystem superblock. ... (Run 'diff -u /root/git/xfstests-dev/tests/ext4/309.out /root/git/xfstests-dev/results//ext4/309.out.bad' to see the entire diff) Ran: ext4/309 Failures: ext4/309 Failed 1 of 1 tests [root@xx-xxxx-xx xfstests-dev]# git diff ... _scratch_mkfs >/dev/null 2>&1 _scratch_mount -echo "ext4/309" > $SCRATCH_MNT/309.tmp +echo "This is a test" > $SCRATCH_MNT/testfile _scratch_shutdown -_scratch_cycle_mount -$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 -if [ $? -eq 0 ]; then - status=0 -else - status=1 -fi +_scratch_cycle_mount +$TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null +status=0 exit Thanks, Zorro > > > > > ( cc ext4 mailist, to get more review) > > > > Thanks, > > Zorro > > Thanks for review! > > -Boyang > > > > > > + > > > +exit > > > diff --git a/tests/ext4/309.out b/tests/ext4/309.out > > > new file mode 100644 > > > index 00000000..56330d65 > > > --- /dev/null > > > +++ b/tests/ext4/309.out > > > @@ -0,0 +1,2 @@ > > > +QA output created by 309 > > > +Silence is golden > > > -- > > > 2.27.0 > > > > > >
On Wed, Aug 18, 2021 at 04:26:01PM +0200, Jan Kara wrote: > On Wed 18-08-21 21:20:44, Boyang Xue wrote: > > > > + > > > > +# real QA test starts here > > > > +_supported_fs ext4 > > > > > > I'm wondering if this case can be a generic case, there's nothing > > > ext4 specified operations, except this line: > > > > > > "$TUNE2FS_PROG -l $SCRATCH_DEV" > > > > > > Hmm... if we can change this line to something likes _get_fs_super(), > > > it might help to make this test to be a generic test. > > > > I think this bug is heavily related to "tune2fs", ext4 only. So I > > guess an ext4 only test is enough? > > FWIW I agree with Boyang here. For this test to make sense for any other > filesystem other the filesystem would need to read fs metadata through > buffer cache in _get_fs_super(). Furthermore it is somewhat ext2/3/4 > specific (due to historical reasons) that reading superblock from the > buffer cache of a mounted filesystem is expected to result in something > sensible. Usually this is plain unsupported use... Thanks for this explanation:) I didn't ask for extending this test to be a generic test, just checking others ideas:) Due to although tune2fs is special, but the test steps are common: 1) mkfs 2) mount 3) write io 4) shutdown fs 5) umount && mount 6) read sb from a mounted fs (make sure using tune2fs for ext4) Anyway, keep this test as ext4 only is fine for me :) Thanks, Zorro > > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR >
Zorro, Please check my reply inline below. On Thu, Aug 19, 2021 at 12:47 AM Zorro Lang <zlang@redhat.com> wrote: > > On Wed, Aug 18, 2021 at 09:20:44PM +0800, Boyang Xue wrote: > > Hi Zorro, > > > > On Wed, Aug 18, 2021 at 7:32 PM Zorro Lang <zlang@redhat.com> wrote: > > > > > > On Wed, Aug 18, 2021 at 04:40:56PM +0800, bxue@redhat.com wrote: > > > > From: Boyang Xue <bxue@redhat.com> > > > > > > > > Regression test for: > > > > > > > > ext4: Fix tune2fs checksum failure for mounted filesystem > > > > > > Better to specify the commit id number. I saw Ted has applied that patch: > > > > > > https://lore.kernel.org/linux-ext4/162895105421.460437.8931255765382647790.b4-ty@mit.edu/ > > > > Thanks. I see the commit id e905fbe3fd0fdb90052f6efdf88f50a78833cfe7 > > in the above URL. I didn't add it since I'm not sure if this id will > > be the final id when the commit is finally merged to the mainline > > kernel (Linus tree)? > > > > > > > > And maybe you can describe *a little* more in commit log. > > > > Yes I can add a few words in the commit log, but actually I expect the > > reader of this test reads the commit message of the mentioned commit > > "ext4: Fix tune2fs checksum failure for mounted filesystem", which I > > think is more precise. > > > > > > > > > > > > > Signed-off-by: Boyang Xue <bxue@redhat.com> > > > > --- > > > > Hi, > > > > > > > > This is a new regression test for the patch > > > > > > > > ``` > > > > ext4: Fix tune2fs checksum failure for mounted filesystem > > > > > > > > Commit 81414b4dd48 ("ext4: remove redundant sb checksum recomputation") > > > > removed checksum recalculation after updating superblock free space / > > > > inode counters in ext4_fill_super() based on the fact that we will > > > > recalculate the checksum on superblock writeout. That is correct > > > > assumption but until the writeout happens (which can take a long time) > > > > the checksum is incorrect in the buffer cache and if tune2fs is called > > > > in that time window it will complain. So return back the checksum > > > > recalculation and add a comment explaining the tune2fs peculiarity. > > > > > > > > Fixes: 81414b4dd48f ("ext4: remove redundant sb checksum recomputation") > > > > Reported-by: Boyang Xue <bxue@xxxxxxxxxx> > > > > Signed-off-by: Jan Kara <jack@xxxxxxx> > > > > ``` > > > > > > > > It's expected to fail on kernels from the kernel-5.11-rc1 to the latest > > > > version, where tune2fs fails with: > > > > > > > > ``` > > > > tune2fs 1.46.2 (28-Feb-2021) > > > > tune2fs: Superblock checksum does not match superblock while trying to > > > > open /dev/loop0 > > > > Couldn't find valid filesystem superblock. > > > > ``` > > > > > > > > Please help review this test, Thanks! > > > > > > > > -Boyang > > > > > > > > tests/ext4/309 | 42 ++++++++++++++++++++++++++++++++++++++++++ > > > > tests/ext4/309.out | 2 ++ > > > > 2 files changed, 44 insertions(+) > > > > create mode 100755 tests/ext4/309 > > > > create mode 100644 tests/ext4/309.out > > > > > > > > diff --git a/tests/ext4/309 b/tests/ext4/309 > > > > new file mode 100755 > > > > index 00000000..ae335617 > > > > --- /dev/null > > > > +++ b/tests/ext4/309 > > > > @@ -0,0 +1,42 @@ > > > > +#! /bin/bash > > > > +# SPDX-License-Identifier: GPL-2.0 > > > > +# Copyright (c) 2021 YOUR NAME HERE. All Rights Reserved. > > > ^^^^^^^^^^^^^^ > > > Write your copyright > > > > I will correct it in the next version. Thanks. > > > > > > > > > +# > > > > +# FS QA Test 309 > > > > +# > > > > +# Test that tune2fs doesn't fail after ext4 shutdown > > > > +# Regression test for commit: > > > > +# ext4: Fix tune2fs checksum failure for mounted filesystem > > > > +# > > > > +. ./common/preamble > > > > +_begin_fstest auto rw quick > > > > + > > > > +_cleanup() > > > > +{ > > > > + _scratch_unmount > > > > +} > > > > > > I think the umount isn't necessary, so the specific _cleanup isn't > > > needed either. > > > > The $SCRATCH_DEV was still mounted before this _cleanup(), so I'm > > wondering why we shouldn't do _scratch_unmount here? And I see at > > least another similar structured test ext4/306 do _scratch_unmount in > > _cleanup(). > > The SCRATCH_DEV will be umounted, don't need to do that in the end of each > test cases, except you need to do something on the unmounted SCRATCH_DEV > in the end. > > I don't know why ext4/306 has that, maybe due to old reason, or we didn't > notice/care that when we reviewed it? And it's not worth sending a patch > just for removing this "not wrong but just redundant" line now. Except a > patch trys to cleanup all _cleanup(). OK. I will remove the _cleanup() in my next version, unless someone else has other opinions. > > > > > > > > > > + > > > > +# Import common functions. > > > > +. ./common/filter > > > > > > Do you use any filter helpers below? > > > > No. I will remove this line in my next version. > > > > > > > > > + > > > > +# real QA test starts here > > > > +_supported_fs ext4 > > > > > > I'm wondering if this case can be a generic case, there's nothing > > > ext4 specified operations, except this line: > > > > > > "$TUNE2FS_PROG -l $SCRATCH_DEV" > > > > > > Hmm... if we can change this line to something likes _get_fs_super(), > > > it might help to make this test to be a generic test. > > > > I think this bug is heavily related to "tune2fs", ext4 only. So I > > guess an ext4 only test is enough? > > Just checking. That's fine for me to keep this case as an ext4 only case. > > > > > > > > > > +_require_scratch > > > > +_require_scratch_shutdown > > > > +_require_command "$TUNE2FS_PROG" tune2fs > > > > + > > > > +echo "Silence is golden" > > > > + > > > > +_scratch_mkfs >/dev/null 2>&1 > > > > +_scratch_mount > > > > +echo "ext4/309" > $SCRATCH_MNT/309.tmp > > > > > > It's sure this case will be "ext4/309", although you use "309" won't > > > affect anything. > > > > Yes I can rename it to something like ext4-309.tmp if it looks better. > > I think something likes: echo "This is a test" > $SCRATCH_MNT/testfile > is good enough, don't need the "ext4" or "309" things. OK. I will modify it to echo "This is a test" > $SCRATCH_MNT/testfile Unless someone else has other preferences. > > > > > > > > > > +_scratch_shutdown > > > > +_scratch_cycle_mount > > > > +$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 > > > > +if [ $? -eq 0 ]; then > > > > + status=0 > > > > +else > > > > + status=1 > > > > +fi > > > > > > Don't need to change the status value, how about write as: > > > > > > $TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null > > > > > > The error output will break the golden image directly. > > > > How did you test that? The error output didn't break the "golden > > image" in my test. > > [root@xx-xxxx-xx xfstests-dev]# ./check ext4/309 > FSTYP -- ext4 > PLATFORM -- Linux/x86_64 xx-xxxx-xx 5.14.0-rc4-xfs #14 SMP Thu Aug 12 00:56:07 CST 2021 > MKFS_OPTIONS -- /dev/mapper/testvg-scratchdev > MOUNT_OPTIONS -- -o acl,user_xattr -o context=system_u:object_r:root_t:s0 /dev/mapper/testvg-scratchdev /mnt/scratch > > ext4/309 5s ... - output mismatch (see /root/git/xfstests-dev/results//ext4/309.out.bad) > --- tests/ext4/309.out 2021-08-18 18:17:15.925427714 +0800 > +++ /root/git/xfstests-dev/results//ext4/309.out.bad 2021-08-19 00:17:57.001648868 +0800 > @@ -1,2 +1,4 @@ > QA output created by 309 > Silence is golden > +/usr/sbin/tune2fs: Superblock checksum does not match superblock while trying to open /dev/mapper/testvg-scratchdev > +Couldn't find valid filesystem superblock. > ... > (Run 'diff -u /root/git/xfstests-dev/tests/ext4/309.out /root/git/xfstests-dev/results//ext4/309.out.bad' to see the entire diff) > Ran: ext4/309 > Failures: ext4/309 > Failed 1 of 1 tests > > [root@xx-xxxx-xx xfstests-dev]# git diff > ... > _scratch_mkfs >/dev/null 2>&1 > _scratch_mount > -echo "ext4/309" > $SCRATCH_MNT/309.tmp > +echo "This is a test" > $SCRATCH_MNT/testfile > _scratch_shutdown > -_scratch_cycle_mount > -$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 > -if [ $? -eq 0 ]; then > - status=0 > -else > - status=1 > -fi > +_scratch_cycle_mount > +$TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null > > +status=0 > exit Unless I missed something, I would say that, tune2fs' error output went to .out.bad just because you had modified the tune2fs line from mine: $TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 to yours: $TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null My original version redirecting stderr and stdout both to $seqres.full doesn't break the "golden output". Test log: ``` [root@kvm102 repo_xfstests]# ./check ext4/309 FSTYP -- ext4 PLATFORM -- Linux/x86_64 kvm102 5.14.0-0.rc4.35.xx.x86_64 #1 SMP Tue Aug 3 13:02:44 EDT 2021 MKFS_OPTIONS -- -b 1024 /dev/vda3 MOUNT_OPTIONS -- -o acl,user_xattr -o context=system_u:object_r:root_t:s0 /dev/vda3 /scratch ext4/309 [failed, exit status 1] Ran: ext4/309 Failures: ext4/309 Failed 1 of 1 tests [root@kvm102 repo_xfstests]# cat results/ext4/309.full /usr/sbin/tune2fs: Superblock checksum does not match superblock while trying to open /dev/vda3 Couldn't find valid filesystem superblock. tune2fs 1.46.2 (28-Feb-2021) [root@kvm102 repo_xfstests]# ls results/ext4/309.out.bad ls: cannot access 'results/ext4/309.out.bad': No such file or directory ``` As far as I can understand, there're at least two approaches to mark a test pass or fail in xfstests: 1) Compare the output of the test with the "golden output" (i.e. 309.out), I guess this is the approach you mentioned. 2) Exit the test with the value of $status (i.e. status=0 - pass, status=non-zero - fail) I'm using the 2) here, not using 1) with the output of tune2fs compared with the 309.out, because the output of tune2fs can be different in different runs. An example output of a successful tune2fs run is like (sorry for this long paste): ``` [root@kvm101 repo_xfstests]# cat results/ext4/309.full tune2fs 1.45.6 (20-Mar-2020) Filesystem volume name: <none> Last mounted on: /scratch Filesystem UUID: 22975090-96d7-49ee-9b4f-ee6afe046219 Filesystem magic number: 0xEF53 Filesystem revision #: 1 (dynamic) Filesystem features: has_journal ext_attr resize_inode dir_index filetype needs_recovery extent 64bit flex_bg sparse_super large_file huge_file dir_nlink extra_isize metadata_csum Filesystem flags: signed_directory_hash Default mount options: user_xattr acl Filesystem state: clean Errors behavior: Continue Filesystem OS type: Linux Inode count: 720896 Block count: 11534336 Reserved block count: 576716 Free blocks: 11280570 Free inodes: 720884 First block: 1 Block size: 1024 Fragment size: 1024 Group descriptor size: 64 Reserved GDT blocks: 256 Blocks per group: 8192 Fragments per group: 8192 Inodes per group: 512 Inode blocks per group: 128 Flex block group size: 16 Filesystem created: Thu Aug 19 05:45:26 2021 Last mount time: Thu Aug 19 05:45:26 2021 Last write time: Thu Aug 19 05:45:26 2021 Mount count: 2 Maximum mount count: -1 Last checked: Thu Aug 19 05:45:26 2021 Check interval: 0 (<none>) Lifetime writes: 462 kB Reserved blocks uid: 0 (user root) Reserved blocks gid: 0 (group root) First inode: 11 Inode size: 256 Required extra isize: 32 Desired extra isize: 32 Journal inode: 8 Default directory hash: half_md4 Directory Hash Seed: f5af5862-d415-460b-9a6b-97584578600f Journal backup: inode blocks Checksum type: crc32c Checksum: 0x0f5b9c13 ``` Thanks, Boyang > > > Thanks, > Zorro > > > > > > > > > ( cc ext4 mailist, to get more review) > > > > > > Thanks, > > > Zorro > > > > Thanks for review! > > > > -Boyang > > > > > > > > > + > > > > +exit > > > > diff --git a/tests/ext4/309.out b/tests/ext4/309.out > > > > new file mode 100644 > > > > index 00000000..56330d65 > > > > --- /dev/null > > > > +++ b/tests/ext4/309.out > > > > @@ -0,0 +1,2 @@ > > > > +QA output created by 309 > > > > +Silence is golden > > > > -- > > > > 2.27.0 > > > > > > > > > >
On Thu, Aug 19, 2021 at 01:48:55PM +0800, Boyang Xue wrote: > Zorro, > > Please check my reply inline below. > > On Thu, Aug 19, 2021 at 12:47 AM Zorro Lang <zlang@redhat.com> wrote: > > > > On Wed, Aug 18, 2021 at 09:20:44PM +0800, Boyang Xue wrote: > > > Hi Zorro, > > > > > > On Wed, Aug 18, 2021 at 7:32 PM Zorro Lang <zlang@redhat.com> wrote: > > > > > > > > On Wed, Aug 18, 2021 at 04:40:56PM +0800, bxue@redhat.com wrote: > > > > > From: Boyang Xue <bxue@redhat.com> > > > > > > > > > > Regression test for: > > > > > > > > > > ext4: Fix tune2fs checksum failure for mounted filesystem > > > > > > > > Better to specify the commit id number. I saw Ted has applied that patch: > > > > > > > > https://lore.kernel.org/linux-ext4/162895105421.460437.8931255765382647790.b4-ty@mit.edu/ > > > > > > Thanks. I see the commit id e905fbe3fd0fdb90052f6efdf88f50a78833cfe7 > > > in the above URL. I didn't add it since I'm not sure if this id will > > > be the final id when the commit is finally merged to the mainline > > > kernel (Linus tree)? > > > > > > > > > > > And maybe you can describe *a little* more in commit log. > > > > > > Yes I can add a few words in the commit log, but actually I expect the > > > reader of this test reads the commit message of the mentioned commit > > > "ext4: Fix tune2fs checksum failure for mounted filesystem", which I > > > think is more precise. > > > > > > > > > > > > > > > > > Signed-off-by: Boyang Xue <bxue@redhat.com> > > > > > --- > > > > > Hi, > > > > > > > > > > This is a new regression test for the patch > > > > > > > > > > ``` > > > > > ext4: Fix tune2fs checksum failure for mounted filesystem > > > > > > > > > > Commit 81414b4dd48 ("ext4: remove redundant sb checksum recomputation") > > > > > removed checksum recalculation after updating superblock free space / > > > > > inode counters in ext4_fill_super() based on the fact that we will > > > > > recalculate the checksum on superblock writeout. That is correct > > > > > assumption but until the writeout happens (which can take a long time) > > > > > the checksum is incorrect in the buffer cache and if tune2fs is called > > > > > in that time window it will complain. So return back the checksum > > > > > recalculation and add a comment explaining the tune2fs peculiarity. > > > > > > > > > > Fixes: 81414b4dd48f ("ext4: remove redundant sb checksum recomputation") > > > > > Reported-by: Boyang Xue <bxue@xxxxxxxxxx> > > > > > Signed-off-by: Jan Kara <jack@xxxxxxx> > > > > > ``` > > > > > > > > > > It's expected to fail on kernels from the kernel-5.11-rc1 to the latest > > > > > version, where tune2fs fails with: > > > > > > > > > > ``` > > > > > tune2fs 1.46.2 (28-Feb-2021) > > > > > tune2fs: Superblock checksum does not match superblock while trying to > > > > > open /dev/loop0 > > > > > Couldn't find valid filesystem superblock. > > > > > ``` > > > > > > > > > > Please help review this test, Thanks! > > > > > > > > > > -Boyang > > > > > > > > > > tests/ext4/309 | 42 ++++++++++++++++++++++++++++++++++++++++++ > > > > > tests/ext4/309.out | 2 ++ > > > > > 2 files changed, 44 insertions(+) > > > > > create mode 100755 tests/ext4/309 > > > > > create mode 100644 tests/ext4/309.out > > > > > > > > > > diff --git a/tests/ext4/309 b/tests/ext4/309 > > > > > new file mode 100755 > > > > > index 00000000..ae335617 > > > > > --- /dev/null > > > > > +++ b/tests/ext4/309 > > > > > @@ -0,0 +1,42 @@ > > > > > +#! /bin/bash > > > > > +# SPDX-License-Identifier: GPL-2.0 > > > > > +# Copyright (c) 2021 YOUR NAME HERE. All Rights Reserved. > > > > ^^^^^^^^^^^^^^ > > > > Write your copyright > > > > > > I will correct it in the next version. Thanks. > > > > > > > > > > > > +# > > > > > +# FS QA Test 309 > > > > > +# > > > > > +# Test that tune2fs doesn't fail after ext4 shutdown > > > > > +# Regression test for commit: > > > > > +# ext4: Fix tune2fs checksum failure for mounted filesystem > > > > > +# > > > > > +. ./common/preamble > > > > > +_begin_fstest auto rw quick > > > > > + > > > > > +_cleanup() > > > > > +{ > > > > > + _scratch_unmount > > > > > +} > > > > > > > > I think the umount isn't necessary, so the specific _cleanup isn't > > > > needed either. > > > > > > The $SCRATCH_DEV was still mounted before this _cleanup(), so I'm > > > wondering why we shouldn't do _scratch_unmount here? And I see at > > > least another similar structured test ext4/306 do _scratch_unmount in > > > _cleanup(). > > > > The SCRATCH_DEV will be umounted, don't need to do that in the end of each > > test cases, except you need to do something on the unmounted SCRATCH_DEV > > in the end. > > > > I don't know why ext4/306 has that, maybe due to old reason, or we didn't > > notice/care that when we reviewed it? And it's not worth sending a patch > > just for removing this "not wrong but just redundant" line now. Except a > > patch trys to cleanup all _cleanup(). > > OK. I will remove the _cleanup() in my next version, unless someone > else has other opinions. > > > > > > > > > > > > > > > + > > > > > +# Import common functions. > > > > > +. ./common/filter > > > > > > > > Do you use any filter helpers below? > > > > > > No. I will remove this line in my next version. > > > > > > > > > > > > + > > > > > +# real QA test starts here > > > > > +_supported_fs ext4 > > > > > > > > I'm wondering if this case can be a generic case, there's nothing > > > > ext4 specified operations, except this line: > > > > > > > > "$TUNE2FS_PROG -l $SCRATCH_DEV" > > > > > > > > Hmm... if we can change this line to something likes _get_fs_super(), > > > > it might help to make this test to be a generic test. > > > > > > I think this bug is heavily related to "tune2fs", ext4 only. So I > > > guess an ext4 only test is enough? > > > > Just checking. That's fine for me to keep this case as an ext4 only case. > > > > > > > > > > > > > > +_require_scratch > > > > > +_require_scratch_shutdown > > > > > +_require_command "$TUNE2FS_PROG" tune2fs > > > > > + > > > > > +echo "Silence is golden" > > > > > + > > > > > +_scratch_mkfs >/dev/null 2>&1 > > > > > +_scratch_mount > > > > > +echo "ext4/309" > $SCRATCH_MNT/309.tmp > > > > > > > > It's sure this case will be "ext4/309", although you use "309" won't > > > > affect anything. > > > > > > Yes I can rename it to something like ext4-309.tmp if it looks better. > > > > I think something likes: echo "This is a test" > $SCRATCH_MNT/testfile > > is good enough, don't need the "ext4" or "309" things. > > OK. I will modify it to > > echo "This is a test" > $SCRATCH_MNT/testfile > > Unless someone else has other preferences. > > > > > > > > > > > > > > > +_scratch_shutdown > > > > > +_scratch_cycle_mount > > > > > +$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 > > > > > +if [ $? -eq 0 ]; then > > > > > + status=0 > > > > > +else > > > > > + status=1 > > > > > +fi > > > > > > > > Don't need to change the status value, how about write as: > > > > > > > > $TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null > > > > > > > > The error output will break the golden image directly. > > > > > > How did you test that? The error output didn't break the "golden > > > image" in my test. > > > > [root@xx-xxxx-xx xfstests-dev]# ./check ext4/309 > > FSTYP -- ext4 > > PLATFORM -- Linux/x86_64 xx-xxxx-xx 5.14.0-rc4-xfs #14 SMP Thu Aug 12 00:56:07 CST 2021 > > MKFS_OPTIONS -- /dev/mapper/testvg-scratchdev > > MOUNT_OPTIONS -- -o acl,user_xattr -o context=system_u:object_r:root_t:s0 /dev/mapper/testvg-scratchdev /mnt/scratch > > > > ext4/309 5s ... - output mismatch (see /root/git/xfstests-dev/results//ext4/309.out.bad) > > --- tests/ext4/309.out 2021-08-18 18:17:15.925427714 +0800 > > +++ /root/git/xfstests-dev/results//ext4/309.out.bad 2021-08-19 00:17:57.001648868 +0800 > > @@ -1,2 +1,4 @@ > > QA output created by 309 > > Silence is golden > > +/usr/sbin/tune2fs: Superblock checksum does not match superblock while trying to open /dev/mapper/testvg-scratchdev > > +Couldn't find valid filesystem superblock. > > ... > > (Run 'diff -u /root/git/xfstests-dev/tests/ext4/309.out /root/git/xfstests-dev/results//ext4/309.out.bad' to see the entire diff) > > Ran: ext4/309 > > Failures: ext4/309 > > Failed 1 of 1 tests > > > > [root@xx-xxxx-xx xfstests-dev]# git diff > > ... > > _scratch_mkfs >/dev/null 2>&1 > > _scratch_mount > > -echo "ext4/309" > $SCRATCH_MNT/309.tmp > > +echo "This is a test" > $SCRATCH_MNT/testfile > > _scratch_shutdown > > -_scratch_cycle_mount > > -$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 > > -if [ $? -eq 0 ]; then > > - status=0 > > -else > > - status=1 > > -fi > > +_scratch_cycle_mount > > +$TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null > > > > +status=0 > > exit > > Unless I missed something, I would say that, tune2fs' error output > went to .out.bad just because you had modified the tune2fs line from > mine: > > $TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 > > to yours: > > $TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null I think what Zorro is suggesting is that *when test fails* just dump stderr and let the error message break golden image to fail the test, so there's no need to set exit status. > > My original version redirecting stderr and stdout both to $seqres.full > doesn't break the "golden output". Test log: > ``` > [root@kvm102 repo_xfstests]# ./check ext4/309 > FSTYP -- ext4 > PLATFORM -- Linux/x86_64 kvm102 5.14.0-0.rc4.35.xx.x86_64 #1 SMP > Tue Aug 3 13:02:44 EDT 2021 > MKFS_OPTIONS -- -b 1024 /dev/vda3 > MOUNT_OPTIONS -- -o acl,user_xattr -o > context=system_u:object_r:root_t:s0 /dev/vda3 /scratch > > ext4/309 [failed, exit status 1] And this failure doesn't give any clues why it failed, you must check $seqres.full file to find out. With stderr breaking golden image, you'll see what fails more easily, e.g. QA output created by 309 Silence is golden +/usr/sbin/tune2fs: Superblock checksum does not match superblock while trying to open /dev/mapper/testvg-scratchdev +Couldn't find valid filesystem superblock. ... (Run 'diff -u /root/git/xfstests-dev/tests/ext4/309.out /root/git/xfstests-dev/results//ext4/309.out.bad' to see the entire diff) Thanks, Eryu
On Thu, Aug 19, 2021 at 01:48:55PM +0800, Boyang Xue wrote: > Zorro, > > Please check my reply inline below. > > On Thu, Aug 19, 2021 at 12:47 AM Zorro Lang <zlang@redhat.com> wrote: > > > > On Wed, Aug 18, 2021 at 09:20:44PM +0800, Boyang Xue wrote: > > > Hi Zorro, > > > > > > On Wed, Aug 18, 2021 at 7:32 PM Zorro Lang <zlang@redhat.com> wrote: > > > > > > > > On Wed, Aug 18, 2021 at 04:40:56PM +0800, bxue@redhat.com wrote: > > > > > From: Boyang Xue <bxue@redhat.com> > > > > > > > > > > Regression test for: > > > > > > > > > > ext4: Fix tune2fs checksum failure for mounted filesystem > > > > > > > > Better to specify the commit id number. I saw Ted has applied that patch: > > > > > > > > https://lore.kernel.org/linux-ext4/162895105421.460437.8931255765382647790.b4-ty@mit.edu/ > > > > > > Thanks. I see the commit id e905fbe3fd0fdb90052f6efdf88f50a78833cfe7 > > > in the above URL. I didn't add it since I'm not sure if this id will > > > be the final id when the commit is finally merged to the mainline > > > kernel (Linus tree)? > > > > > > > > > > > And maybe you can describe *a little* more in commit log. > > > > > > Yes I can add a few words in the commit log, but actually I expect the > > > reader of this test reads the commit message of the mentioned commit > > > "ext4: Fix tune2fs checksum failure for mounted filesystem", which I > > > think is more precise. > > > > > > > > > > > > > > > > > Signed-off-by: Boyang Xue <bxue@redhat.com> > > > > > --- > > > > > Hi, > > > > > > > > > > This is a new regression test for the patch > > > > > > > > > > ``` > > > > > ext4: Fix tune2fs checksum failure for mounted filesystem > > > > > > > > > > Commit 81414b4dd48 ("ext4: remove redundant sb checksum recomputation") > > > > > removed checksum recalculation after updating superblock free space / > > > > > inode counters in ext4_fill_super() based on the fact that we will > > > > > recalculate the checksum on superblock writeout. That is correct > > > > > assumption but until the writeout happens (which can take a long time) > > > > > the checksum is incorrect in the buffer cache and if tune2fs is called > > > > > in that time window it will complain. So return back the checksum > > > > > recalculation and add a comment explaining the tune2fs peculiarity. > > > > > > > > > > Fixes: 81414b4dd48f ("ext4: remove redundant sb checksum recomputation") > > > > > Reported-by: Boyang Xue <bxue@xxxxxxxxxx> > > > > > Signed-off-by: Jan Kara <jack@xxxxxxx> > > > > > ``` > > > > > > > > > > It's expected to fail on kernels from the kernel-5.11-rc1 to the latest > > > > > version, where tune2fs fails with: > > > > > > > > > > ``` > > > > > tune2fs 1.46.2 (28-Feb-2021) > > > > > tune2fs: Superblock checksum does not match superblock while trying to > > > > > open /dev/loop0 > > > > > Couldn't find valid filesystem superblock. > > > > > ``` > > > > > > > > > > Please help review this test, Thanks! > > > > > > > > > > -Boyang > > > > > > > > > > tests/ext4/309 | 42 ++++++++++++++++++++++++++++++++++++++++++ > > > > > tests/ext4/309.out | 2 ++ > > > > > 2 files changed, 44 insertions(+) > > > > > create mode 100755 tests/ext4/309 > > > > > create mode 100644 tests/ext4/309.out > > > > > > > > > > diff --git a/tests/ext4/309 b/tests/ext4/309 > > > > > new file mode 100755 > > > > > index 00000000..ae335617 > > > > > --- /dev/null > > > > > +++ b/tests/ext4/309 > > > > > @@ -0,0 +1,42 @@ > > > > > +#! /bin/bash > > > > > +# SPDX-License-Identifier: GPL-2.0 > > > > > +# Copyright (c) 2021 YOUR NAME HERE. All Rights Reserved. > > > > ^^^^^^^^^^^^^^ > > > > Write your copyright > > > > > > I will correct it in the next version. Thanks. > > > > > > > > > > > > +# > > > > > +# FS QA Test 309 > > > > > +# > > > > > +# Test that tune2fs doesn't fail after ext4 shutdown > > > > > +# Regression test for commit: > > > > > +# ext4: Fix tune2fs checksum failure for mounted filesystem > > > > > +# > > > > > +. ./common/preamble > > > > > +_begin_fstest auto rw quick > > > > > + > > > > > +_cleanup() > > > > > +{ > > > > > + _scratch_unmount > > > > > +} > > > > > > > > I think the umount isn't necessary, so the specific _cleanup isn't > > > > needed either. > > > > > > The $SCRATCH_DEV was still mounted before this _cleanup(), so I'm > > > wondering why we shouldn't do _scratch_unmount here? And I see at > > > least another similar structured test ext4/306 do _scratch_unmount in > > > _cleanup(). > > > > The SCRATCH_DEV will be umounted, don't need to do that in the end of each > > test cases, except you need to do something on the unmounted SCRATCH_DEV > > in the end. > > > > I don't know why ext4/306 has that, maybe due to old reason, or we didn't > > notice/care that when we reviewed it? And it's not worth sending a patch > > just for removing this "not wrong but just redundant" line now. Except a > > patch trys to cleanup all _cleanup(). > > OK. I will remove the _cleanup() in my next version, unless someone > else has other opinions. > > > > > > > > > > > > > > > + > > > > > +# Import common functions. > > > > > +. ./common/filter > > > > > > > > Do you use any filter helpers below? > > > > > > No. I will remove this line in my next version. > > > > > > > > > > > > + > > > > > +# real QA test starts here > > > > > +_supported_fs ext4 > > > > > > > > I'm wondering if this case can be a generic case, there's nothing > > > > ext4 specified operations, except this line: > > > > > > > > "$TUNE2FS_PROG -l $SCRATCH_DEV" > > > > > > > > Hmm... if we can change this line to something likes _get_fs_super(), > > > > it might help to make this test to be a generic test. > > > > > > I think this bug is heavily related to "tune2fs", ext4 only. So I > > > guess an ext4 only test is enough? > > > > Just checking. That's fine for me to keep this case as an ext4 only case. > > > > > > > > > > > > > > +_require_scratch > > > > > +_require_scratch_shutdown > > > > > +_require_command "$TUNE2FS_PROG" tune2fs > > > > > + > > > > > +echo "Silence is golden" > > > > > + > > > > > +_scratch_mkfs >/dev/null 2>&1 > > > > > +_scratch_mount > > > > > +echo "ext4/309" > $SCRATCH_MNT/309.tmp > > > > > > > > It's sure this case will be "ext4/309", although you use "309" won't > > > > affect anything. > > > > > > Yes I can rename it to something like ext4-309.tmp if it looks better. > > > > I think something likes: echo "This is a test" > $SCRATCH_MNT/testfile > > is good enough, don't need the "ext4" or "309" things. > > OK. I will modify it to > > echo "This is a test" > $SCRATCH_MNT/testfile > > Unless someone else has other preferences. > > > > > > > > > > > > > > > +_scratch_shutdown > > > > > +_scratch_cycle_mount > > > > > +$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 > > > > > +if [ $? -eq 0 ]; then > > > > > + status=0 > > > > > +else > > > > > + status=1 > > > > > +fi > > > > > > > > Don't need to change the status value, how about write as: > > > > > > > > $TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null > > > > > > > > The error output will break the golden image directly. > > > > > > How did you test that? The error output didn't break the "golden > > > image" in my test. > > > > [root@xx-xxxx-xx xfstests-dev]# ./check ext4/309 > > FSTYP -- ext4 > > PLATFORM -- Linux/x86_64 xx-xxxx-xx 5.14.0-rc4-xfs #14 SMP Thu Aug 12 00:56:07 CST 2021 > > MKFS_OPTIONS -- /dev/mapper/testvg-scratchdev > > MOUNT_OPTIONS -- -o acl,user_xattr -o context=system_u:object_r:root_t:s0 /dev/mapper/testvg-scratchdev /mnt/scratch > > > > ext4/309 5s ... - output mismatch (see /root/git/xfstests-dev/results//ext4/309.out.bad) > > --- tests/ext4/309.out 2021-08-18 18:17:15.925427714 +0800 > > +++ /root/git/xfstests-dev/results//ext4/309.out.bad 2021-08-19 00:17:57.001648868 +0800 > > @@ -1,2 +1,4 @@ > > QA output created by 309 > > Silence is golden > > +/usr/sbin/tune2fs: Superblock checksum does not match superblock while trying to open /dev/mapper/testvg-scratchdev > > +Couldn't find valid filesystem superblock. > > ... > > (Run 'diff -u /root/git/xfstests-dev/tests/ext4/309.out /root/git/xfstests-dev/results//ext4/309.out.bad' to see the entire diff) > > Ran: ext4/309 > > Failures: ext4/309 > > Failed 1 of 1 tests > > > > [root@xx-xxxx-xx xfstests-dev]# git diff > > ... > > _scratch_mkfs >/dev/null 2>&1 > > _scratch_mount > > -echo "ext4/309" > $SCRATCH_MNT/309.tmp > > +echo "This is a test" > $SCRATCH_MNT/testfile > > _scratch_shutdown > > -_scratch_cycle_mount > > -$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 > > -if [ $? -eq 0 ]; then > > - status=0 > > -else > > - status=1 > > -fi > > +_scratch_cycle_mount > > +$TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null > > > > +status=0 > > exit > > Unless I missed something, I would say that, tune2fs' error output > went to .out.bad just because you had modified the tune2fs line from > mine: > > $TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 > > to yours: > > $TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null Yes, I've metioned that in my first reply above. > > My original version redirecting stderr and stdout both to $seqres.full > doesn't break the "golden output". Test log: > ``` > [root@kvm102 repo_xfstests]# ./check ext4/309 > FSTYP -- ext4 > PLATFORM -- Linux/x86_64 kvm102 5.14.0-0.rc4.35.xx.x86_64 #1 SMP > Tue Aug 3 13:02:44 EDT 2021 > MKFS_OPTIONS -- -b 1024 /dev/vda3 > MOUNT_OPTIONS -- -o acl,user_xattr -o > context=system_u:object_r:root_t:s0 /dev/vda3 /scratch > > ext4/309 [failed, exit status 1] > Ran: ext4/309 > Failures: ext4/309 > Failed 1 of 1 tests > [root@kvm102 repo_xfstests]# cat results/ext4/309.full > /usr/sbin/tune2fs: Superblock checksum does not match superblock while > trying to open /dev/vda3 > Couldn't find valid filesystem superblock. > tune2fs 1.46.2 (28-Feb-2021) > [root@kvm102 repo_xfstests]# ls results/ext4/309.out.bad > ls: cannot access 'results/ext4/309.out.bad': No such file or directory > ``` > > As far as I can understand, there're at least two approaches to mark a > test pass or fail in xfstests: > 1) Compare the output of the test with the "golden output" (i.e. > 309.out), I guess this is the approach you mentioned. > 2) Exit the test with the value of $status (i.e. status=0 - pass, > status=non-zero - fail) > > I'm using the 2) here, not using 1) with the output of tune2fs > compared with the 309.out, because the output of tune2fs can be > different in different runs. An example output of a successful tune2fs > run is like (sorry for this long paste): > ``` You can [1] $TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full status=0 exit Or: [2] $TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 if [ $? -ne 0 ];then echo "Fail to get superblock from SCRATCH_DEV" fi status=0 exit Or: [3] $TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 || exit status=0 exit Or: [4] $TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 && status=0 exit I perfer the 1st one, most xfstests cases fail by breaking golden image. I just showed my review points, anyway I don't mind if maintainer would like to choose anyone else :) Thanks, Zorro > [root@kvm101 repo_xfstests]# cat results/ext4/309.full > tune2fs 1.45.6 (20-Mar-2020) > Filesystem volume name: <none> > Last mounted on: /scratch > Filesystem UUID: 22975090-96d7-49ee-9b4f-ee6afe046219 > Filesystem magic number: 0xEF53 > Filesystem revision #: 1 (dynamic) > Filesystem features: has_journal ext_attr resize_inode dir_index > filetype needs_recovery extent 64bit flex_bg sparse_super large_file > huge_file dir_nlink extra_isize metadata_csum > Filesystem flags: signed_directory_hash > Default mount options: user_xattr acl > Filesystem state: clean > Errors behavior: Continue > Filesystem OS type: Linux > Inode count: 720896 > Block count: 11534336 > Reserved block count: 576716 > Free blocks: 11280570 > Free inodes: 720884 > First block: 1 > Block size: 1024 > Fragment size: 1024 > Group descriptor size: 64 > Reserved GDT blocks: 256 > Blocks per group: 8192 > Fragments per group: 8192 > Inodes per group: 512 > Inode blocks per group: 128 > Flex block group size: 16 > Filesystem created: Thu Aug 19 05:45:26 2021 > Last mount time: Thu Aug 19 05:45:26 2021 > Last write time: Thu Aug 19 05:45:26 2021 > Mount count: 2 > Maximum mount count: -1 > Last checked: Thu Aug 19 05:45:26 2021 > Check interval: 0 (<none>) > Lifetime writes: 462 kB > Reserved blocks uid: 0 (user root) > Reserved blocks gid: 0 (group root) > First inode: 11 > Inode size: 256 > Required extra isize: 32 > Desired extra isize: 32 > Journal inode: 8 > Default directory hash: half_md4 > Directory Hash Seed: f5af5862-d415-460b-9a6b-97584578600f > Journal backup: inode blocks > Checksum type: crc32c > Checksum: 0x0f5b9c13 > ``` > > Thanks, > Boyang > > > > > > > Thanks, > > Zorro > > > > > > > > > > > > > ( cc ext4 mailist, to get more review) > > > > > > > > Thanks, > > > > Zorro > > > > > > Thanks for review! > > > > > > -Boyang > > > > > > > > > > > > + > > > > > +exit > > > > > diff --git a/tests/ext4/309.out b/tests/ext4/309.out > > > > > new file mode 100644 > > > > > index 00000000..56330d65 > > > > > --- /dev/null > > > > > +++ b/tests/ext4/309.out > > > > > @@ -0,0 +1,2 @@ > > > > > +QA output created by 309 > > > > > +Silence is golden > > > > > -- > > > > > 2.27.0 > > > > > > > > > > > > > > >
diff --git a/tests/ext4/309 b/tests/ext4/309 new file mode 100755 index 00000000..ae335617 --- /dev/null +++ b/tests/ext4/309 @@ -0,0 +1,42 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2021 YOUR NAME HERE. All Rights Reserved. +# +# FS QA Test 309 +# +# Test that tune2fs doesn't fail after ext4 shutdown +# Regression test for commit: +# ext4: Fix tune2fs checksum failure for mounted filesystem +# +. ./common/preamble +_begin_fstest auto rw quick + +_cleanup() +{ + _scratch_unmount +} + +# Import common functions. +. ./common/filter + +# real QA test starts here +_supported_fs ext4 +_require_scratch +_require_scratch_shutdown +_require_command "$TUNE2FS_PROG" tune2fs + +echo "Silence is golden" + +_scratch_mkfs >/dev/null 2>&1 +_scratch_mount +echo "ext4/309" > $SCRATCH_MNT/309.tmp +_scratch_shutdown +_scratch_cycle_mount +$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 +if [ $? -eq 0 ]; then + status=0 +else + status=1 +fi + +exit diff --git a/tests/ext4/309.out b/tests/ext4/309.out new file mode 100644 index 00000000..56330d65 --- /dev/null +++ b/tests/ext4/309.out @@ -0,0 +1,2 @@ +QA output created by 309 +Silence is golden