| Message ID | 20230720061727.2363548-1-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fstests: add helper to canonicalize devices used to enable persistent disks | expand |
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>
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 >
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 > > >
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 > > > > > >
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
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
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
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
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 >
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
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(-)