On Mon, Oct 30, 2023 at 2:15 PM Anand Jain <anand.jain@xxxxxxxxxx> wrote:
>
> When we fail with the message 'We were allowed to mount when we should
> have failed,' it will fail to clean up the loop devices, making it
> difficult to run further test cases or the same test case again.
>
> Before temp-fsid support, the last mount would fail, so there is no need
> to free the last 2nd loop device, and there is no local variable to
> release it. However, with temp-fsid, the mount shall be successful, so we
> need a 2nd loop device local variable to release it. Let's reorganize the
> local variables to clean them up in the _cleanup() function.
>
> Signed-off-by: Anand Jain <anand.jain@xxxxxxxxxx>
> ---
>
> v3: a split from the patch 5/6
>
> tests/btrfs/219 | 63 ++++++++++++++++++++++++++++---------------------
> 1 file changed, 36 insertions(+), 27 deletions(-)
>
> diff --git a/tests/btrfs/219 b/tests/btrfs/219
> index b747ce34fcc4..44a4c79dc05d 100755
> --- a/tests/btrfs/219
> +++ b/tests/btrfs/219
> @@ -19,14 +19,19 @@ _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.
> + $UMOUNT_PROG ${loop_mnt1} &> /dev/null
> + $UMOUNT_PROG ${loop_mnt2} &> /dev/null
> + rm -rf $loop_mnt1 &> /dev/null
> + rm -rf $loop_mnt2 &> /dev/null
No need for the redirection when calling rm with -f.
rm doesn't print anything to stdout or stderr if the target file/dir
does not exist.
> +
> + _destroy_loop_device $loop_dev1 &> /dev/null
> + _destroy_loop_device $loop_dev2 &> /dev/null
Rather than making _destroy_loop_device() ignore a missing device
argument, it's cleaner to
avoid calling it if $loop_dev1 and $loop_dev2 are not defined / are
empty strings, as commented before.
> +
> + rm -rf $fs_img1 &> /dev/null
> + rm -rf $fs_img2 &> /dev/null
Same here.
Thanks.
> +
> _btrfs_rescan_devices
> }
>
> @@ -36,56 +41,60 @@ _cleanup()
> # real QA test starts here
>
> _supported_fs btrfs
> +
> +loop_mnt1=$TEST_DIR/$seq/mnt1
> +loop_mnt2=$TEST_DIR/$seq/mnt2
> +fs_img1=$TEST_DIR/$seq/img1
> +fs_img2=$TEST_DIR/$seq/img2
> +loop_dev1=""
> +loop_dev2=""
> +
> _require_test
> _require_loop
> _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
> -
> -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.
> _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 && \
> +_mount $loop_dev2 $loop_mnt2 > /dev/null 2>&1 && \
> _fail "We were allowed to mount when we should have failed"
>
> _btrfs_rescan_devices
> --
> 2.39.3
>
