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

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

[Luis Chamberlain]

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>
---
 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
+		TEST_DEV=$(realpath -e $TEST_DEV)
+	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)"
+			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

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

[Andrey Albershteyn]

On 2023-07-19 23:17:27, 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>
> ---

Looks good to me:
Reviewed-by: Andrey Albershteyn <aalbersh@redhat.com>

-- 
- Andrey

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

[Zorro Lang]

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
> 

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

[Darrick J. Wong]

On Tue, Jul 25, 2023 at 04:13:07PM +0800, Zorro Lang wrote:
> 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.

Hmm.  SCRATCH_DEV=/dev/testvg/testlv works right now, it'd be sort of
annoying to have "/dev/dm-4" get written into the report and then you've
lost the correlation to whatever the user passed in.

Oh wait.  My giant mound of ./check wrapper script already does that
canonicalization and has for years.

Ok.  Sounds good to me then. :P

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

I don't think it is.

$ unset moo
$ [ -L $moo ]
$ echo $?
0
$ realpath -e $moo
realpath: missing operand
Try 'realpath --help' for more information.

> > +		TEST_DEV=$(realpath -e $TEST_DEV)
> 
> Anyone knows the difference of realpatch and readlink?

readlink doesn't follow nested symlinks; realpath does:

$ touch /tmp/a
$ ln -s /tmp/a /tmp/b
$ ln -s /tmp/b /tmp/c
$ readlink /tmp/c
/tmp/b
$ realpath /tmp/c
/tmp/a
$ readlink -m /tmp/c
/tmp/a

> > +	fi
> > +
> > +	if [ ! -z "$SCRATCH_DEV" ] && [ -L $SCRATCH_DEV ]; then
> > +		SCRATCH_DEV=$(realpath -e $SCRATCH_DEV)

Extra question: Shouldn't we be putting theis ^^^ variables in quotes?

Supposing someone starts passing in
SCRATCH_DEV=/dev/disk/by-label/har har"

This expression will barf everywhere:

$ SCRATCH_DEV=$(realpath -e $SCRATCH_DEV)
realpath: /dev/disk/by-label/har: No such file or directory
realpath: har: No such file or directory

Due to the lack of quoting on its way to turning that into /dev/sda3.
Granted fstests has historically required no spaces in anything, but
still, it's bad hygiene.

[writing anything in bash is bad hygiene, but for the sweet sweet pipe
goodness]

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

Some kind of typo, I assume...

--D

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

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

[Zorro Lang]

On Tue, Jul 25, 2023 at 08:54:39AM -0700, Darrick J. Wong wrote:
> On Tue, Jul 25, 2023 at 04:13:07PM +0800, Zorro Lang wrote:
> > 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.
> 
> Hmm.  SCRATCH_DEV=/dev/testvg/testlv works right now, it'd be sort of
> annoying to have "/dev/dm-4" get written into the report and then you've
> lost the correlation to whatever the user passed in.
> 
> Oh wait.  My giant mound of ./check wrapper script already does that
> canonicalization and has for years.
> 
> Ok.  Sounds good to me then. :P

So you hope fstests can do this translation (use real device name) by default?

> 
> > >  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.
> 
> I don't think it is.
> 
> $ unset moo
> $ [ -L $moo ]

The double quotation marks "" are needed.

> $ echo $?
> 0
> $ realpath -e $moo
> realpath: missing operand
> Try 'realpath --help' for more information.
> 
> > > +		TEST_DEV=$(realpath -e $TEST_DEV)
> > 
> > Anyone knows the difference of realpatch and readlink?
> 
> readlink doesn't follow nested symlinks; realpath does:
> 
> $ touch /tmp/a
> $ ln -s /tmp/a /tmp/b
> $ ln -s /tmp/b /tmp/c
> $ readlink /tmp/c
> /tmp/b
> $ realpath /tmp/c
> /tmp/a
> $ readlink -m /tmp/c
> /tmp/a

The -e option helps:

# readlink -e /tmp/c
/tmp/a
# realpath -e /tmp/c
/tmp/a

