Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 18 Aug 2021 19:45:17 +0800
From:   Zorro Lang <zlang@redhat.com>
To:     bxue@redhat.com
Cc:     fstests@vger.kernel.org, jack@suse.cz, linux-ext4@vger.kernel.org
Subject: Re: [PATCH] ext4: regression test for "tune2fs -l" after ext4
 shutdown
Message-ID: <20210818114517.kqvfzu2vd45vuhze@fedora>
Mail-Followup-To: bxue@redhat.com, fstests@vger.kernel.org, jack@suse.cz,
        linux-ext4@vger.kernel.org
References: <20210818084126.4167799-1-bxue@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210818084126.4167799-1-bxue@redhat.com>
Precedence: bulk

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
>