Received-SPF: pass (google.com: domain of linux-ext4+bounces-1421-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 29 Feb 2024 09:38:48 +0800
From: Zorro Lang <zlang@redhat.com>
To: Suraj Jitindar Singh <surajjs@amazon.com>
Cc: fstests@vger.kernel.org, sjitindarsingh@gmail.com,
	linux-ext4@vger.kernel.org
Subject: Re: [PATCH] ext4/060: Test marking last group as trimmed
Message-ID: <20240229013848.3kqkj6sytp7xs3oh@dell-per750-06-vm-08.rhts.eng.pek2.redhat.com>
References: <20240228211612.2602355-1-surajjs@amazon.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240228211612.2602355-1-surajjs@amazon.com>

On Wed, Feb 28, 2024 at 01:16:12PM -0800, Suraj Jitindar Singh wrote:
> Regression test for upstream commit added in v6.8-rc1:
> 7c784d624819a ext4: allow for the last group to be marked as trimmed
> 
> Which fixes bug introduced by upstream commit in v6.6-rc2:
> 45e4ab320c9b5 ext4: move setting of trimmed bit into ext4_try_to_trim_range()
> 
> Applicable to kernels 4.19..6.7:
> Kernel		Bug Introduced			Bug Fixed
> 		kver      - commit sha		kver      - commit sha
> 4.19		v4.19.296 - d61445f6a5c57	v4.19.307 - 5b6a7f323b533
> 5.4		v5.4.258  - 4db34feaf2977	v5.4.269  - a7edaf40fccae
> 5.10		v5.10.198 - c502b09d9befc	v5.10.210 - fa94912241835
> 5.15		v5.15.134 - a9d3bb58da959	v5.15.149 - cb904f5c71629
> 6.1		v6.1.56   - b4d5db1c77fac	v6.1.76   - 852b6b2a2f7b7
> 6.6		v6.6-rc2  - 45e4ab320c9b5	v6.6.15   - da9008da96404
> 6.7		v6.6-rc2  - 45e4ab320c9b5	v6.7.3    - 73986e8d2808c
> 
> Signed-off-by: Suraj Jitindar Singh <surajjs@amazon.com>
> ---
>  tests/ext4/060     | 62 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/ext4/060.out |  2 ++

Great, a new ext4 case :) CC ext4 list to get more review :)

>  2 files changed, 64 insertions(+)
>  create mode 100755 tests/ext4/060
>  create mode 100644 tests/ext4/060.out
> 
> diff --git a/tests/ext4/060 b/tests/ext4/060
> new file mode 100755
> index 00000000..cc5f3819
> --- /dev/null
> +++ b/tests/ext4/060
> @@ -0,0 +1,62 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# FS QA Test No. 309
> +#
> +# Regression test for upstream commit added in v6.8-rc1:
> +# 7c784d624819a ext4: allow for the last group to be marked as trimmed
> +#
> +# Which fixes bug introduced by upstream commit in v6.6-rc2:
> +# 45e4ab320c9b5 ext4: move setting of trimmed bit into ext4_try_to_trim_range()
> +#
> +# Applicable to kernels 4.19..6.7:
> +# Kernel	Bug Introduced			Bug Fixed
> +#		kver      - commit sha		kver      - commit sha
> +# 4.19		v4.19.296 - d61445f6a5c57	v4.19.307 - 5b6a7f323b533
> +# 5.4		v5.4.258  - 4db34feaf2977	v5.4.269  - a7edaf40fccae
> +# 5.10		v5.10.198 - c502b09d9befc	v5.10.210 - fa94912241835
> +# 5.15		v5.15.134 - a9d3bb58da959	v5.15.149 - cb904f5c71629
> +# 6.1		v6.1.56   - b4d5db1c77fac	v6.1.76   - 852b6b2a2f7b7
> +# 6.6		v6.6-rc2  - 45e4ab320c9b5	v6.6.15   - da9008da96404
> +# 6.7		v6.6-rc2  - 45e4ab320c9b5	v6.7.3    - 73986e8d2808c
> +
> +. ./common/preamble
> +_begin_fstest auto
> +
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +    _scratch_unmount

This cleanup is useless, the SCRATCH_DEV will be unmounted and checked by default,
so you can remove this specific _cleanup if you don't need other cleanup steps.

> +}
> +
> +# Import common functions.
> +. ./common/filter
> +
> +# real QA test starts here
> +_supported_fs ext4

fstrim isn't an ext4 specific operation. Can this case be a generic case?
And you can have:

[[ "$FSTYP" =~ ext[0-9]+ ]] && _fixed_by_kernel_commit 7c784d624819a \
	ext4: allow for the last group to be marked as trimmed

> +
> +_require_scratch
> +_require_fstrim
> +
> +# Make an ext4 fs where the last group has fewer blocks than blocks per group
> +blksz=$(_get_page_size)
> +blocks_per_group=8192
> +
> +$MKFS_EXT4_PROG -F -b $blksz -g $blocks_per_group $SCRATCH_DEV $(( blocks_per_group - 1 )) >>$seqres.full 2>&1

Can it be replaced by _scratch_mkfs_sized? e.g.

MKFS_OPTIONS="-b $blksz -g $blocks_per_group" _scratch_mkfs_sized $(( blocks_per_group - 1 )) >> $seqres.full 2>&1

If you don't mind the effection of $MKFS_OPTIONS outside, you can keep it:
MKFS_OPTIONS="-b $blksz -g $blocks_per_group $MKFS_OPTIONS" ....

And of course better to check if the mkfs fails or not:

... _scratch_mkfs_sized .... || _fail "......"

It also helps to make this case be generic.

> +_scratch_mount
> +
> +$FSTRIM_PROG -v $SCRATCH_MNT >>$seqres.full 2>&1

Due to _require_fstrim only checks [ -z "$FSTRIM_PROG" ], the fstrim might still not
be supported by the SCRATCH_DEV, so we can check at here:

$FSTRIM_PROG -v $SCRATCH_MNT >>$seqres.full 2>&1 || _notrun "FSTRIM not supported"

> +# If we have the fix commit then the above trim command should have marked the
> +# group as trimmed and subsequent trim operations shouldn't discard anything.
> +# If we don't have the fix commit then the group won't have been marked as
> +# trimmed and the below trim operation will discard more than 0.
> +bytes=$($FSTRIM_PROG -v $SCRATCH_MNT | tee -a $seqres.full | _filter_fstrim)
> +if [ $bytes -gt 0 ]; then
> +	status=1
> +	echo "Final group in filesystem not marked as trimmed after trimming entire fs."
> +else
> +	status=0
> +	echo "Final group in filesystem correctly marked as trimmed after trimming entire fs."
> +fi

How about simplify this part as:

  $FSTRIM_PROG -v $SCRATCH_MNT | _filter_scratch

It will get "SCRATCH_MNT: 0 B (0 bytes) trimmed" as golden image (in .out file).
So if it's "0", then the test will fail by the golden image broken.

> +
> +exit
> diff --git a/tests/ext4/060.out b/tests/ext4/060.out
> new file mode 100644
> index 00000000..f3457134
> --- /dev/null
> +++ b/tests/ext4/060.out
> @@ -0,0 +1,2 @@
> +QA output created by 060
> +Final group in filesystem correctly marked as trimmed after trimming entire fs.
> -- 
> 2.34.1
> 
>