> 
> > > +	fi
> > > +
> > > +	if [ ! -z "$SCRATCH_DEV" ] && [ -L $SCRATCH_DEV ]; then
> > > +		SCRATCH_DEV=$(realpath -e $SCRATCH_DEV)
> 
> Extra question: Shouldn't we be putting theis ^^^ variables in quotes?

Make sense to me.

Thanks,
Zorro

> 
> Supposing someone starts passing in
> SCRATCH_DEV=/dev/disk/by-label/har har"
> 
> This expression will barf everywhere:
> 
> $ SCRATCH_DEV=$(realpath -e $SCRATCH_DEV)
> realpath: /dev/disk/by-label/har: No such file or directory
> realpath: har: No such file or directory
> 
> Due to the lack of quoting on its way to turning that into /dev/sda3.
> Granted fstests has historically required no spaces in anything, but
> still, it's bad hygiene.
> 
> [writing anything in bash is bad hygiene, but for the sweet sweet pipe
> goodness]
> 
> > > +	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 ?
> 
> Some kind of typo, I assume...
> 
> --D
> 
> > 
> > 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
> > > 
> > 
> 

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

[Luis Chamberlain]

On Wed, Jul 26, 2023 at 01:50:09AM +0800, Zorro Lang wrote:
> On Tue, Jul 25, 2023 at 08:54:39AM -0700, Darrick J. Wong wrote:
> > On Tue, Jul 25, 2023 at 04:13:07PM +0800, Zorro Lang wrote:
> > > On Wed, Jul 19, 2023 at 11:17:27PM -0700, Luis Chamberlain wrote:
> > > > 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.
> > >
> > > 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.
> > 
> > Hmm.  SCRATCH_DEV=/dev/testvg/testlv works right now, it'd be sort of
> > annoying to have "/dev/dm-4" get written into the report and then you've
> > lost the correlation to whatever the user passed in.
> > 
> > Oh wait.  My giant mound of ./check wrapper script already does that
> > canonicalization and has for years.
> > 
> > Ok.  Sounds good to me then. :P
> 
> So you hope fstests can do this translation (use real device name) by default?

I'm fine if we make this optional too. In my next spin I'll add an
option that does this.

> > > I think [ -L "$TEST_DEV" ] is enough.
> > 
> > I don't think it is.
> > 
> > $ unset moo
> > $ [ -L $moo ]
> 
> The double quotation marks "" are needed.

Indeed it does. Will go with that for v2.

> > $ echo $?
> > 0
> > $ realpath -e $moo
> > realpath: missing operand
> > Try 'realpath --help' for more information.
> > 
> > > > +		TEST_DEV=$(realpath -e $TEST_DEV)
> > > 
> > > Anyone knows the difference of realpatch and readlink?
> > 
> > readlink doesn't follow nested symlinks; realpath does:
> > 
> > $ touch /tmp/a
> > $ ln -s /tmp/a /tmp/b
> > $ ln -s /tmp/b /tmp/c
> > $ readlink /tmp/c
> > /tmp/b
> > $ realpath /tmp/c
> > /tmp/a
> > $ readlink -m /tmp/c
> > /tmp/a
> 
> The -e option helps:
> 
> # readlink -e /tmp/c
> /tmp/a

I can trace readlink -e support back to 2004:

https://github.com/coreutils/coreutils/commit/e0b8973bd4b146b5fb39641a4ee7984e922c3ff5

> # realpath -e /tmp/c
> /tmp/a

realpeath is much newer than readlink, it's first commit including
support for -e dates back to 2011;

https://github.com/coreutils/coreutils/commit/77ea441f79aa115f79b47d9c1fc9c0004c5c7111

And so canonicalizing with readlink is 7 years older and likely to be
supported in more ancient distros with older coreutils. I'll go with
readlink -e unless I hear otherwise.

