On Wed, Aug 18, 2021 at 04:40:56PM +0800, bxue@xxxxxxxxxx wrote: > From: Boyang Xue <bxue@xxxxxxxxxx> > > 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@xxxxxxx/ And maybe you can describe *a little* more in commit log. > > Signed-off-by: Boyang Xue <bxue@xxxxxxxxxx> > --- > 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 >
