[PATCH 0/6 v3] btrfs/219 cloned-device mount capability update

[PATCH 0/6 v3] btrfs/219 cloned-device mount capability update

[Anand Jain]

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 (6):
  common/rc: _fs_sysfs_dname fetch fsid using btrfs tool
  common/rc: _destroy_loop_device confirm arg1 is set
  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       | 10 +++++--
 tests/btrfs/219 | 74 ++++++++++++++++++++++++++++---------------------
 3 files changed, 63 insertions(+), 33 deletions(-)

-- 
2.39.3

[PATCH 1/6 v3] common/rc: _fs_sysfs_dname fetch fsid using btrfs tool

[Anand Jain]

Currently _fs_sysfs_dname gets fsid from the findmnt command however
this command provides the metadata_uuid in the context device is
mounted with temp-fsid. So instead, use btrfs filesystem show command to
know the temp-fsid.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---

v3: add local variable fsid

 common/rc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index 259a1ffb09b9..18d2ddcf8e35 100644
--- a/common/rc
+++ b/common/rc
@@ -4721,6 +4721,7 @@ _require_statx()
 _fs_sysfs_dname()
 {
 	local dev=$1
+	local fsid
 
 	if [ ! -b "$dev" ]; then
 		_fail "Usage: _fs_sysfs_dname <mounted_device>"
@@ -4728,7 +4729,9 @@ _fs_sysfs_dname()
 
 	case "$FSTYP" in
 	btrfs)
-		findmnt -n -o UUID ${dev} ;;
+		fsid=$($BTRFS_UTIL_PROG filesystem show ${dev} | grep uuid: | \
+							awk '{print $NF}')
+		echo $fsid ;;
 	*)
 		_short_dev $dev ;;
 	esac
-- 
2.39.3

[PATCH 2/6 v3] common/rc: _destroy_loop_device confirm arg1 is set

[Anand Jain]

Check if the dev arg1 is set before calling losetup -d on it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 common/rc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index 18d2ddcf8e35..e7d6801b20e8 100644
--- a/common/rc
+++ b/common/rc
@@ -4150,7 +4150,10 @@ _create_loop_device()
 _destroy_loop_device()
 {
 	local dev=$1
-	losetup -d $dev || _fail "Cannot destroy loop device $dev"
+
+	if [ ! -z $dev ]; then
+		losetup -d $dev || _fail "Cannot destroy loop device $dev"
+	fi
 }
 
 _scale_fsstress_args()
-- 
2.39.3

[PATCH 3/6 v3] common/btrfs: add helper _has_btrfs_sysfs_feature_attr

[Anand Jain]

With this helper, btrfs test cases can now check if a particular feature
is implemented in the kernel.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 common/btrfs | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/common/btrfs b/common/btrfs
index c3bffd2ae3f7..fbc26181f7bc 100644
--- a/common/btrfs
+++ b/common/btrfs
@@ -753,3 +753,15 @@ _require_scratch_enable_simple_quota()
 					_notrun "simple quotas not available"
 	_scratch_unmount
 }
+
+_has_btrfs_sysfs_feature_attr()
+{
+	local feature_attr=$1
+
+	[ -z $feature_attr ] && \
+		_fail "Missing feature name argument for _has_btrfs_sysfs_attr"
+
+	modprobe btrfs &> /dev/null
+
+	test -e /sys/fs/btrfs/features/$feature_attr
+}
-- 
2.39.3

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

[Anand Jain]

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

[PATCH 5/6 v3] btrfs/219: cloned-device mount capability update

[Anand Jain]