> > > > +	fi
> > > > +
> > > > +	if [ ! -z "$SCRATCH_DEV" ] && [ -L $SCRATCH_DEV ]; then
> > > > +		SCRATCH_DEV=$(realpath -e $SCRATCH_DEV)
> > 
> > Extra question: Shouldn't we be putting theis ^^^ variables in quotes?
> 
> Make sense to me.

Done. The only eye sore is how to convert the loop for the SCRATCH_DEV_POOL,
here's what I have for a v2:

diff --git a/README b/README
index 1ca506492bf0..97ef63d6d693 100644
--- a/README
+++ b/README
@@ -268,6 +268,9 @@ Misc:
    this option is supported for all filesystems currently only -overlay is
    expected to run without issues. For other filesystems additional patches
    and fixes to the test suite might be needed.
+ - set CANON_DEVS=yes to canonicalize device symlinks. This will let you
+   for example use something like TEST_DEV/dev/disk/by-id/nvme-* so the
+   device remains persistent between reboots. This is disabled by default.
 
 ______________________
 USING THE FSQA SUITE
diff --git a/check b/check
index 0bf5b22e061a..577e09655844 100755
--- a/check
+++ b/check
@@ -711,6 +711,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 6c8cb3a5ba68..7d74c285ac71 100644
--- a/common/config
+++ b/common/config
@@ -25,6 +25,9 @@
 # KEEP_DMESG -      whether to keep all dmesg for each test case.
 #                   yes: keep all dmesg
 #                   no: only keep dmesg with error/warning (default)
+# CANON_DEVS -      whether or not to canonicalize device symlinks
+#                   yes: canonicalize device symlinks
+#                   no (default) do not canonicalize device if they are symlinks
 #
 # - These can be added to $HOST_CONFIG_DIR (witch default to ./config)
 #   below or a separate local configuration file can be used (using
@@ -644,6 +647,32 @@ _canonicalize_mountpoint()
 	echo "$parent/$base"
 }
 
