On Fri, Oct 27, 2023 at 4:14 PM Anand Jain <anand.jain@xxxxxxxxxx> wrote:
>
> This test case originally checked for failed cloned device mounts, which
> is no longer relevant after the commit a5b8a5f9f835 ("btrfs: support
> cloned-device mount capability"). So removing the obsolete part.
>
> Additionally, add this test case back to the auto group which reverts the
> commit e2e7b549380a ("fstests: btrfs/219: remove it from auto group") since
> the previously missing kernel commit 5f58d783fd78 ("btrfs: free device in
> btrfs_close_devices for a single device filesystem") has already been
> integrated.
>
> Add cleanups of the local variables and the _cleanup() function.
>
> Reported-by: kernel test robot <oliver.sang@xxxxxxxxx>
> Closes: https://lore.kernel.org/oe-lkp/202310251645.5fe5495a-oliver.sang@xxxxxxxxx
> Signed-off-by: Anand Jain <anand.jain@xxxxxxxxxx>
> ---
> v2: Restores the code where it tests clone-devices.
>
> tests/btrfs/219 | 79 ++++++++++++++++++++++++++++++-------------------
> 1 file changed, 49 insertions(+), 30 deletions(-)
>
> diff --git a/tests/btrfs/219 b/tests/btrfs/219
> index b747ce34fcc4..eb3e0487aa74 100755
> --- a/tests/btrfs/219
> +++ b/tests/btrfs/219
> @@ -12,21 +12,27 @@
> #
>
> . ./common/preamble
> -_begin_fstest quick volume
> +_begin_fstest auto quick volume
>
> # Override the default cleanup function.
> _cleanup()
> {
> cd /
> rm -f $tmp.*
> - if [ ! -z "$loop_mnt" ]; then
> - $UMOUNT_PROG $loop_mnt
> - rm -rf $loop_mnt
> - fi
> - [ ! -z "$loop_mnt1" ] && rm -rf $loop_mnt1
> - [ ! -z "$fs_img1" ] && rm -rf $fs_img1
> - [ ! -z "$fs_img2" ] && rm -rf $fs_img2
> - [ ! -z "$loop_dev" ] && _destroy_loop_device $loop_dev
> +
> + # The variables are set before the test case can fail.
What if the _require_* statements fail?
Then the variables won't be defined...
> + $UMOUNT_PROG ${loop_mnt1} &> /dev/null
> + $UMOUNT_PROG ${loop_mnt2} &> /dev/null
> +
> + _destroy_loop_device $loop_dev1 &> /dev/null
> + _destroy_loop_device $loop_dev2 &> /dev/null
> +
> + rm -rf $fs_img1 &> /dev/null
> + rm -rf $fs_img2 &> /dev/null
> +
> + rm -rf $loop_mnt1 &> /dev/null
> + rm -rf $loop_mnt2 &> /dev/null
Also please for simplicity and clarity don't mix this type of change
with the actual purpose of the patch,
to make the test succeed on a kernel with the temp-fsid feature.
You're mixing 3 different changes in the same patch...
> +
> _btrfs_rescan_devices
> }
>
> @@ -38,55 +44,68 @@ _cleanup()
> _supported_fs btrfs
> _require_test
> _require_loop
> +_require_btrfs_fs_sysfs
> _require_btrfs_forget_or_module_loadable
> _fixed_by_kernel_commit 5f58d783fd78 \
> "btrfs: free device in btrfs_close_devices for a single device filesystem"
>
> -loop_mnt=$TEST_DIR/$seq.mnt
> -loop_mnt1=$TEST_DIR/$seq.mnt1
> -fs_img1=$TEST_DIR/$seq.img1
> -fs_img2=$TEST_DIR/$seq.img2
> +loop_mnt1=$TEST_DIR/$seq/mnt1
> +loop_mnt2=$TEST_DIR/$seq/mnt2
> +fs_img1=$TEST_DIR/$seq/img1
> +fs_img2=$TEST_DIR/$seq/img2
So this is the other unrelated change, renaming all these variables...
This is making the diff larger to follow as this has nothing to do
with the goal of making the test succeed on a kernel with the
temp-fsid feature.
>
> -mkdir $loop_mnt
> -mkdir $loop_mnt1
> +mkdir -p $loop_mnt1
> +mkdir -p $loop_mnt2
>
> $XFS_IO_PROG -f -c "truncate 256m" $fs_img1 >>$seqres.full 2>&1
>
> _mkfs_dev $fs_img1 >>$seqres.full 2>&1
> cp $fs_img1 $fs_img2
>
> +loop_dev1=`_create_loop_device $fs_img1`
> +loop_dev2=`_create_loop_device $fs_img2`
> +
> # Normal single device case, should pass just fine
> -_mount -o loop $fs_img1 $loop_mnt > /dev/null 2>&1 || \
> +_mount $loop_dev1 $loop_mnt1 > /dev/null 2>&1 || \
> _fail "Couldn't do initial mount"
> -$UMOUNT_PROG $loop_mnt
> +$UMOUNT_PROG $loop_mnt1
>
> _btrfs_forget_or_module_reload
>
> # Now mount the new version again to get the higher generation cached, umount
> # and try to mount the old version. Mount the new version again just for good
> # measure.
> -loop_dev=`_create_loop_device $fs_img1`
> -
> -_mount $loop_dev $loop_mnt > /dev/null 2>&1 || \
> +_mount $loop_dev1 $loop_mnt1 > /dev/null 2>&1 || \
> _fail "Failed to mount the second time"
> -$UMOUNT_PROG $loop_mnt
> +$UMOUNT_PROG $loop_mnt1
>
> -_mount -o loop $fs_img2 $loop_mnt > /dev/null 2>&1 || \
> +_mount $loop_dev2 $loop_mnt2 > /dev/null 2>&1 || \
> _fail "We couldn't mount the old generation"
> -$UMOUNT_PROG $loop_mnt
> +$UMOUNT_PROG $loop_mnt2
>
> -_mount $loop_dev $loop_mnt > /dev/null 2>&1 || \
> +_mount $loop_dev1 $loop_mnt1 > /dev/null 2>&1 || \
> _fail "Failed to mount the second time"
> -$UMOUNT_PROG $loop_mnt
> +$UMOUNT_PROG $loop_mnt1
>
> -# Now we definitely can't mount them at the same time, because we're still tied
> -# to the limitation of one fs_devices per fsid.
> +# Now try mount them at the same time, if kernel does not support
> +# temp-fsid feature then mount should fail.
> _btrfs_forget_or_module_reload
>
> -_mount $loop_dev $loop_mnt > /dev/null 2>&1 || \
> +_mount $loop_dev1 $loop_mnt1 > /dev/null 2>&1 || \
> _fail "Failed to mount the third time"
> -_mount -o loop $fs_img2 $loop_mnt1 > /dev/null 2>&1 && \
> - _fail "We were allowed to mount when we should have failed"
> +_mount $loop_dev2 $loop_mnt2 > /dev/null 2>&1
> +
> +if [ $? == 0 ]; then
> + # On temp-fsid supported kernels, mounting cloned device will be successfull.
> + if _has_fs_sysfs_attr $loop_dev2 temp_fsid; then
> + temp_fsid=$(_get_fs_sysfs_attr ${loop_dev2} temp_fsid)
> + if [ $temp_fsid == 0 ]; then
> + _fail "Cloned devices mounted without temp_fsid"
> + fi
This is too complex. Why not just surround the existing code in an if
statement like this:
if "sysfs-file-for-temp-fsid does not exist" then
run this code that fails with a temp-fsid enabled kernel
fi
Thanks.
> + else
> + _fail "We were allowed to mount when we should have failed"
> + fi
> +fi
>
> _btrfs_rescan_devices
> # success, all done
> --
> 2.39.3
>
