Re: [PATCH 10/12] btrfs: verify tempfsid clones using mkfs

From: Filipe Manana <fdmanana@kernel.org>
To: Anand Jain <anand.jain@oracle.com>
Cc: fstests@vger.kernel.org, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 10/12] btrfs: verify tempfsid clones using mkfs
Date: Thu, 15 Feb 2024 12:46:36 +0000	[thread overview]
Message-ID: <CAL3q7H6=+HgSk6upxbTFM6M3Y3aynNeq-3Pp5-FtZU-0EB+kqw@mail.gmail.com> (raw)
In-Reply-To: <d59e5946ce2bbdb5dd954e715b15b9e834b88493.1707969354.git.anand.jain@oracle.com>

On Thu, Feb 15, 2024 at 6:36 AM Anand Jain <anand.jain@oracle.com> wrote:
>
> Create appearing to be a clone using the mkfs.btrfs option and test if
> the tempfsid is active.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  tests/btrfs/313     | 66 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/313.out | 16 +++++++++++
>  2 files changed, 82 insertions(+)
>  create mode 100755 tests/btrfs/313
>  create mode 100644 tests/btrfs/313.out
>
> diff --git a/tests/btrfs/313 b/tests/btrfs/313
> new file mode 100755
> index 000000000000..45811320e596
> --- /dev/null
> +++ b/tests/btrfs/313
> @@ -0,0 +1,66 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2024 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test 313
> +#
> +# Functional test for the tempfsid, clone devices created using the mkfs option.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick tempfsid

clone group as well, it uses reflinks.

> +
> +_cleanup()
> +{
> +       cd /
> +       umount $mnt1 > /dev/null 2>&1

$UMOUNT_PROG

> +       rm -r -f $tmp.*
> +       rm -r -f $mnt1
> +}
> +
> +. ./common/filter.btrfs
> +. ./common/reflink
> +
> +_supported_fs btrfs
> +_require_cp_reflink
> +_require_btrfs_sysfs_fsid
> +_require_scratch_dev_pool 2
> +_require_btrfs_fs_feature temp_fsid
> +_require_btrfs_command inspect-internal dump-super
> +_require_btrfs_mkfs_uuid_option
> +
> +_scratch_dev_pool_get 2
> +
> +mnt1=$TEST_DIR/$seq/mnt1
> +mkdir -p $mnt1
> +
> +clone_uuids_verify_tempfsid()
> +{
> +       echo ---- $FUNCNAME ----
> +       mkfs_clone ${SCRATCH_DEV_NAME[0]} ${SCRATCH_DEV_NAME[1]}
> +
> +       echo Mounting original device
> +       _mount ${SCRATCH_DEV_NAME[0]} $SCRATCH_MNT
> +       check_fsid ${SCRATCH_DEV_NAME[0]}
> +
> +       echo Mounting cloned device
> +       _mount ${SCRATCH_DEV_NAME[1]} $mnt1 || \
> +                               _fail "mount failed, tempfsid didn't work"
> +       check_fsid ${SCRATCH_DEV_NAME[1]}
> +
> +       $XFS_IO_PROG -fc 'pwrite -S 0x61 0 9000' $SCRATCH_MNT/foo | \
> +                                                               _filter_xfs_io
> +       echo cp reflink must fail
> +       _cp_reflink $SCRATCH_MNT/foo $mnt1/bar > $tmp.cp.out 2>&1
> +       ret=$?
> +       cat $tmp.cp.out | _filter_testdir_and_scratch
> +       if [ $ret -ne 1 ]; then
> +               _fail "reflink failed to fail"
> +       fi

Same comment as in a previous patch.

So much complexity to check if a reflink fails...

All this could be accomplished with a single line:

_cp_reflink $SCRATCH_MNT/foo $mnt1/bar

And then have the golden output expect an error message. That's the
most standard and prefered way to do things in fstests.
No need to redirect stdout and stderr to a temporary file, check
return value, check the temporary file, etc...

> +}
> +
> +clone_uuids_verify_tempfsid

Really, why have all the test code inside a function that is called only once?
Get rid of the function...

Thanks.

> +_scratch_dev_pool_put
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/btrfs/313.out b/tests/btrfs/313.out
> new file mode 100644
> index 000000000000..7a089d2c29c5
> --- /dev/null
> +++ b/tests/btrfs/313.out
> @@ -0,0 +1,16 @@
> +QA output created by 313
> +---- clone_uuids_verify_tempfsid ----
> +Mounting original device
> +On disk fsid:          FSID
> +Metadata uuid:         FSID
> +Temp fsid:             FSID
> +Tempfsid status:       0
> +Mounting cloned device
> +On disk fsid:          FSID
> +Metadata uuid:         FSID
> +Temp fsid:             TEMPFSID
> +Tempfsid status:       1
> +wrote 9000/9000 bytes at offset 0
> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +cp reflink must fail
> +cp: failed to clone 'TEST_DIR/313/mnt1/bar' from 'SCRATCH_MNT/foo': Invalid cross-device link
> --
> 2.39.3
>
>