+# Enables usage of /dev/disk/by-id/ symlinks to persist target devices
+# over reboots
+_canonicalize_devices()
+{
+	if [ "$CANON_DEVS" != "yes" ]; then
+		return
+	fi
+	[ -L "$TEST_DEV" ]	&& TEST_DEV=$(readlink -e "$TEST_DEV")
+	[ -L $SCRATCH_DEV ]	&& SCRATCH_DEV=$(readlink -e "$SCRATCH_DEV")
+	[ -L $TEST_LOGDEV ]	&& TEST_LOGDEV=$(readlink -e "$TEST_LOGDEV")
+	[ -L $TEST_RTDEV ]	&& TEST_RTDEV=$(readlink -e "$TEST_RTDEV")
+	[ -L $SCRATCH_RTDEV ]	&& SCRATCH_RTDEV=$(readlink -e "$SCRATCH_RTDEV")
+	[ -L $LOGWRITES_DEV ]	&& LOGWRITES_DEV=$(readlink -e "$LOGWRITES_DEV")
+	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 $(readlink -e $i)"
+			else
+				NEW_SCRATCH_POOL="$NEW_SCRATCH_POOL $i)"
+			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
@@ -774,7 +803,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
@@ -890,5 +918,7 @@ else
 	fi
 fi
 
+_canonicalize_devices
+
 # make sure this script returns success
 /bin/true

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

[Theodore Ts'o]

On Tue, Jul 25, 2023 at 08:54:39AM -0700, Darrick J. Wong wrote:
> On Tue, Jul 25, 2023 at 04:13:07PM +0800, Zorro Lang wrote:
> > 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.

I'm a little confused.  Where are these "sanity checks" enforced?
I've been using

SCRATCH_DEV=/dev/mapper/xt-vdc

where /dev/mapper/xt-vdc is a symlink to /dev/dm-4 (or some such)
without any problems.  So I don't quite understand why we need to
canonicalize devices?

					- Ted

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

[Luis Chamberlain]

On Wed, Jul 26, 2023 at 12:41:32AM -0400, Theodore Ts'o wrote:
> On Tue, Jul 25, 2023 at 08:54:39AM -0700, Darrick J. Wong wrote:
> > On Tue, Jul 25, 2023 at 04:13:07PM +0800, Zorro Lang wrote:
> > > 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.
> 
> I'm a little confused.  Where are these "sanity checks" enforced?
> I've been using
> 
> SCRATCH_DEV=/dev/mapper/xt-vdc
> 
> where /dev/mapper/xt-vdc is a symlink to /dev/dm-4 (or some such)
> without any problems.  So I don't quite understand why we need to
> canonicalize devices?

That might work, but try using /dev/disk/by-id/ stuff, that'll bust. So
to keep existing expecations by fstests, it's needed.

  Luis

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

[Theodore Ts'o]

On Wed, Jul 26, 2023 at 09:28:23AM -0700, Luis Chamberlain wrote:
> > I'm a little confused.  Where are these "sanity checks" enforced?
> > I've been using
> > 
> > SCRATCH_DEV=/dev/mapper/xt-vdc
> > 
> > where /dev/mapper/xt-vdc is a symlink to /dev/dm-4 (or some such)
> > without any problems.  So I don't quite understand why we need to
> > canonicalize devices?
> 
> That might work, but try using /dev/disk/by-id/ stuff, that'll bust. So
> to keep existing expecations by fstests, it's needed.

What goes wrong, and why?  /dev/disk/by-id/<disk-id> is a symlink,
just like /dev/mapper/<vg>-<lv> is a symlink.

What am I missing?

Thanks,

						- Ted

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

[Darrick J. Wong]

On Wed, Jul 26, 2023 at 09:13:30PM -0400, Theodore Ts'o wrote:
> On Wed, Jul 26, 2023 at 09:28:23AM -0700, Luis Chamberlain wrote:
> > > I'm a little confused.  Where are these "sanity checks" enforced?
> > > I've been using
> > > 
> > > SCRATCH_DEV=/dev/mapper/xt-vdc
> > > 
> > > where /dev/mapper/xt-vdc is a symlink to /dev/dm-4 (or some such)
> > > without any problems.  So I don't quite understand why we need to
> > > canonicalize devices?
> > 
> > That might work, but try using /dev/disk/by-id/ stuff, that'll bust. So
> > to keep existing expecations by fstests, it's needed.
> 
> What goes wrong, and why?  /dev/disk/by-id/<disk-id> is a symlink,
> just like /dev/mapper/<vg>-<lv> is a symlink.
> 
> What am I missing?

# mkfs.xfs -f /dev/sda
# mount /dev/sda /mnt
# TEST_DIR=/mnt TEST_DEV=/dev/sda FSTYP=xfs ./check generic/110
FSTYP         -- xfs (debug)
PLATFORM      -- Linux/x86_64 flax-mtr01 6.5.0-rc3-djwx #rc3 SMP PREEMPT_DYNAMIC Wed Jul 26 14:26:48 PDT 2023

generic/110        2s
Ran: generic/110
Passed all 1 tests

versus:

# TEST_DIR=/mnt TEST_DEV=/dev/disk/by-id/scsi-0QEMU_RAMDISK_drive-scsi0-0-0-0 FSTYP=xfs ./check generic/110
mount: /mnt: /dev/sda already mounted on /mnt.
common/rc: retrying test device mount with external set
mount: /mnt: /dev/sda already mounted on /mnt.
common/rc: could not mount /dev/disk/by-id/scsi-0QEMU_RAMDISK_drive-scsi0-0-0-0 on /mnt
# umount /mnt
# TEST_DIR=/mnt TEST_DEV=/dev/disk/by-id/scsi-0QEMU_RAMDISK_drive-scsi0-0-0-0 FSTYP=xfs ./check generic/110
TEST_DEV=/dev/disk/by-id/scsi-0QEMU_RAMDISK_drive-scsi0-0-0-0 is mounted but not on TEST_DIR=/mnt - aborting
Already mounted result:
/dev/sda /mnt

(This is not really how I run fstests, it's just the minimum example.)

--D

> Thanks,
> 
> 						- Ted
>