This test case checks for failure of the cloned device mounts, which
is no longer true after the commit a5b8a5f9f835 ("btrfs: support
cloned-device mount capability"). So check for the non-presence the
temp-fsid feature and do not test for the failure of the cloned device
mount.

Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202310251645.5fe5495a-oliver.sang@intel.com
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---

v3: check only that mount of clone device fails if temp-fsid is not
supported.

 tests/btrfs/219 | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/tests/btrfs/219 b/tests/btrfs/219
index 44a4c79dc05d..3c414dd9c2e0 100755
--- a/tests/btrfs/219
+++ b/tests/btrfs/219
@@ -51,6 +51,7 @@ loop_dev2=""
 
 _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"
@@ -88,14 +89,16 @@ _mount $loop_dev1 $loop_mnt1 > /dev/null 2>&1 || \
 	_fail "Failed to mount the second time"
 $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 will fail.
 _btrfs_forget_or_module_reload
 
 _mount $loop_dev1 $loop_mnt1 > /dev/null 2>&1 || \
 	_fail "Failed to mount the third time"
-_mount $loop_dev2 $loop_mnt2 > /dev/null 2>&1 && \
-	_fail "We were allowed to mount when we should have failed"
+if ! _has_btrfs_sysfs_feature_attr temp_fsid; then
+	_mount $loop_dev2 $loop_mnt2 > /dev/null 2>&1 && \
+		_fail "We were allowed to mount when we should have failed"
+fi
 
 _btrfs_rescan_devices
 # success, all done
-- 
2.39.3

[PATCH 6/6 v3] btrfs/219: add to the auto group

[Anand Jain]

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.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---

v3: A split from the patch 5/6.

 tests/btrfs/219 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/btrfs/219 b/tests/btrfs/219
index 3c414dd9c2e0..ebebb75f4b1d 100755
--- a/tests/btrfs/219
+++ b/tests/btrfs/219
@@ -12,7 +12,7 @@
 #
 
 . ./common/preamble
-_begin_fstest quick volume
+_begin_fstest auto quick volume
 
 # Override the default cleanup function.
 _cleanup()
-- 
2.39.3

Re: [PATCH 1/6 v3] common/rc: _fs_sysfs_dname fetch fsid using btrfs tool

[Filipe Manana]

On Mon, Oct 30, 2023 at 2:15 PM Anand Jain <anand.jain@oracle.com> wrote:
>
> Currently _fs_sysfs_dname gets fsid from the findmnt command however
> this command provides the metadata_uuid in the context device is

in -> if

> mounted with temp-fsid. So instead, use btrfs filesystem show command to
> know the temp-fsid.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>
> v3: add local variable fsid
>
>  common/rc | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/common/rc b/common/rc
> index 259a1ffb09b9..18d2ddcf8e35 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -4721,6 +4721,7 @@ _require_statx()
>  _fs_sysfs_dname()
>  {
>         local dev=$1
> +       local fsid
>
>         if [ ! -b "$dev" ]; then
>                 _fail "Usage: _fs_sysfs_dname <mounted_device>"
> @@ -4728,7 +4729,9 @@ _fs_sysfs_dname()
>
>         case "$FSTYP" in
>         btrfs)
> -               findmnt -n -o UUID ${dev} ;;
> +               fsid=$($BTRFS_UTIL_PROG filesystem show ${dev} | grep uuid: | \
> +                                                       awk '{print $NF}')

awk -> $AWK_PROG

Besides that, it looks fine, thanks.

Reviewed-by: Filipe Manana <fdmanana@suse.com>

> +               echo $fsid ;;
>         *)
>                 _short_dev $dev ;;
>         esac
> --
> 2.39.3
>

Re: [PATCH 2/6 v3] common/rc: _destroy_loop_device confirm arg1 is set

[Filipe Manana]

On Mon, Oct 30, 2023 at 2:15 PM Anand Jain <anand.jain@oracle.com> wrote:
>
> Check if the dev arg1 is set before calling losetup -d on it.

Why?

Do we have any callers that call the function without an argument?
More comments below.

