Re: [PATCH] fstests: add helper to canonicalize devices used to enable persistent disks

From: Zorro Lang <zlang@redhat.com>
To: Luis Chamberlain <mcgrof@kernel.org>
Cc: fstests@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	patches@lists.linux.dev
Subject: Re: [PATCH] fstests: add helper to canonicalize devices used to enable persistent disks
Date: Tue, 25 Jul 2023 16:13:07 +0800	[thread overview]
Message-ID: <20230725081307.xydlwjdl4lq3ts3m@zlang-mailbox> (raw)
In-Reply-To: <20230720061727.2363548-1-mcgrof@kernel.org>

On Wed, Jul 19, 2023 at 11:17:27PM -0700, Luis Chamberlain wrote:
> The filesystem configuration file does not allow you to use symlinks to
> devices given the existing sanity checks verify that the target end
> device matches the source.
> 
> Using a symlink is desirable if you want to enable persistent tests
> across reboots. For example you may want to use /dev/disk/by-id/nvme-eui.*
> so to ensure that the same drives are used even after reboot. This
> is very useful if you are testing for example with a virtualized
> environment and are using PCIe passthrough with other qemu NVMe drives
> with one or many NVMe drives.
> 
> To enable support just add a helper to canonicalize devices prior to
> running the tests.
> 
> This allows one test runner, kdevops, which I just extended with
> support to use real NVMe drives. The drives it uses for the filesystem
> configuration optionally is with NVMe eui symlinks so to allow
> the same drives to be used over reboots.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---

Hi Luis,

Hmmm... this's a default behavior change for fstests, although I'm not sure
what will be affect. I'm wondering if we should do this in fstests. I don't
want to tell the users that the device names they give to fstests will always
be truned to real names from now on.

Generally the users of fstests provide the device names, so the users
might need to take care of the name is "/dev/mapper/testvg-testdev"
or "/dev/dm-4". The users can deal with the device names when their
script prepare to run fstests.

If more developers prefer this change, I'd like to make it to be
optional by an option of ./check at least, not always turn devname
to realpath. Welcome review points from others.

>  check         |  1 +
>  common/config | 44 +++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/check b/check
> index 89e7e7bf20df..d063d3f498fd 100755
> --- a/check
> +++ b/check
> @@ -734,6 +734,7 @@ function run_section()
>  	fi
>  
>  	get_next_config $section
> +	_canonicalize_devices
>  
>  	mkdir -p $RESULT_BASE
>  	if [ ! -d $RESULT_BASE ]; then
> diff --git a/common/config b/common/config
> index 936ac225f4b1..f5a3815a0435 100644
> --- a/common/config
> +++ b/common/config
> @@ -655,6 +655,47 @@ _canonicalize_mountpoint()
>  	echo "$parent/$base"
>  }
>  
> +# Enables usage of /dev/disk/by-id/ symlinks to persist target devices
> +# over reboots
> +_canonicalize_devices()
> +{
> +	if [ ! -z "$TEST_DEV" ] && [ -L $TEST_DEV ]; then

I think [ -L "$TEST_DEV" ] is enough.

> +		TEST_DEV=$(realpath -e $TEST_DEV)

Anyone knows the difference of realpatch and readlink?

> +	fi
> +
> +	if [ ! -z "$SCRATCH_DEV" ] && [ -L $SCRATCH_DEV ]; then
> +		SCRATCH_DEV=$(realpath -e $SCRATCH_DEV)
> +	fi
> +
> +	if [ ! -z "$TEST_LOGDEV" ] && [ -L $TEST_LOGDEV ]; then
> +		TEST_LOGDEV=$(realpath -e $TEST_LOGDEV)
> +	fi
> +
> +	if [ ! -z "$TEST_RTDEV" ] && [ -L $TEST_RTDEV ]; then
> +		TEST_RTDEV=$(realpath -e $TEST_RTDEV)
> +	fi
> +
> +	if [ ! -z "$SCRATCH_RTDEV" ] && [ -L $SCRATCH_RTDEV ]; then
> +		SCRATCH_RTDEV=$(realpath -e $SCRATCH_RTDEV)
> +	fi
> +
> +	if [ ! -z "$LOGWRITES_DEV" ] && [ -L $LOGWRITES_DEV ]; then
> +		LOGWRITES_DEV=$(realpath -e $LOGWRITES_DEV)
> +	fi
> +
> +	if [ ! -z "$SCRATCH_DEV_POOL" ]; then
> +		NEW_SCRATCH_POOL=""
> +		for i in $SCRATCH_DEV_POOL; do
> +			if [ -L $i ]; then
> +				NEW_SCRATCH_POOL="$NEW_SCRATCH_POOL $(realpath -e $i)"
> +			else
> +				NEW_SCRATCH_POOL="$NEW_SCRATCH_POOL $i)"
                                                                     ^^^

What's this half ")" for ?

Thanks,
Zorro

> +			fi
> +		done
> +		SCRATCH_DEV_POOL="$NEW_SCRATCH_POOL"
> +	fi
> +}
> +
>  # On check -overlay, for the non multi section config case, this
>  # function is called on every test, before init_rc().
>  # When SCRATCH/TEST_* vars are defined in config file, config file
> @@ -785,7 +826,6 @@ get_next_config() {
>  	fi
>  
>  	parse_config_section $1
> -
>  	if [ ! -z "$OLD_FSTYP" ] && [ $OLD_FSTYP != $FSTYP ]; then
>  		[ -z "$MOUNT_OPTIONS" ] && _mount_opts
>  		[ -z "$TEST_FS_MOUNT_OPTS" ] && _test_mount_opts
> @@ -901,5 +941,7 @@ else
>  	fi
>  fi
>  
> +_canonicalize_devices
> +
>  # make sure this script returns success
>  /bin/true
> -- 
> 2.39.2
> 