Hi Zorro,

On Wed, Aug 18, 2021 at 7:32 PM Zorro Lang [off-list ref] wrote:

On Wed, Aug 18, 2021 at 04:40:56PM +0800, bxue@redhat.com wrote:

quoted

From: Boyang Xue <redacted>

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/ (local)

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.

quoted

Signed-off-by: Boyang Xue <redacted>
---
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.

quoted

+#
+# 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().

quoted

+
+# Import common functions.
+. ./common/filter

Do you use any filter helpers below?

No. I will remove this line in my next version.

quoted

+
+# 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?

quoted

+_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.

quoted

+_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

quoted

+
+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