>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  common/rc | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/common/rc b/common/rc
> index 18d2ddcf8e35..e7d6801b20e8 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -4150,7 +4150,10 @@ _create_loop_device()
>  _destroy_loop_device()
>  {
>         local dev=$1
> -       losetup -d $dev || _fail "Cannot destroy loop device $dev"
> +
> +       if [ ! -z $dev ]; then
> +               losetup -d $dev || _fail "Cannot destroy loop device $dev"
> +       fi

So this is just ignoring if no argument is given and the function does nothing.
This is quite the opposite of everywhere else, where we error out if a
necessary argument is missing.

btrfs/219 never calls this function without the argument, both before
and after this patchset, so I don't see why this patch is needed.
If we have any callers not passing the argument, I'd rather fix them
and make the function error out if no argument is given.

Thanks.

>  }
>
>  _scale_fsstress_args()
> --
> 2.39.3
>

Re: [PATCH 3/6 v3] common/btrfs: add helper _has_btrfs_sysfs_feature_attr

[Filipe Manana]

On Mon, Oct 30, 2023 at 2:15 PM Anand Jain <anand.jain@oracle.com> wrote:
>
> With this helper, btrfs test cases can now check if a particular feature
> is implemented in the kernel.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  common/btrfs | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/common/btrfs b/common/btrfs
> index c3bffd2ae3f7..fbc26181f7bc 100644
> --- a/common/btrfs
> +++ b/common/btrfs
> @@ -753,3 +753,15 @@ _require_scratch_enable_simple_quota()
>                                         _notrun "simple quotas not available"
>         _scratch_unmount
>  }
> +
> +_has_btrfs_sysfs_feature_attr()
> +{
> +       local feature_attr=$1
> +
> +       [ -z $feature_attr ] && \
> +               _fail "Missing feature name argument for _has_btrfs_sysfs_attr"
> +
> +       modprobe btrfs &> /dev/null
> +
> +       test -e /sys/fs/btrfs/features/$feature_attr
> +}

We already have _require_btrfs_fs_feature() to do exactly this.
Tried it in patch 5/6 and it works perfectly...

Thanks.

> --
> 2.39.3
>

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

[Filipe Manana]

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
>

Re: [PATCH 5/6 v3] btrfs/219: cloned-device mount capability update

[Filipe Manana]

