Re: [PATCH 06/12] create a helper to clone devices

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 06/12] create a helper to clone devices
Date: Thu, 15 Feb 2024 12:27:17 +0000	[thread overview]
Message-ID: <CAL3q7H7SW8u=nxZAPtbYyiJ_3KRudzjDKxwghQixsFoPUUzSBA@mail.gmail.com> (raw)
In-Reply-To: <8a9a66bdd309ea6ea81c3586674922517c3460be.1707969354.git.anand.jain@oracle.com>

On Thu, Feb 15, 2024 at 6:35 AM Anand Jain <anand.jain@oracle.com> wrote:
>
> The function create_cloned_devices() takes two devices, creates
> a filesystem using mkfs, and clones the content from one device
> to another using device dump.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  common/btrfs | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/common/btrfs b/common/btrfs
> index 5cba9b16b4de..61d5812d49d9 100644
> --- a/common/btrfs
> +++ b/common/btrfs
> @@ -826,3 +826,25 @@ check_fsid()
>         echo -e -n "Tempfsid status:\t"
>         cat /sys/fs/btrfs/$tempfsid/temp_fsid
>  }
> +
> +create_cloned_devices()
> +{
> +       local dev1=$1
> +       local dev2=$2
> +
> +       [[ -z $dev1 || -z $dev2 ]] && \
> +               _fail "BUGGY code, create_cloned_devices needs arg1 arg3"

arg3 should be arg2.

But rather than such a cryptic message, I would for something more
clear and informative such as:

"create_cloned_devices() requires two devices as arguments"

> +
> +       echo -n Creating cloned device...
> +       _mkfs_dev -fq -b $((1024 * 1024 * 300)) $dev1
> +
> +       _mount $dev1 $SCRATCH_MNT
> +
> +       $XFS_IO_PROG -fc 'pwrite -S 0x61 0 9000' $SCRATCH_MNT/foo | \
> +                                                               _filter_xfs_io
> +       umount $SCRATCH_MNT

$UMOUNT_PROG

So we have a generic function that is named create_cloned_devices()
that is too specific for
a single test. The whole thing of creating exactly a 300M fs and with
a single file, it's anything but generic.

Really as it's only used in a single test case and given its
specificity, it should not be made generic by having it in
common/btrfs.
I'd rather have it in the test case that is using it.

Thanks.

> +       # device dump of $dev1 to $dev2
> +       dd if=$dev1 of=$dev2 bs=300M count=1 conv=fsync status=none || \
> +                                                       _fail "dd failed: $?"
> +       echo done
> +}
> --
> 2.39.3
>
>