Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 19 Feb 2022 12:52:11 +0530
From:   Ritesh Harjani <riteshh@linux.ibm.com>
To:     Ojaswin Mujoo <ojaswin@linux.ibm.com>
Cc:     fstests@vger.kernel.org, linux-ext4@vger.kernel.org
Subject: Re: [PATCH 2/2] ext4: Test to ensure resize with sparse_super2 is
 handled correctly
Message-ID: <20220219072211.lmp64ntxmrcz7sua@riteshh-domain>
References: <cover.1644217569.git.ojaswin@linux.ibm.com>
 <aead63bfa6cca5a8a1c8225075f48a29d06df1ae.1644217569.git.ojaswin@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <aead63bfa6cca5a8a1c8225075f48a29d06df1ae.1644217569.git.ojaswin@linux.ibm.com>
Precedence: bulk

On 22/02/07 01:55PM, Ojaswin Mujoo wrote:
> Kernel currently doesn't support resize of EXT4 mounted
> with sparse_super2 option enabled. Earlier, it used to leave the resize
> incomplete and the fs would be left in an inconsistent state, however commit
> b1489186cc83[1] fixed this to avoid the fs corruption by clearly returning
> -ENOTSUPP.
>
> Test to ensure that kernel handles resizing with sparse_super2 correctly. Run
> resize for multiple iterations because this leads to kernel crash due to
> fs corruption, which we want to detect.
>
> Related commit in mainline:
>
> [1] commit b1489186cc8391e0c1e342f9fbc3eedf6b944c61
>
> 	ext4: add check to prevent attempting to resize an fs with sparse_super2

Thanks for the patch. Few nits below.

>
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> ---
>  tests/ext4/056     | 102 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/ext4/056.out |   2 +
>  2 files changed, 104 insertions(+)
>  create mode 100755 tests/ext4/056
>  create mode 100644 tests/ext4/056.out
>
> diff --git a/tests/ext4/056 b/tests/ext4/056
> new file mode 100755
> index 00000000..9185621d
> --- /dev/null
> +++ b/tests/ext4/056
> @@ -0,0 +1,102 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 IBM. All Rights Reserved.
> +#
> +# We don't currently support resize of EXT4 filesystems mounted
> +# with sparse_super2 option enabled. Earlier, kernel used to leave the resize
> +# incomplete and the fs would be left into an incomplete state, however commit
> +# b1489186cc83 fixed this to avoid the fs corruption by clearly returning
> +# -ENOTSUPP.
> +#
> +# This test ensures that kernel handles resizing with sparse_super2 correctly
> +#
> +# Related commit in mainline:
> +#
> +# commit b1489186cc8391e0c1e342f9fbc3eedf6b944c61
> +# ext4: add check to prevent attempting to resize an fs with sparse_super2
> +#
> +
> +. ./common/preamble
> +_begin_fstest auto ioctl resize quick
> +
> +# real QA test starts here
> +
> +INITIAL_FS_SIZE=1G
> +RESIZED_FS_SIZE=$((2*1024*1024*1024))  # 2G
> +ONLINE_RESIZE_BLOCK_LIMIT=$((256*1024*1024))
> +
> +_supported_fs ext4
> +_require_scratch_size $(($RESIZED_FS_SIZE/1024))
> +_require_test_program "ext4_resize"
> +
> +_log()
> +{
> +	echo "$seq: $*" >> $seqres.full 2>&1
> +}
> +
> +do_resize()
> +{
> +
> +	$MKFS_PROG `_scratch_mkfs_options -t ext4 -E resize=$ONLINE_RESIZE_BLOCK_LIMIT \
> +		-O sparse_super2` $INITIAL_FS_SIZE >> $seqres.full 2>&1 \
> +		|| _fail "$MKFS_PROG failed. Exiting"
> +
> +	_scratch_mount || _fail "Failed to mount scratch partition. Exiting"
> +
> +	BS=$(_get_block_size $SCRATCH_MNT)
> +	NEW_BLOCKS=$(($RESIZED_FS_SIZE/$BS))
> +
> +	local RESIZE_RET
> +	local EOPNOTSUPP=95
> +
> +	$here/src/ext4_resize $SCRATCH_MNT $NEW_BLOCKS >> $seqres.full 2>&1
> +	RESIZE_RET=$?
> +
> +	# Use $RESIZE_RET for logging
> +	if [ $RESIZE_RET = 0 ]
> +	then
> +		_log "Resizing succeeded but FS might still be corrupt."

IMO, this above logs is not required. Putting iteration count is more than
enough. You could log more info if the resize fails. But above log is somewhat
confusing to me.


> +	elif [ $RESIZE_RET = $EOPNOTSUPP ]
> +	then
> +		_log "Resize operation not supported with sparse_super2"
> +		_log "Threw expected error $RESIZE_RET (EOPNOTSUPP)"

If it fails with EOPNOTSUPP, do we still need to iterate in do_resize()
8 times?

> +
> +	else
> +		_log "Output of resize = $RESIZE_RET. Expected $EOPNOTSUPP (EOPNOTSUPP)"
> +		_log "You might be missing kernel patch b1489186cc83"

Not sure if we should pin point to a particular patch in this case.
It could be that we add some features later and then some of those doesn't again
support resize feature where it should return EOPNOTSUPP, but this test could
capture that. So, I feel above may not be required.

> +	fi
> +
> +	# unount after ioctl sometimes fails with "device busy" so add a small delay
> +	sleep 0.1

Let's not add this sleep for EOPNOTSUPP case.

> +
> +	_scratch_unmount >> $seqres.full 2>&1 || _fail "$UMOUNT_PROG failed. Exiting"
> +}
> +
> +run_test()
> +{
> +	local FSCK_RET
> +	local ITERS=8
> +
> +	for i in $(seq 1 $ITERS)
> +	do
> +		_log "----------- Iteration: $i ------------"
> +		do_resize
> +	done
> +
> +	_log "-------- Iterations Complete ---------"
> +	_log "Checking if FS is in consistent state"
> +	_check_scratch_fs
> +	FSCK_RET=$?
> +
> +	return $FSCK_RET
> +}
> +
> +run_test
> +status=$?
> +
> +if [ "$status" -eq "0" ]
> +then
> +	echo "Test Succeeded!" | tee -a $seqres.full
> +fi
> +
> +exit
> diff --git a/tests/ext4/056.out b/tests/ext4/056.out
> new file mode 100644
> index 00000000..41706284
> --- /dev/null
> +++ b/tests/ext4/056.out
> @@ -0,0 +1,2 @@
> +QA output created by 056
> +Test Succeeded!
> --
> 2.27.0
>