On Mon, Oct 30, 2023 at 2:15 PM Anand Jain <anand.jain@oracle.com> wrote:
>
> This test case checks for failure of the cloned device mounts, which
> is no longer true after the commit a5b8a5f9f835 ("btrfs: support
> cloned-device mount capability"). So check for the non-presence the
> temp-fsid feature and do not test for the failure of the cloned device
> mount.
>
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202310251645.5fe5495a-oliver.sang@intel.com
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>
> v3: check only that mount of clone device fails if temp-fsid is not
> supported.
>
>  tests/btrfs/219 | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/tests/btrfs/219 b/tests/btrfs/219
> index 44a4c79dc05d..3c414dd9c2e0 100755
> --- a/tests/btrfs/219
> +++ b/tests/btrfs/219
> @@ -51,6 +51,7 @@ loop_dev2=""
>
>  _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"
> @@ -88,14 +89,16 @@ _mount $loop_dev1 $loop_mnt1 > /dev/null 2>&1 || \
>         _fail "Failed to mount the second time"
>  $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 will fail.
>  _btrfs_forget_or_module_reload
>
>  _mount $loop_dev1 $loop_mnt1 > /dev/null 2>&1 || \
>         _fail "Failed to mount the third time"
> -_mount $loop_dev2 $loop_mnt2 > /dev/null 2>&1 && \
> -       _fail "We were allowed to mount when we should have failed"
> +if ! _has_btrfs_sysfs_feature_attr temp_fsid; then
> +       _mount $loop_dev2 $loop_mnt2 > /dev/null 2>&1 && \
> +               _fail "We were allowed to mount when we should have failed"
> +fi
>
>  _btrfs_rescan_devices
>  # success, all done
> --
> 2.39.3
>

Re: [PATCH 6/6 v3] btrfs/219: add to the auto group

[Filipe Manana]

On Mon, Oct 30, 2023 at 2:15 PM Anand Jain <anand.jain@oracle.com> wrote:
>
> 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.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>
> v3: A split from the patch 5/6.
>
>  tests/btrfs/219 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/btrfs/219 b/tests/btrfs/219
> index 3c414dd9c2e0..ebebb75f4b1d 100755
> --- a/tests/btrfs/219
> +++ b/tests/btrfs/219
> @@ -12,7 +12,7 @@
>  #
>
>  . ./common/preamble
> -_begin_fstest quick volume
> +_begin_fstest auto quick volume
>
>  # Override the default cleanup function.
>  _cleanup()
> --
> 2.39.3
>

Re: [PATCH 3/6 v3] common/btrfs: add helper _has_btrfs_sysfs_feature_attr

[Anand Jain]

>> +_has_btrfs_sysfs_feature_attr()
>> +{
>> +       local feature_attr=$1
>> +
>> +       [ -z $feature_attr ] && \
>> +               _fail "Missing feature name argument for _has_btrfs_sysfs_attr"
>> +
>> +       modprobe btrfs &> /dev/null
>> +
>> +       test -e /sys/fs/btrfs/features/$feature_attr
>> +}
> 
> We already have _require_btrfs_fs_feature() to do exactly this.
> Tried it in patch 5/6 and it works perfectly...

I noticed that. But, _require_btrfs_fs_feature() triggers _notrun()
when the feature is absent in the kernel. Given our need to run the
testcase with and without temp-fsid, it is not suitable.

Thanks, Anand

Re: [PATCH 2/6 v3] common/rc: _destroy_loop_device confirm arg1 is set

[Anand Jain]

On 10/31/23 00:21, Filipe Manana wrote:
> On Mon, Oct 30, 2023 at 2:15 PM Anand Jain <anand.jain@oracle.com> wrote:
>>
>> Check if the dev arg1 is set before calling losetup -d on it.
> 
> Why?
> 
> Do we have any callers that call the function without an argument?
> More comments below.
> 
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   common/rc | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/rc b/common/rc
>> index 18d2ddcf8e35..e7d6801b20e8 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -4150,7 +4150,10 @@ _create_loop_device()
>>   _destroy_loop_device()
>>   {
>>          local dev=$1
>> -       losetup -d $dev || _fail "Cannot destroy loop device $dev"
>> +
>> +       if [ ! -z $dev ]; then
>> +               losetup -d $dev || _fail "Cannot destroy loop device $dev"
>> +       fi
> 
> So this is just ignoring if no argument is given and the function does nothing.
> This is quite the opposite of everywhere else, where we error out if a
> necessary argument is missing.
> 

> btrfs/219 never calls this function without the argument,
> both before
> and after this patchset, so I don't see why this patch is needed.

You are right. We no longer need this patch. In version 2, we had the
last mount called with or without temp-fsid. That's being removed,
and initialization is taken care of, so we can drop this patch.
I'll drop this patch.

> If we have any callers not passing the argument, I'd rather fix them
> and make the function error out if no argument is given.

Thanks, Anand

Re: [PATCH 1/6 v3] common/rc: _fs_sysfs_dname fetch fsid using btrfs tool

[Anand Jain]

On 10/31/23 00:16, Filipe Manana wrote:
> On Mon, Oct 30, 2023 at 2:15 PM Anand Jain <anand.jain@oracle.com> wrote:
>>
>> Currently _fs_sysfs_dname gets fsid from the findmnt command however
>> this command provides the metadata_uuid in the context device is
> 
> in -> if
> 
>> mounted with temp-fsid. So instead, use btrfs filesystem show command to
>> know the temp-fsid.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>
>> v3: add local variable fsid
>>
>>   common/rc | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/rc b/common/rc
>> index 259a1ffb09b9..18d2ddcf8e35 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -4721,6 +4721,7 @@ _require_statx()
>>   _fs_sysfs_dname()
>>   {
>>          local dev=$1
>> +       local fsid
>>
>>          if [ ! -b "$dev" ]; then
>>                  _fail "Usage: _fs_sysfs_dname <mounted_device>"
>> @@ -4728,7 +4729,9 @@ _fs_sysfs_dname()
>>
>>          case "$FSTYP" in
>>          btrfs)
>> -               findmnt -n -o UUID ${dev} ;;
>> +               fsid=$($BTRFS_UTIL_PROG filesystem show ${dev} | grep uuid: | \
>> +                                                       awk '{print $NF}')
> 
> awk -> $AWK_PROG
> 
> Besides that, it looks fine, thanks.
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> 

Fixed locally. Thx.

Anand

>> +               echo $fsid ;;
>>          *)
>>                  _short_dev $dev ;;
>>          esac
>> --
>> 2.39.3
>>

[PATCH v4 3/5] btrfs/219: fix _cleanup() to successful release the loop-device

[Anand Jain]

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.

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

v4: rm -f, removed error/output redirection
    rm, -r removed for file image
    Check for the initialization of the local variable loop_dev[1-2]
     before calling  _destroy_loop_device().

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..35824df2baaa 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
+	rm -rf $loop_mnt2
+
+	[ ! -z $loop_dev1 ] && _destroy_loop_device $loop_dev1
+	[ ! -z $loop_dev1 ] && _destroy_loop_device $loop_dev2
+
+	rm -f $fs_img1
+	rm -f $fs_img2
+
 	_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.31.1

Re: [PATCH 3/6 v3] common/btrfs: add helper _has_btrfs_sysfs_feature_attr

[Filipe Manana]

On Mon, Oct 30, 2023 at 11:30 PM Anand Jain <anand.jain@oracle.com> wrote:
>
>
> >> +_has_btrfs_sysfs_feature_attr()
> >> +{
> >> +       local feature_attr=$1
> >> +
> >> +       [ -z $feature_attr ] && \
> >> +               _fail "Missing feature name argument for _has_btrfs_sysfs_attr"
> >> +
> >> +       modprobe btrfs &> /dev/null
> >> +
> >> +       test -e /sys/fs/btrfs/features/$feature_attr
> >> +}
> >
> > We already have _require_btrfs_fs_feature() to do exactly this.
> > Tried it in patch 5/6 and it works perfectly...
>
> I noticed that. But, _require_btrfs_fs_feature() triggers _notrun()
> when the feature is absent in the kernel. Given our need to run the
> testcase with and without temp-fsid, it is not suitable.

Yes, nevermind. It's fine then.

Reviewed-by: Filipe Manana <fdmanana@suse.com>

>
> Thanks, Anand
>

Re: [PATCH v4 3/5] btrfs/219: fix _cleanup() to successful release the loop-device

[Filipe Manana]

On Tue, Oct 31, 2023 at 12:54 AM 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.
>
> 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>
> ---
>
> v4: rm -f, removed error/output redirection
>     rm, -r removed for file image
>     Check for the initialization of the local variable loop_dev[1-2]
>      before calling  _destroy_loop_device().
>
> v3: a split from the patch 5/6

One patch has a v4, others remain as v3 and one from v3 is dropped. A
bit hard to follow, as the common practice is usually to send a new
version of the whole patchset.

Nevertheless, unless I missed something, it looks good now:

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.

>
>  tests/btrfs/219 | 63 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 36 insertions(+), 27 deletions(-)
>
> diff --git a/tests/btrfs/219 b/tests/btrfs/219
> index b747ce34fcc4..35824df2baaa 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
> +       rm -rf $loop_mnt2
> +
> +       [ ! -z $loop_dev1 ] && _destroy_loop_device $loop_dev1
> +       [ ! -z $loop_dev1 ] && _destroy_loop_device $loop_dev2
> +
> +       rm -f $fs_img1
> +       rm -f $fs_img2
> +
>         _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.31.1
>

Re: [PATCH v4 3/5] btrfs/219: fix _cleanup() to successful release the loop-device

[Anand Jain]

On 31/10/2023 19:41, Filipe Manana wrote:
> On Tue, Oct 31, 2023 at 12:54 AM 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.
>>
>> 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>
>> ---
>>
>> v4: rm -f, removed error/output redirection
>>      rm, -r removed for file image
>>      Check for the initialization of the local variable loop_dev[1-2]
>>       before calling  _destroy_loop_device().
>>
>> v3: a split from the patch 5/6
> 
> One patch has a v4, others remain as v3 and one from v3 is dropped. A
> bit hard to follow, as the common practice is usually to send a new
> version of the whole patchset.

Yeah, it should have been confusing. But I thought this would reduce
unnecessary noise. Also my '--in-reply-to=<message-id-patch-4/6-v3>'
option didn't work this.

> 
> Nevertheless, unless I missed something, it looks good now:
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks, Anand

Re: [PATCH v4 3/5] btrfs/219: fix _cleanup() to successful release the loop-device

[Zorro Lang]

On Tue, Oct 31, 2023 at 08:53:41AM +0800, Anand Jain 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.
> 
> 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>
> ---
> 
> v4: rm -f, removed error/output redirection
>     rm, -r removed for file image
>     Check for the initialization of the local variable loop_dev[1-2]
>      before calling  _destroy_loop_device().

This looks like a single change of the whole patchset below:
  [PATCH 0/6 v3] btrfs/219 cloned-device mount capability update

The v3 patchset has some review points, better to be changed too.
And the V3 has 6 patches in the patchset, now this patch names 3/5.
I'm a little confused. Could you send the whole v4 patchset? Of
course you can send them with the RVBs you've gotten.

Thanks,
Zorro

> 
> 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..35824df2baaa 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
> +	rm -rf $loop_mnt2
> +
> +	[ ! -z $loop_dev1 ] && _destroy_loop_device $loop_dev1
> +	[ ! -z $loop_dev1 ] && _destroy_loop_device $loop_dev2
> +
> +	rm -f $fs_img1
> +	rm -f $fs_img2
> +
>  	_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.31.1
> 
> 

[PATCH v4 0/5] btrfs/219 cloned-device mount capability update

[Anand Jain]

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(-)

-- 
2.31.1

[PATCH v4 1/5] common/rc: _fs_sysfs_dname fetch fsid using btrfs tool

[Anand Jain]

Currently _fs_sysfs_dname gets fsid from the findmnt command however
this command provides the metadata_uuid if the device is mounted with
temp-fsid. So instead, use btrfs filesystem show command to know the fsid.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
---
v3: add local variable fsid

 common/rc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index 259a1ffb09b9..7f14c19ca89e 100644
--- a/common/rc
+++ b/common/rc
@@ -4721,6 +4721,7 @@ _require_statx()
 _fs_sysfs_dname()
 {
 	local dev=$1
+	local fsid
 
 	if [ ! -b "$dev" ]; then
 		_fail "Usage: _fs_sysfs_dname <mounted_device>"
@@ -4728,7 +4729,9 @@ _fs_sysfs_dname()
 
 	case "$FSTYP" in
 	btrfs)
-		findmnt -n -o UUID ${dev} ;;
+		fsid=$($BTRFS_UTIL_PROG filesystem show ${dev} | grep uuid: | \
+							$AWK_PROG '{print $NF}')
+		echo $fsid ;;
 	*)
 		_short_dev $dev ;;
 	esac
