diff mbox series

[4/6,v3] btrfs/219: fix _cleanup() to successful release the loop-device

Message ID 3559a441f8dfb450881001b7f4cbf780d7fa178e.1698674332.git.anand.jain@oracle.com (mailing list archive)
State New, archived
Headers show
Series btrfs/219 cloned-device mount capability update | expand

Commit Message

Anand Jain Oct. 30, 2023, 2:15 p.m. UTC 
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@oracle.com>
---

v3: a split from the patch 5/6

 tests/btrfs/219 | 63 ++++++++++++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 27 deletions(-)

Comments

Filipe Manana Oct. 30, 2023, 4:29 p.m. UTC | #1 
On Mon, Oct 30, 2023 at 2:15 PM Anand Jain <anand.jain@oracle.com> 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@oracle.com>
> ---
>
> 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
>
Anand Jain Nov. 2, 2023, 11:28 a.m. UTC | #2 
v4:
Removed a patch:
  [PATCH 2/6 v3] common/rc: _destroy_loop_device confirm arg1 is set
Fixed patch 4/6 in v3. (Ref to the patch 3/5 in v4).
Added received RB.

v3:
Split changes into smaller discrete patches.
Add a helper function to check if the temp-fsid is supported.
Check for the second failure only when the temp-fsid is not supported.

v2:
Patch 1/2 has been added, which performs the cleanup of the local
variables and the _clean_up() function. Patch 2/2 in v2 restores
the code where it tests clone-device that it does not mount if the
temp-fsid feature is not present in the kernel.

Fixes btrfs/219 bug when temp-fsid is supported in the kernel.

Anand Jain (5):
  common/rc: _fs_sysfs_dname fetch fsid using btrfs tool
  common/btrfs: add helper _has_btrfs_sysfs_feature_attr
  btrfs/219: fix _cleanup() to successful release the loop-device
  btrfs/219: cloned-device mount capability update
  btrfs/219: add to the auto group

 common/btrfs    | 12 ++++++++
 common/rc       |  5 +++-
 tests/btrfs/219 | 74 ++++++++++++++++++++++++++++---------------------
 3 files changed, 59 insertions(+), 32 deletions(-)
diff mbox series

Patch

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
+
+	_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
+
 	_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