Fwd: [PATCH v2 0/5] re-enable tests that require scratch dev on NFS

Fwd: [PATCH v2 0/5] re-enable tests that require scratch dev on NFS

[Steve French]

Has anyone tried these newly enabled xfstests on cifs?


---------- Forwarded message ----------
From: Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date: Fri, Oct 31, 2014 at 12:03 PM
Subject: [PATCH v2 0/5] re-enable tests that require scratch dev on NFS
To: fstests-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>


The commit below disables tests requires scratch dev running on NFS

c041421 xfstests: stop special casing nfs and udf

Now re-enable them to get a larger test coverage on NFS.

Also do more updates to avoid unnecessary failures on NFS.

I tested against NFSv3 NFSv4.0 NFSv4.1 (both server and client are linux
running 3.18-rc1 kernel), the results look good.

Failures on NFSv3:
generic/035 generic/089 generic/258 generic/294

Failures on NFSv4.0:
generic/035 generic/169 generic/294

Failures on NFSv4.1:
I hit kernel BUG_ON when testing on NFSv4.1 in generic/285
SEEK_DATA/SEEK_HOLE test. I think there's already a patch to fix it.

http://www.spinics.net/lists/linux-nfs/msg47359.html

Note that generic/294 does remount,ro on SCRATCH_DEV, but TEST_DEV is
affected too, so some tests after generic/294 fail because of EROFS.
Run the failed tests separately and they all passed.

I'm not sure if it's a bug, but I filed one, please see
https://bugzilla.redhat.com/show_bug.cgi?id=1158046


The third patch disables all atime related tests on NFS, and Christoph
starts a discussion about NFS atime handling issue in v1 thread. Before
there's a conclusion I keep the patch as it is, and we can update it
later when we decide to test atime on NFS again.

v2:
- introduce _scratch_cleanup_files helper to remove all files on
  $SCRATCH_MNT, for later CIFS use. (Christoph Hellwig)
- split _require_relatime change into another patch (Christoph Hellwig)

v1: http://www.spinics.net/lists/linux-nfs/msg47423.html

Thanks,
Eryu

---

Eryu Guan (5):
  common: re-enable tests that require scratch dev on NFS
  common: add _require_block_device() helper
  common: skip atime related tests on NFS
  common: use _scratch_mount helper in _require_relatime()
  generic/277: add _require_attrs

 common/rc         | 46 ++++++++++++++++++++++++++++++++++++++++++----
 tests/generic/003 |  1 +
 tests/generic/076 |  1 +
 tests/generic/192 |  1 +
 tests/generic/277 |  2 ++
 5 files changed, 47 insertions(+), 4 deletions(-)

--
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Thanks,

Steve

Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS

[Steve French]

There should be a check to make sure SCRATCH_MNT exists before you
wipe the whole disk ....

+# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS
+_scratch_cleanup_files()
+{
+       _scratch_mount
+       rm -rf $SCRATCH_MNT/*
+       _scratch_unmount
+}
+
so if no SCRATCH_MNT then this does rm -rf/*
right ... (and wipes out your whole system ...)

On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> This commit disables tests requires scratch dev running on NFS
>
> c041421 xfstests: stop special casing nfs and udf
>
> Now re-enable them to get a larger test coverage on NFS.
>
> Signed-off-by: Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  common/rc | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/common/rc b/common/rc
> index 747cf72..ae03712 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -551,6 +551,14 @@ _mkfs_dev()
>      rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd
>  }
>
> +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS
> +_scratch_cleanup_files()
> +{
> +       _scratch_mount
> +       rm -rf $SCRATCH_MNT/*
> +       _scratch_unmount
> +}
> +
>  _scratch_mkfs()
>  {
>      case $FSTYP in
> @@ -558,7 +566,9 @@ _scratch_mkfs()
>          _scratch_mkfs_xfs $*
>         ;;
>      nfs*)
> -       # do nothing for nfs
> +       # unable to re-create NFS, just remove all files in $SCRATCH_MNT to
> +       # avoid EEXIST caused by the leftover files created in previous runs
> +        _scratch_cleanup_files
>         ;;
>      cifs)
>         # do nothing for cifs
> @@ -1032,8 +1042,14 @@ _require_scratch_nocheck()
>  {
>      case "$FSTYP" in
>         nfs*)
> -                 _notrun "requires a scratch device"
> -                ;;
> +               echo $SCRATCH_DEV | grep -q ":/" > /dev/null 2>&1
> +               if [ -z "$SCRATCH_DEV" -o "$?" != "0" ]; then
> +                       _notrun "this test requires a valid \$SCRATCH_DEV"
> +               fi
> +               if [ ! -d "$SCRATCH_MNT" ]; then
> +                       _notrun "this test requires a valid \$SCRATCH_MNT"
> +               fi
> +               ;;
>         cifs)
>                 _notrun "requires a scratch device"
>                 ;;
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,

Steve

Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS

[Dave Chinner]

On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote:
> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > This commit disables tests requires scratch dev running on NFS
> >
> > c041421 xfstests: stop special casing nfs and udf
> >
> > Now re-enable them to get a larger test coverage on NFS.
> >
> > Signed-off-by: Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  common/rc | 22 +++++++++++++++++++---
> >  1 file changed, 19 insertions(+), 3 deletions(-)
> >
> > diff --git a/common/rc b/common/rc
> > index 747cf72..ae03712 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -551,6 +551,14 @@ _mkfs_dev()
> >      rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd
> >  }
> >
> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS
> > +_scratch_cleanup_files()
> > +{
> > +       _scratch_mount
> > +       rm -rf $SCRATCH_MNT/*
> > +       _scratch_unmount
> > +}
>
> There should be a check to make sure SCRATCH_MNT exists before you
> wipe the whole disk ....
> 
> so if no SCRATCH_MNT then this does rm -rf/*
> right ... (and wipes out your whole system ...)

You can't get to that function until after all the checks that
SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that
is only called in tests after all the startup checks validate
devices and mounts exist. i.e. see common/config::get_next_config()

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS

[Steve French]

On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote:
> On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote:
>> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > This commit disables tests requires scratch dev running on NFS
>> >
>> > c041421 xfstests: stop special casing nfs and udf
>> >
>> > Now re-enable them to get a larger test coverage on NFS.
>> >
>> > Signed-off-by: Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> > ---
>> >  common/rc | 22 +++++++++++++++++++---
>> >  1 file changed, 19 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/common/rc b/common/rc
>> > index 747cf72..ae03712 100644
>> > --- a/common/rc
>> > +++ b/common/rc
>> > @@ -551,6 +551,14 @@ _mkfs_dev()
>> >      rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd
>> >  }
>> >
>> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS
>> > +_scratch_cleanup_files()
>> > +{
>> > +       _scratch_mount
>> > +       rm -rf $SCRATCH_MNT/*
>> > +       _scratch_unmount
>> > +}
>>
>> There should be a check to make sure SCRATCH_MNT exists before you
>> wipe the whole disk ....
>>
>> so if no SCRATCH_MNT then this does rm -rf/*
>> right ... (and wipes out your whole system ...)
>
> You can't get to that function until after all the checks that
> SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that
> is only called in tests after all the startup checks validate
> devices and mounts exist. i.e. see common/config::get_next_config()

Well, I reproduced it easily enough again today (after taking a
snapshot of the VM)
by simply running generic/120 against NFS with SCRATCH_MNT not
specified in local.config
Dros also ran into this problem.

The patch below fixes it for me but it wasn't immediately obvious how to best
return info to the user (ie print messages that make sense here -
"echo" seems to be supressed
in common/rc and notrun exits and logs it to a file but not to the
screen in this case)

sfrench@ubuntu:~/xfstests$ git diff -a
diff --git a/common/rc b/common/rc
index d5e3aff..866244b 100644
--- a/common/rc
+++ b/common/rc
@@ -555,6 +555,9 @@ _mkfs_dev()
 _scratch_cleanup_files()
 {
        _scratch_mount
+       if ! [ "$SCRATCH_MNT" ]; then
+               _notrun "this test requires a \$SCRATCH_MNT to be specified"
+       fi
        rm -rf $SCRATCH_MNT/*
        _scratch_unmount
 }


-- 
Thanks,

Steve

Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS

[Eryu Guan]

On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote:
> On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote:
> > On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote:
> >> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >> > This commit disables tests requires scratch dev running on NFS
> >> >
> >> > c041421 xfstests: stop special casing nfs and udf
> >> >
> >> > Now re-enable them to get a larger test coverage on NFS.
> >> >
> >> > Signed-off-by: Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> > ---
> >> >  common/rc | 22 +++++++++++++++++++---
> >> >  1 file changed, 19 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/common/rc b/common/rc
> >> > index 747cf72..ae03712 100644
> >> > --- a/common/rc
> >> > +++ b/common/rc
> >> > @@ -551,6 +551,14 @@ _mkfs_dev()
> >> >      rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd
> >> >  }
> >> >
> >> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS
> >> > +_scratch_cleanup_files()
> >> > +{
> >> > +       _scratch_mount
> >> > +       rm -rf $SCRATCH_MNT/*
> >> > +       _scratch_unmount
> >> > +}
> >>
> >> There should be a check to make sure SCRATCH_MNT exists before you
> >> wipe the whole disk ....
> >>
> >> so if no SCRATCH_MNT then this does rm -rf/*
> >> right ... (and wipes out your whole system ...)
> >
> > You can't get to that function until after all the checks that
> > SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that
> > is only called in tests after all the startup checks validate
> > devices and mounts exist. i.e. see common/config::get_next_config()
> 
> Well, I reproduced it easily enough again today (after taking a
> snapshot of the VM)
> by simply running generic/120 against NFS with SCRATCH_MNT not
> specified in local.config
> Dros also ran into this problem.

You're right, I missed that _scratch_mkfs is also called by ./check,
and if there's no SCRATCH_MNT set in local.config, only SCRATCH_DEV is
set, _scratch_mkfs can be dangerous.

[root@hp-dl388eg8-01 xfstests]# ./check -nfs generic/001
FSTYP         -- nfs
PLATFORM      -- Linux/x86_64 hp-dl388eg8-01 3.18.0-rc4+
MKFS_OPTIONS  -- -bsize=4096 localhost:/mnt/nfsscratch
MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 localhost:/mnt/nfsscratch

our local _scratch_mkfs routine ...
mount: can't find localhost:/mnt/nfsscratch in /etc/fstab
rm -rf /*
umount: localhost:/mnt/nfsscratch: mountpoint not found
check: failed to mkfs $SCRATCH_DEV using specified options
Passed all 0 tests

(I replaced the real "rm -rf ..." by "echo 'rm -rf ...' in above test)

> 
> The patch below fixes it for me but it wasn't immediately obvious how to best
> return info to the user (ie print messages that make sense here -
> "echo" seems to be supressed
> in common/rc and notrun exits and logs it to a file but not to the
> screen in this case)
> 
> sfrench@ubuntu:~/xfstests$ git diff -a
> diff --git a/common/rc b/common/rc
> index d5e3aff..866244b 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -555,6 +555,9 @@ _mkfs_dev()
>  _scratch_cleanup_files()
>  {
>         _scratch_mount
> +       if ! [ "$SCRATCH_MNT" ]; then
> +               _notrun "this test requires a \$SCRATCH_MNT to be specified"
> +       fi
>         rm -rf $SCRATCH_MNT/*
>         _scratch_unmount
>  }

I propose this patch, which skips _scratch_cleanup_files if called by check

[root@hp-dl388eg8-01 xfstests]# git diff
diff --git a/common/rc b/common/rc
index 435f74f..254fb66 100644
--- a/common/rc
+++ b/common/rc
@@ -554,6 +554,11 @@ _mkfs_dev()
 # remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS
 _scratch_cleanup_files()
 {
+       # do nothing if called by check, variables are not fully valided yet
+       # SCRATCH_MNT can be empty, which is dangerous
+       if [ "$iam" == "check" ]; then
+               return
+       fi
        _scratch_mount
        rm -rf $SCRATCH_MNT/*
        _scratch_unmount

Thanks,
Eryu

Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS

[Dave Chinner]

On Sat, Nov 15, 2014 at 01:35:33PM +0800, Eryu Guan wrote:
> On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote:
> > On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote:
> > > On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote:
> > >> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > >> > This commit disables tests requires scratch dev running on NFS
> > >> >
> > >> > c041421 xfstests: stop special casing nfs and udf
> > >> >
> > >> > Now re-enable them to get a larger test coverage on NFS.
> > >> >
> > >> > Signed-off-by: Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > >> > ---
> > >> >  common/rc | 22 +++++++++++++++++++---
> > >> >  1 file changed, 19 insertions(+), 3 deletions(-)
> > >> >
> > >> > diff --git a/common/rc b/common/rc
> > >> > index 747cf72..ae03712 100644
> > >> > --- a/common/rc
> > >> > +++ b/common/rc
> > >> > @@ -551,6 +551,14 @@ _mkfs_dev()
> > >> >      rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd
> > >> >  }
> > >> >
> > >> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS
> > >> > +_scratch_cleanup_files()
> > >> > +{
> > >> > +       _scratch_mount
> > >> > +       rm -rf $SCRATCH_MNT/*
> > >> > +       _scratch_unmount
> > >> > +}
> > >>
> > >> There should be a check to make sure SCRATCH_MNT exists before you
> > >> wipe the whole disk ....
> > >>
> > >> so if no SCRATCH_MNT then this does rm -rf/*
> > >> right ... (and wipes out your whole system ...)
> > >
> > > You can't get to that function until after all the checks that
> > > SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that
> > > is only called in tests after all the startup checks validate
> > > devices and mounts exist. i.e. see common/config::get_next_config()
> > 
> > Well, I reproduced it easily enough again today (after taking a
> > snapshot of the VM)
> > by simply running generic/120 against NFS with SCRATCH_MNT not
> > specified in local.config
> > Dros also ran into this problem.
> 
> You're right, I missed that _scratch_mkfs is also called by ./check,
> and if there's no SCRATCH_MNT set in local.config, only SCRATCH_DEV is
> set, _scratch_mkfs can be dangerous.

As I asked earlier in this thread: why isn't get_next_config()
catching this?

> I propose this patch, which skips _scratch_cleanup_files if called by check
> 
> [root@hp-dl388eg8-01 xfstests]# git diff
> diff --git a/common/rc b/common/rc
> index 435f74f..254fb66 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -554,6 +554,11 @@ _mkfs_dev()
>  # remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS
>  _scratch_cleanup_files()
>  {
> +       # do nothing if called by check, variables are not fully valided yet
> +       # SCRATCH_MNT can be empty, which is dangerous
> +       if [ "$iam" == "check" ]; then
> +               return
> +       fi

Again, this is the wrong place to try to fix this - we haven't fixed
the landmine that has left us running with an invalid config. IOWs,
by the time _scratch_mkfs is called from *anywhere* we should have
fully validated the environment to be correct and valid. Parsing and
validating the config we have loaded from the config file is the job
of get_next_config(), yes?

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS

[Eryu Guan]

On Mon, Nov 17, 2014 at 04:41:41PM +1100, Dave Chinner wrote:
> On Sat, Nov 15, 2014 at 01:35:33PM +0800, Eryu Guan wrote:
> > On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote:
> > > On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote:
> > > > On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote:
> > > >> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > > >> > This commit disables tests requires scratch dev running on NFS
> > > >> >
> > > >> > c041421 xfstests: stop special casing nfs and udf
> > > >> >
> > > >> > Now re-enable them to get a larger test coverage on NFS.
> > > >> >
> > > >> > Signed-off-by: Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > >> > ---
> > > >> >  common/rc | 22 +++++++++++++++++++---
> > > >> >  1 file changed, 19 insertions(+), 3 deletions(-)
> > > >> >
> > > >> > diff --git a/common/rc b/common/rc
> > > >> > index 747cf72..ae03712 100644
> > > >> > --- a/common/rc
> > > >> > +++ b/common/rc
> > > >> > @@ -551,6 +551,14 @@ _mkfs_dev()
> > > >> >      rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd
> > > >> >  }
> > > >> >
> > > >> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS
> > > >> > +_scratch_cleanup_files()
> > > >> > +{
> > > >> > +       _scratch_mount
> > > >> > +       rm -rf $SCRATCH_MNT/*
> > > >> > +       _scratch_unmount
> > > >> > +}
> > > >>
> > > >> There should be a check to make sure SCRATCH_MNT exists before you
> > > >> wipe the whole disk ....
> > > >>
> > > >> so if no SCRATCH_MNT then this does rm -rf/*
> > > >> right ... (and wipes out your whole system ...)
> > > >
> > > > You can't get to that function until after all the checks that
> > > > SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that
> > > > is only called in tests after all the startup checks validate
> > > > devices and mounts exist. i.e. see common/config::get_next_config()
> > > 
> > > Well, I reproduced it easily enough again today (after taking a
> > > snapshot of the VM)
> > > by simply running generic/120 against NFS with SCRATCH_MNT not
> > > specified in local.config
> > > Dros also ran into this problem.
> > 
> > You're right, I missed that _scratch_mkfs is also called by ./check,
> > and if there's no SCRATCH_MNT set in local.config, only SCRATCH_DEV is
> > set, _scratch_mkfs can be dangerous.
> 
> As I asked earlier in this thread: why isn't get_next_config()
> catching this?

I think I see the problem, this patch catches the empty SCRATCH_MNT
and works for me:

eguan@dhcp-13-216:~/workspace/src/xfstests$ git diff
diff --git a/common/config b/common/config
index 1cb08c0..409f1b8 100644
--- a/common/config
+++ b/common/config
@@ -464,7 +464,7 @@ get_next_config() {
                exit 1
        fi
 
-       if [ ! -z "$SCRATCH_MNT" -a ! -d "$SCRATCH_MNT" ]; then
+       if [ ! -z "$SCRATCH_MNT" -o ! -d "$SCRATCH_MNT" ]; then
                echo "common/config: Error: \$SCRATCH_MNT ($SCRATCH_MNT) is not a directory"
                exit 1
        fi

If it looks sane I will send out a patch.

> 
> > I propose this patch, which skips _scratch_cleanup_files if called by check
> > 
> > [root@hp-dl388eg8-01 xfstests]# git diff
> > diff --git a/common/rc b/common/rc
> > index 435f74f..254fb66 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -554,6 +554,11 @@ _mkfs_dev()
> >  # remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS
> >  _scratch_cleanup_files()
> >  {
> > +       # do nothing if called by check, variables are not fully valided yet
> > +       # SCRATCH_MNT can be empty, which is dangerous
> > +       if [ "$iam" == "check" ]; then
> > +               return
> > +       fi
> 
> Again, this is the wrong place to try to fix this - we haven't fixed
> the landmine that has left us running with an invalid config. IOWs,
> by the time _scratch_mkfs is called from *anywhere* we should have
> fully validated the environment to be correct and valid. Parsing and
> validating the config we have loaded from the config file is the job
> of get_next_config(), yes?

Yes, that makes more sense, thanks for the explanation!

Thanks,
Eryu
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS

[Dave Chinner]

On Mon, Nov 17, 2014 at 02:06:03PM +0800, Eryu Guan wrote:
> On Mon, Nov 17, 2014 at 04:41:41PM +1100, Dave Chinner wrote:
> > On Sat, Nov 15, 2014 at 01:35:33PM +0800, Eryu Guan wrote:
> > > On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote:
> > > > On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote:
> > > > > On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote:
> > > > >> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > > > >> > This commit disables tests requires scratch dev running on NFS
> > > > >> >
> > > > >> > c041421 xfstests: stop special casing nfs and udf
> > > > >> >
> > > > >> > Now re-enable them to get a larger test coverage on NFS.
> > > > >> >
> > > > >> > Signed-off-by: Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > > >> > ---
> > > > >> >  common/rc | 22 +++++++++++++++++++---
> > > > >> >  1 file changed, 19 insertions(+), 3 deletions(-)
> > > > >> >
> > > > >> > diff --git a/common/rc b/common/rc
> > > > >> > index 747cf72..ae03712 100644
> > > > >> > --- a/common/rc
> > > > >> > +++ b/common/rc
> > > > >> > @@ -551,6 +551,14 @@ _mkfs_dev()
> > > > >> >      rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd
> > > > >> >  }
> > > > >> >
> > > > >> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS
> > > > >> > +_scratch_cleanup_files()
> > > > >> > +{
> > > > >> > +       _scratch_mount
> > > > >> > +       rm -rf $SCRATCH_MNT/*
> > > > >> > +       _scratch_unmount
> > > > >> > +}
> > > > >>
> > > > >> There should be a check to make sure SCRATCH_MNT exists before you
> > > > >> wipe the whole disk ....
> > > > >>
> > > > >> so if no SCRATCH_MNT then this does rm -rf/*
> > > > >> right ... (and wipes out your whole system ...)
> > > > >
> > > > > You can't get to that function until after all the checks that
> > > > > SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that
> > > > > is only called in tests after all the startup checks validate
> > > > > devices and mounts exist. i.e. see common/config::get_next_config()
> > > > 
> > > > Well, I reproduced it easily enough again today (after taking a
> > > > snapshot of the VM)
> > > > by simply running generic/120 against NFS with SCRATCH_MNT not
> > > > specified in local.config
> > > > Dros also ran into this problem.
> > > 
> > > You're right, I missed that _scratch_mkfs is also called by ./check,
> > > and if there's no SCRATCH_MNT set in local.config, only SCRATCH_DEV is
> > > set, _scratch_mkfs can be dangerous.
> > 
> > As I asked earlier in this thread: why isn't get_next_config()
> > catching this?
> 
> I think I see the problem, this patch catches the empty SCRATCH_MNT
> and works for me:
> 
> eguan@dhcp-13-216:~/workspace/src/xfstests$ git diff
> diff --git a/common/config b/common/config
> index 1cb08c0..409f1b8 100644
> --- a/common/config
> +++ b/common/config
> @@ -464,7 +464,7 @@ get_next_config() {
>                 exit 1
>         fi
>  
> -       if [ ! -z "$SCRATCH_MNT" -a ! -d "$SCRATCH_MNT" ]; then
> +       if [ ! -z "$SCRATCH_MNT" -o ! -d "$SCRATCH_MNT" ]; then

"if (not an empty string OR not a directory)"

That doesn't work if you define $SCRATCH_MNT - the directory check
fails on an empty string.

As it is, this isn't solving the underlying problem - that "empty
string" check is to avoid failing configs that are not configured
with a scratch device.  i.e. if you define SCRATCH_MNT, it must be a
directory. However, there's no check that if you define SCRATCH_DEV
you've also defined a SCRATCH_MNT...

IOWs, the underlying problem here is that SCRATCH_DEV is defined,
but SCRATCH_MNT is not and we are not catching that issue.  FYI, the
setup script is used for checking these sorts of things.  e.g.  with
an empty config:

$ sudo ./setup
Warning: need to define parameters for host test2
       or set variables:
               TEST_DIR TEST_DEV
$

And you'll note that it catches when we only have one of the two
necessary TEST_D* variables set.  However, even with XFS as the
probe fstyp, it doesn't error out if I don't define a SCRATCH_MNT:

$ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test SCRATCH_DEV=/dev/vdb
./setup
TEST: DIR=/mnt/test DEV=/dev/vda rt=[] log=[]
TAPE: dev=[] rmt=[] rmtirix=[@]
SCRATCH: MNT= DEV=/dev/vdb rt=[/dev/vdc] log=[/dev/vdd]
VARIABLES: external=no largeblk=no fstyp=xfs
           large_scratch_dev=no attrsecure=no
$ 

But it should fail with a sane error message indicating that we
caught the invalid config and didn't leave check to fail randomly
wiht some downstream error like:

$ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test SCRATCH_DEV=/dev/vdb ./check generic/120
....
check: failed to mount $SCRATCH_DEV using specified options
Passed all 0 tests
$

Another FYI: we can actually test any filesystem without a scratch
device configured:

$ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test ./check generic/120
FSTYP         -- xfs (non-debug)
PLATFORM      -- Linux/x86_64 test2 3.18.0-rc2-dgc+

generic/120 16s ... [not run] this test requires a valid $SCRATCH_DEV
Not run: generic/120
Passed all 0 tests
$

So, really, all we need to do is ensure that SCRATCH_DEV/SCRATCH_MNT
are sanely configured...

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org

Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS

[Omer Zilberberg]

> Another FYI: we can actually test any filesystem without a scratch
> device configured:
> 
> $ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test ./check generic/120
> FSTYP         -- xfs (non-debug)
> PLATFORM      -- Linux/x86_64 test2 3.18.0-rc2-dgc+
> 
> generic/120 16s ... [not run] this test requires a valid $SCRATCH_DEV
> Not run: generic/120
> Passed all 0 tests
> $

Please note that since commit 83ef157d, that is no longer true, because _require_test calls _is_block_dev with empty parameter, which prints usage:
$ sudo TEST_DEV=/dev/sda TEST_DIR=/mnt/dev0  ./check generic/001
FSTYP         -- ext4
PLATFORM      -- Linux/x86_64 testvm 3.17.0

generic/001 7s ... - output mismatch (see /opt/xfstests/results//generic/001.out.bad)
    --- tests/generic/001.out   2014-09-10 11:04:44.249185592 +0300
    +++ /opt/xfstests/results//generic/001.out.bad      2014-11-17 15:14:12.380061760 +0200
    @@ -1,4 +1,5 @@
     QA output created by 001
    +Usage: _is_block_dev dev
     cleanup
     setup ....................................
     iter 1 chain ... check ....................................
    ...
    (Run 'diff -u tests/generic/001.out /opt/xfstests/results//generic/001.out.bad'  to see the entire diff)
Ran: generic/001
Failures: generic/001
Failed 1 of 1 tests

Here's a patch to fix that:
----
Subject: [PATCH] _required_test: removed unneeded test for scratch_dev

testing for scratch_dev in _required_test is unnecessary, since it is
not required for these tests. Furthermore, if a scratch_dev is not given,
all tests which are supposed to pass on test_dev fail, because
_is_block_dev is given an empty string as scratch_dev, and prints usage.

Signed-off-by: Omer Zilberberg <omzg-/8YdC2HfS5554TAoqtyWWQ@public.gmane.org>
---
 common/rc | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/common/rc b/common/rc
index d5e3aff..b863808 100644
--- a/common/rc
+++ b/common/rc
@@ -1104,7 +1104,7 @@ _require_scratch()
 }
 
 
-# this test needs a test partition - check we're ok & unmount it
+# this test needs a test partition - check we're ok & mount if necessary
 #
 _require_test()
 {
@@ -1138,10 +1138,6 @@ _require_test()
                 then
                     _notrun "this test requires a valid \$TEST_DEV"
                 fi
-                if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ]
-                then
-                    _notrun "this test requires a valid \$TEST_DEV"
-                fi
                if [ ! -d "$TEST_DIR" ]
                then
                     _notrun "this test requires a valid \$TEST_DIR"
-- 
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS

[Dave Chinner]

On Mon, Nov 17, 2014 at 04:53:50PM +0200, Omer Zilberberg wrote:
> > Another FYI: we can actually test any filesystem without a scratch
> > device configured:
> > 
> > $ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test ./check generic/120
> > FSTYP         -- xfs (non-debug)
> > PLATFORM      -- Linux/x86_64 test2 3.18.0-rc2-dgc+
> > 
> > generic/120 16s ... [not run] this test requires a valid $SCRATCH_DEV
> > Not run: generic/120
> > Passed all 0 tests
> > $
> 
> Please note that since commit 83ef157d, that is no longer true, because _require_test calls _is_block_dev with empty parameter, which prints usage:
> $ sudo TEST_DEV=/dev/sda TEST_DIR=/mnt/dev0  ./check generic/001
> FSTYP         -- ext4
> PLATFORM      -- Linux/x86_64 testvm 3.17.0
> 
> generic/001 7s ... - output mismatch (see /opt/xfstests/results//generic/001.out.bad)
>     --- tests/generic/001.out   2014-09-10 11:04:44.249185592 +0300
>     +++ /opt/xfstests/results//generic/001.out.bad      2014-11-17 15:14:12.380061760 +0200
>     @@ -1,4 +1,5 @@
>      QA output created by 001
>     +Usage: _is_block_dev dev
>      cleanup
>      setup ....................................
>      iter 1 chain ... check ....................................
>     ...
>     (Run 'diff -u tests/generic/001.out /opt/xfstests/results//generic/001.out.bad'  to see the entire diff)
> Ran: generic/001
> Failures: generic/001
> Failed 1 of 1 tests
> 
> Here's a patch to fix that:
> ----
> Subject: [PATCH] _required_test: removed unneeded test for scratch_dev
> 
> testing for scratch_dev in _required_test is unnecessary, since it is
> not required for these tests. Furthermore, if a scratch_dev is not given,
> all tests which are supposed to pass on test_dev fail, because
> _is_block_dev is given an empty string as scratch_dev, and prints usage.

I'm beginning to sound like a broken record. Why isn't this *invalid
config check* being done in the config parsing rather than on every
test that requires a scratch or test device?

i.e. removing the check is not the correct fix - checking it during
config parsing means none of these checks need to be done in
_require_test, nor in _require_scratch_nocheck.

i.e. in get_next_config() we already know the FSTYP, so we should
be checking that:

	a) TEST_DEV is valid for fstyp
	b) SCRATCH_DEV (if configured) is valid for fstyp
	c) TEST_DEV != SCRATCH_DEV if they both exist and fstyp
	   indicates they should be block devices

And then we can pretty much assume that they are valid everywhere
else, and _require_scratch_nocheck() simply needs to check for a
non-empty string...

> Signed-off-by: Omer Zilberberg <omzg-/8YdC2HfS5554TAoqtyWWQ@public.gmane.org>
> ---
>  common/rc | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index d5e3aff..b863808 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1104,7 +1104,7 @@ _require_scratch()
>  }
>  
>  
> -# this test needs a test partition - check we're ok & unmount it
> +# this test needs a test partition - check we're ok & mount if necessary
>  #
>  _require_test()
>  {
> @@ -1138,10 +1138,6 @@ _require_test()
>                  then
>                      _notrun "this test requires a valid \$TEST_DEV"
>                  fi
> -                if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ]
> -                then
> -                    _notrun "this test requires a valid \$TEST_DEV"
> -                fi
>                 if [ ! -d "$TEST_DIR" ]
>                 then
>                      _notrun "this test requires a valid \$TEST_DIR"

The patch is whitespace damaged. Please read:

https://www.kernel.org/doc/Documentation/email-clients.txt

and resend.

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org

Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS

[Omer Zilberberg]

On 11/17/2014 10:22 PM, Dave Chinner wrote:
> On Mon, Nov 17, 2014 at 04:53:50PM +0200, Omer Zilberberg wrote:
>>> Another FYI: we can actually test any filesystem without a scratch
>>> device configured:
>>>
>>> $ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test ./check generic/120
>>> FSTYP         -- xfs (non-debug)
>>> PLATFORM      -- Linux/x86_64 test2 3.18.0-rc2-dgc+
>>>
>>> generic/120 16s ... [not run] this test requires a valid $SCRATCH_DEV
>>> Not run: generic/120
>>> Passed all 0 tests
>>> $
>>
>> Please note that since commit 83ef157d, that is no longer true, because _require_test calls _is_block_dev with empty parameter, which prints usage:
>> $ sudo TEST_DEV=/dev/sda TEST_DIR=/mnt/dev0  ./check generic/001
>> FSTYP         -- ext4
>> PLATFORM      -- Linux/x86_64 testvm 3.17.0
>>
>> generic/001 7s ... - output mismatch (see /opt/xfstests/results//generic/001.out.bad)
>>     --- tests/generic/001.out   2014-09-10 11:04:44.249185592 +0300
>>     +++ /opt/xfstests/results//generic/001.out.bad      2014-11-17 15:14:12.380061760 +0200
>>     @@ -1,4 +1,5 @@
>>      QA output created by 001
>>     +Usage: _is_block_dev dev
>>      cleanup
>>      setup ....................................
>>      iter 1 chain ... check ....................................
>>     ...
>>     (Run 'diff -u tests/generic/001.out /opt/xfstests/results//generic/001.out.bad'  to see the entire diff)
>> Ran: generic/001
>> Failures: generic/001
>> Failed 1 of 1 tests
>>
>> Here's a patch to fix that:
>> ----
>> Subject: [PATCH] _required_test: removed unneeded test for scratch_dev
>>
>> testing for scratch_dev in _required_test is unnecessary, since it is
>> not required for these tests. Furthermore, if a scratch_dev is not given,
>> all tests which are supposed to pass on test_dev fail, because
>> _is_block_dev is given an empty string as scratch_dev, and prints usage.
> 
> I'm beginning to sound like a broken record. Why isn't this *invalid
> config check* being done in the config parsing rather than on every
> test that requires a scratch or test device?
> 
> i.e. removing the check is not the correct fix - checking it during
> config parsing means none of these checks need to be done in
> _require_test, nor in _require_scratch_nocheck.
> 
> i.e. in get_next_config() we already know the FSTYP, so we should
> be checking that:
> 
> 	a) TEST_DEV is valid for fstyp
> 	b) SCRATCH_DEV (if configured) is valid for fstyp
> 	c) TEST_DEV != SCRATCH_DEV if they both exist and fstyp
> 	   indicates they should be block devices
> 
> And then we can pretty much assume that they are valid everywhere
> else, and _require_scratch_nocheck() simply needs to check for a
> non-empty string...
> 
>> Signed-off-by: Omer Zilberberg <omzg-/8YdC2HfS5554TAoqtyWWQ@public.gmane.org>
>> ---
>>  common/rc | 6 +-----
>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/common/rc b/common/rc
>> index d5e3aff..b863808 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -1104,7 +1104,7 @@ _require_scratch()
>>  }
>>  
>>  
>> -# this test needs a test partition - check we're ok & unmount it
>> +# this test needs a test partition - check we're ok & mount if necessary
>>  #
>>  _require_test()
>>  {
>> @@ -1138,10 +1138,6 @@ _require_test()
>>                  then
>>                      _notrun "this test requires a valid \$TEST_DEV"
>>                  fi
>> -                if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ]
>> -                then
>> -                    _notrun "this test requires a valid \$TEST_DEV"
>> -                fi
>>                 if [ ! -d "$TEST_DIR" ]
>>                 then
>>                      _notrun "this test requires a valid \$TEST_DIR"
> 
> The patch is whitespace damaged. Please read:
> 
> https://www.kernel.org/doc/Documentation/email-clients.txt

Here's the patch resent with whitespace fixed, at least as a temporary solution...
----
Subject: [PATCH] _required_test: removed unneeded test for scratch_dev

testing for scratch_dev in _required_test is unnecessary, since it is
not required for these tests. Furthermore, if a scratch_dev is not given,
all tests which are supposed to pass on test_dev fail, because
_is_block_dev is given an empty string as scratch_dev, and prints usage.

Signed-off-by: Omer Zilberberg <omzg-/8YdC2HfS5554TAoqtyWWQ@public.gmane.org>
---
 common/rc | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/common/rc b/common/rc
index d5e3aff..b863808 100644
--- a/common/rc
+++ b/common/rc
@@ -1104,7 +1104,7 @@ _require_scratch()
 }
 
 
-# this test needs a test partition - check we're ok & unmount it
+# this test needs a test partition - check we're ok & mount if necessary
 #
 _require_test()
 {
@@ -1138,10 +1138,6 @@ _require_test()
 		 then
 		     _notrun "this test requires a valid \$TEST_DEV"
 		 fi
-		 if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ]
-		 then
-		     _notrun "this test requires a valid \$TEST_DEV"
-		 fi
 		if [ ! -d "$TEST_DIR" ]
 		then
 		     _notrun "this test requires a valid \$TEST_DIR"

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS

[Dave Chinner]

On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote:
> On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote:
> > On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote:
> >> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >> > This commit disables tests requires scratch dev running on NFS
> >> >
> >> > c041421 xfstests: stop special casing nfs and udf
> >> >
> >> > Now re-enable them to get a larger test coverage on NFS.
> >> >
> >> > Signed-off-by: Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> > ---
> >> >  common/rc | 22 +++++++++++++++++++---
> >> >  1 file changed, 19 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/common/rc b/common/rc
> >> > index 747cf72..ae03712 100644
> >> > --- a/common/rc
> >> > +++ b/common/rc
> >> > @@ -551,6 +551,14 @@ _mkfs_dev()
> >> >      rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd
> >> >  }
> >> >
> >> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS
> >> > +_scratch_cleanup_files()
> >> > +{
> >> > +       _scratch_mount
> >> > +       rm -rf $SCRATCH_MNT/*
> >> > +       _scratch_unmount
> >> > +}
> >>
> >> There should be a check to make sure SCRATCH_MNT exists before you
> >> wipe the whole disk ....
> >>
> >> so if no SCRATCH_MNT then this does rm -rf/*
> >> right ... (and wipes out your whole system ...)
> >
> > You can't get to that function until after all the checks that
> > SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that
> > is only called in tests after all the startup checks validate
> > devices and mounts exist. i.e. see common/config::get_next_config()
> 
> Well, I reproduced it easily enough again today (after taking a
> snapshot of the VM)
> by simply running generic/120 against NFS with SCRATCH_MNT not
> specified in local.config
> Dros also ran into this problem.

tests/generic/120 does this:

....
_require_scratch
_scratch_mkfs ....

So we call:

_require_scratch
  _require_scratch_nocheck

	case "$FSTYP" in
        nfs*)                                                                    
                echo $SCRATCH_DEV | grep -q ":/" > /dev/null 2>&1                
                if [ -z "$SCRATCH_DEV" -o "$?" != "0" ]; then                    
                        _notrun "this test requires a valid \$SCRATCH_DEV"       
                fi                                                               
                if [ ! -d "$SCRATCH_MNT" ]; then                                 
                        _notrun "this test requires a valid \$SCRATCH_MNT"       
                fi                                                               

If $SCRATCH_MNT is empty, then it should fail right there. An empty
string gives:

$ if [ ! -d "" ]; then
> echo foo
> fi
foo
$

before it gets anywhere near _scratch_mkfs. So, why isn't
generic/120 aborting during the _require_scratch check?

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org