-- 
2.31.1

[PATCH v4 2/5] common/btrfs: add helper _has_btrfs_sysfs_feature_attr

[Anand Jain]

With this helper, btrfs test cases can now check if a particular feature
is implemented in the kernel.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
---
 common/btrfs | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/common/btrfs b/common/btrfs
index c3bffd2ae3f7..fbc26181f7bc 100644
--- a/common/btrfs
+++ b/common/btrfs
@@ -753,3 +753,15 @@ _require_scratch_enable_simple_quota()
 					_notrun "simple quotas not available"
 	_scratch_unmount
 }
+
+_has_btrfs_sysfs_feature_attr()
+{
+	local feature_attr=$1
+
+	[ -z $feature_attr ] && \
+		_fail "Missing feature name argument for _has_btrfs_sysfs_attr"
+
+	modprobe btrfs &> /dev/null
+
+	test -e /sys/fs/btrfs/features/$feature_attr
+}
-- 
2.31.1

[PATCH v4 3/5] btrfs/219: fix _cleanup() to successful release the loop-device

[Anand Jain]

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.

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>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
---
v4: rm -f, removed error/output redirection
    rm, -r removed for file image
    Check for the initialization of the local variable loop_dev[1-2]
     before calling  _destroy_loop_device().

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..35824df2baaa 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
+	rm -rf $loop_mnt2
+
+	[ ! -z $loop_dev1 ] && _destroy_loop_device $loop_dev1
+	[ ! -z $loop_dev1 ] && _destroy_loop_device $loop_dev2
+
+	rm -f $fs_img1
+	rm -f $fs_img2
+
 	_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.31.1

[PATCH v4 4/5] btrfs/219: cloned-device mount capability update

[Anand Jain]

This test case checks for failure of the cloned device mounts, which
is no longer true after the commit a5b8a5f9f835 ("btrfs: support
cloned-device mount capability"). So check for the non-presence the
temp-fsid feature and do not test for the failure of the cloned device
mount.

Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202310251645.5fe5495a-oliver.sang@intel.com
Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
---
v3: check only that mount of clone device fails if temp-fsid is not
supported.

 tests/btrfs/219 | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/tests/btrfs/219 b/tests/btrfs/219
index 35824df2baaa..0bbfd6949cc8 100755
--- a/tests/btrfs/219
+++ b/tests/btrfs/219
@@ -51,6 +51,7 @@ loop_dev2=""
 
 _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"
@@ -88,14 +89,16 @@ _mount $loop_dev1 $loop_mnt1 > /dev/null 2>&1 || \
 	_fail "Failed to mount the second time"
 $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 will fail.
 _btrfs_forget_or_module_reload
 
 _mount $loop_dev1 $loop_mnt1 > /dev/null 2>&1 || \
 	_fail "Failed to mount the third time"
-_mount $loop_dev2 $loop_mnt2 > /dev/null 2>&1 && \
-	_fail "We were allowed to mount when we should have failed"
+if ! _has_btrfs_sysfs_feature_attr temp_fsid; then
+	_mount $loop_dev2 $loop_mnt2 > /dev/null 2>&1 && \
+		_fail "We were allowed to mount when we should have failed"
+fi
 
 _btrfs_rescan_devices
 # success, all done
-- 
2.31.1

[PATCH v4 5/5] btrfs/219: add to the auto group

[Anand Jain]

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.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
---
v3: A split from the patch 5/6.

 tests/btrfs/219 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/btrfs/219 b/tests/btrfs/219
index 0bbfd6949cc8..9da835dcab10 100755
--- a/tests/btrfs/219
+++ b/tests/btrfs/219
@@ -12,7 +12,7 @@
 #
 
 . ./common/preamble
-_begin_fstest quick volume
+_begin_fstest auto quick volume
 
 # Override the default cleanup function.
 _cleanup()
-- 
2.31.1

Re: [PATCH v4 3/5] btrfs/219: fix _cleanup() to successful release the loop-device

[Anand Jain]

On 11/1/23 19:20, Zorro Lang wrote:
> On Tue, Oct 31, 2023 at 08:53:41AM +0800, Anand Jain 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.
>>
>> 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>
>> ---
>>
>> v4: rm -f, removed error/output redirection
>>      rm, -r removed for file image
>>      Check for the initialization of the local variable loop_dev[1-2]
>>       before calling  _destroy_loop_device().
> 
> This looks like a single change of the whole patchset below:
>    [PATCH 0/6 v3] btrfs/219 cloned-device mount capability update
> 
> The v3 patchset has some review points, better to be changed too.
> And the V3 has 6 patches in the patchset, now this patch names 3/5.
> I'm a little confused. Could you send the whole v4 patchset? Of
> course you can send them with the RVBs you've gotten.

One patch, 2/6 of v3, was dropped, and one patch, 4/6 of v3, was
changed; it was sent as a reply to that patch. Somehow, it didn't
work. Now, I have sent all the patches as a bundle in v4. HTH.


Thanks, Anand

Re: [PATCH v4 1/5] common/rc: _fs_sysfs_dname fetch fsid using btrfs tool

[David Sterba]

On Thu, Nov 02, 2023 at 07:28:18PM +0800, Anand Jain wrote:
> Currently _fs_sysfs_dname gets fsid from the findmnt command however
> this command provides the metadata_uuid if the device is mounted with
> temp-fsid. So instead, use btrfs filesystem show command to know the fsid.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> ---
> v3: add local variable fsid
> 
>  common/rc | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/common/rc b/common/rc
> index 259a1ffb09b9..7f14c19ca89e 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -4721,6 +4721,7 @@ _require_statx()
>  _fs_sysfs_dname()
>  {
>  	local dev=$1
> +	local fsid
>  
>  	if [ ! -b "$dev" ]; then
>  		_fail "Usage: _fs_sysfs_dname <mounted_device>"
> @@ -4728,7 +4729,9 @@ _fs_sysfs_dname()
>  
>  	case "$FSTYP" in
>  	btrfs)
> -		findmnt -n -o UUID ${dev} ;;
> +		fsid=$($BTRFS_UTIL_PROG filesystem show ${dev} | grep uuid: | \
> +							$AWK_PROG '{print $NF}')

This could be also added as a subcommand to 'inspect-internal', ie. from
a file to it's filesystem uuid, then it should be easy to get any other
id if needed through the sysfs directory.

Re: [PATCH v4 1/5] common/rc: _fs_sysfs_dname fetch fsid using btrfs tool

[Anand Jain]

On 11/3/23 04:00, David Sterba wrote:
> On Thu, Nov 02, 2023 at 07:28:18PM +0800, Anand Jain wrote:
>> Currently _fs_sysfs_dname gets fsid from the findmnt command however
>> this command provides the metadata_uuid if the device is mounted with
>> temp-fsid. So instead, use btrfs filesystem show command to know the fsid.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> Reviewed-by: Filipe Manana <fdmanana@suse.com>
>> ---
>> v3: add local variable fsid
>>
>>   common/rc | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/rc b/common/rc
>> index 259a1ffb09b9..7f14c19ca89e 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -4721,6 +4721,7 @@ _require_statx()
>>   _fs_sysfs_dname()
>>   {
>>   	local dev=$1
>> +	local fsid
>>   
>>   	if [ ! -b "$dev" ]; then
>>   		_fail "Usage: _fs_sysfs_dname <mounted_device>"
>> @@ -4728,7 +4729,9 @@ _fs_sysfs_dname()
>>   
>>   	case "$FSTYP" in
>>   	btrfs)
>> -		findmnt -n -o UUID ${dev} ;;
>> +		fsid=$($BTRFS_UTIL_PROG filesystem show ${dev} | grep uuid: | \
>> +							$AWK_PROG '{print $NF}')
> 
> This could be also added as a subcommand to 'inspect-internal', ie. from
> a file to it's filesystem uuid, then it should be easy to get any other
> id if needed through the sysfs directory.


Thanks for the feedback. I'll create a separate patch for the
'get_btrfs_fsid' helper, making it possible to merge this series
with RB.

Thanks Anand