[11/12] check: wipe scratch devices between tests

Commit Message

Darrick J. Wong March 20, 2019, 12:45 a.m. UTC

From: Darrick J. Wong <[e-mail]>

Wipe the scratch devices in between each test to ensure that tests are
formatting them and not making assumptions about previous contents.

Signed-off-by: Darrick J. Wong <[e-mail]>
---
 check      |    1 +
 common/rc  |    6 ++++++
 common/xfs |    1 +
 3 files changed, 8 insertions(+)

Comments

Amir Goldstein March 20, 2019, 7:06 a.m. UTC | #1

On Wed, Mar 20, 2019 at 2:46 AM Darrick J. Wong <[e-mail]> wrote:
>
> From: Darrick J. Wong <[e-mail]>
>
> Wipe the scratch devices in between each test to ensure that tests are
> formatting them and not making assumptions about previous contents.
>
> Signed-off-by: Darrick J. Wong <[e-mail]>
> ---
>  check      |    1 +
>  common/rc  |    6 ++++++
>  common/xfs |    1 +
>  3 files changed, 8 insertions(+)
>
>
> diff --git a/check b/check
> index a2c5ba21..bcf08dfe 100755
> --- a/check
> +++ b/check
> @@ -737,6 +737,7 @@ for section in $HOST_OPTIONS_SECTIONS; do
>                         # _check_dmesg depends on this log in dmesg
>                         touch ${RESULT_DIR}/check_dmesg
>                 fi
> +               _try_wipe_scratch_devs > /dev/null 2>&1
>                 if [ "$DUMP_OUTPUT" = true ]; then
>                         ./$seq 2>&1 | tee $tmp.out
>                         # Because $? would get tee's return code
> diff --git a/common/rc b/common/rc
> index 1c42515f..40eef80f 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3942,6 +3942,12 @@ _require_fibmap()
>         rm -f $file
>  }
>
> +_try_wipe_scratch_devs()
> +{
> +       _scratch_unmount

So I find this change strange, not because it doesn't make sense
to start every test with scratch unmounted, but because today scratch *is*
mounted on test start and there is quite a bit of code to make it mounted
on test start, which seems backwards.

IOW, instead of unmounting scratch just before the test runs, seems
better to make sure it is unmounted at the end of each test and before
starting the loop.

Note that for a test with _require_scratch_nocheck, _check_filesystems()
will unmount scratch, but for a test with _require_scratch, _check_scratch_fs()
will unmount+fsck+mount scratch - inconsistent.
Or am I reading this wrong?


> +       test -x "$WIPEFS_PROG" && $WIPEFS_PROG -a $SCRATCH_DEV_POOL $SCRATCH_DEV $SCRATCH_LOGDEV $SCRATCH_RTDEV

Please check that $SCRATCH_DEV is not a network name (nfs|cifs) and that it
is really a blockdev (overlayfs) before trying to wipefs.

Thanks,
Amir.

Darrick J. Wong March 21, 2019, 4:15 a.m. UTC | #2

On Wed, Mar 20, 2019 at 09:06:08AM +0200, Amir Goldstein wrote:
> On Wed, Mar 20, 2019 at 2:46 AM Darrick J. Wong <[e-mail]> wrote:
> >
> > From: Darrick J. Wong <[e-mail]>
> >
> > Wipe the scratch devices in between each test to ensure that tests are
> > formatting them and not making assumptions about previous contents.
> >
> > Signed-off-by: Darrick J. Wong <[e-mail]>
> > ---
> >  check      |    1 +
> >  common/rc  |    6 ++++++
> >  common/xfs |    1 +
> >  3 files changed, 8 insertions(+)
> >
> >
> > diff --git a/check b/check
> > index a2c5ba21..bcf08dfe 100755
> > --- a/check
> > +++ b/check
> > @@ -737,6 +737,7 @@ for section in $HOST_OPTIONS_SECTIONS; do
> >                         # _check_dmesg depends on this log in dmesg
> >                         touch ${RESULT_DIR}/check_dmesg
> >                 fi
> > +               _try_wipe_scratch_devs > /dev/null 2>&1
> >                 if [ "$DUMP_OUTPUT" = true ]; then
> >                         ./$seq 2>&1 | tee $tmp.out
> >                         # Because $? would get tee's return code
> > diff --git a/common/rc b/common/rc
> > index 1c42515f..40eef80f 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -3942,6 +3942,12 @@ _require_fibmap()
> >         rm -f $file
> >  }
> >
> > +_try_wipe_scratch_devs()
> > +{
> > +       _scratch_unmount
> 
> So I find this change strange, not because it doesn't make sense
> to start every test with scratch unmounted, but because today scratch *is*
> mounted on test start and there is quite a bit of code to make it mounted
> on test start, which seems backwards.

Huh?  On XFS the test device is always mounted, but the scratch device
is never mounted before the test starts.

Hmmm, maybe this is one of those weird things that different with
nonblock filesystems...?

> IOW, instead of unmounting scratch just before the test runs, seems
> better to make sure it is unmounted at the end of each test and before
> starting the loop.
> 
> Note that for a test with _require_scratch_nocheck, _check_filesystems()
> will unmount scratch, but for a test with _require_scratch, _check_scratch_fs()
> will unmount+fsck+mount scratch - inconsistent.
> Or am I reading this wrong?
> 
> 
> > +       test -x "$WIPEFS_PROG" && $WIPEFS_PROG -a $SCRATCH_DEV_POOL $SCRATCH_DEV $SCRATCH_LOGDEV $SCRATCH_RTDEV
> 
> Please check that $SCRATCH_DEV is not a network name (nfs|cifs) and that it
> is really a blockdev (overlayfs) before trying to wipefs.

I wonder if maybe we only bother with wipefs for filesystems where
all tests have been fixed to not stumble over unformatted scratchdev
(i.e. xfs and ext* only)

--D

> Thanks,
> Amir.

Eryu Guan March 23, 2019, 1:29 p.m. UTC | #3

On Wed, Mar 20, 2019 at 09:15:44PM -0700, Darrick J. Wong wrote:
> On Wed, Mar 20, 2019 at 09:06:08AM +0200, Amir Goldstein wrote:
> > On Wed, Mar 20, 2019 at 2:46 AM Darrick J. Wong <[e-mail]> wrote:
> > >
> > > From: Darrick J. Wong <[e-mail]>
> > >
> > > Wipe the scratch devices in between each test to ensure that tests are
> > > formatting them and not making assumptions about previous contents.
> > >
> > > Signed-off-by: Darrick J. Wong <[e-mail]>
> > > ---
> > >  check      |    1 +
> > >  common/rc  |    6 ++++++
> > >  common/xfs |    1 +
> > >  3 files changed, 8 insertions(+)
> > >
> > >
> > > diff --git a/check b/check
> > > index a2c5ba21..bcf08dfe 100755
> > > --- a/check
> > > +++ b/check
> > > @@ -737,6 +737,7 @@ for section in $HOST_OPTIONS_SECTIONS; do
> > >                         # _check_dmesg depends on this log in dmesg
> > >                         touch ${RESULT_DIR}/check_dmesg
> > >                 fi
> > > +               _try_wipe_scratch_devs > /dev/null 2>&1
> > >                 if [ "$DUMP_OUTPUT" = true ]; then
> > >                         ./$seq 2>&1 | tee $tmp.out
> > >                         # Because $? would get tee's return code
> > > diff --git a/common/rc b/common/rc
> > > index 1c42515f..40eef80f 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -3942,6 +3942,12 @@ _require_fibmap()
> > >         rm -f $file
> > >  }
> > >
> > > +_try_wipe_scratch_devs()
> > > +{
> > > +       _scratch_unmount
> > 
> > So I find this change strange, not because it doesn't make sense
> > to start every test with scratch unmounted, but because today scratch *is*
> > mounted on test start and there is quite a bit of code to make it mounted
> > on test start, which seems backwards.
> 
> Huh?  On XFS the test device is always mounted, but the scratch device
> is never mounted before the test starts.

Yes, as _require_test always mounts TEST_DEV and
_require_scratch_nocheck always umounts SCRATCH_DEV.

> 
> Hmmm, maybe this is one of those weird things that different with
> nonblock filesystems...?
> 
> > IOW, instead of unmounting scratch just before the test runs, seems
> > better to make sure it is unmounted at the end of each test and before
> > starting the loop.
> > 
> > Note that for a test with _require_scratch_nocheck, _check_filesystems()
> > will unmount scratch, but for a test with _require_scratch, _check_scratch_fs()
> > will unmount+fsck+mount scratch - inconsistent.
> > Or am I reading this wrong?

For tests that don't call _require_scratch{_nocheck}, to make sure
SCRATCH_DEV is umounted I think we could just call _scratch_unmount
unconditionally in _check_filesystems(), then we could wipefs before
test safely.

> > 
> > 
> > > +       test -x "$WIPEFS_PROG" && $WIPEFS_PROG -a $SCRATCH_DEV_POOL $SCRATCH_DEV $SCRATCH_LOGDEV $SCRATCH_RTDEV
> > 
> > Please check that $SCRATCH_DEV is not a network name (nfs|cifs) and that it
> > is really a blockdev (overlayfs) before trying to wipefs.

Yes, good catch!

Thanks,
Eryu

> 
> I wonder if maybe we only bother with wipefs for filesystems where
> all tests have been fixed to not stumble over unformatted scratchdev
> (i.e. xfs and ext* only)
> 
> --D
> 
> > Thanks,
> > Amir.

Darrick J. Wong March 25, 2019, 11:48 p.m. UTC | #4

On Sat, Mar 23, 2019 at 09:29:48PM +0800, Eryu Guan wrote:
> On Wed, Mar 20, 2019 at 09:15:44PM -0700, Darrick J. Wong wrote:
> > On Wed, Mar 20, 2019 at 09:06:08AM +0200, Amir Goldstein wrote:
> > > On Wed, Mar 20, 2019 at 2:46 AM Darrick J. Wong <[e-mail]> wrote:
> > > >
> > > > From: Darrick J. Wong <[e-mail]>
> > > >
> > > > Wipe the scratch devices in between each test to ensure that tests are
> > > > formatting them and not making assumptions about previous contents.
> > > >
> > > > Signed-off-by: Darrick J. Wong <[e-mail]>
> > > > ---
> > > >  check      |    1 +
> > > >  common/rc  |    6 ++++++
> > > >  common/xfs |    1 +
> > > >  3 files changed, 8 insertions(+)
> > > >
> > > >
> > > > diff --git a/check b/check
> > > > index a2c5ba21..bcf08dfe 100755
> > > > --- a/check
> > > > +++ b/check
> > > > @@ -737,6 +737,7 @@ for section in $HOST_OPTIONS_SECTIONS; do
> > > >                         # _check_dmesg depends on this log in dmesg
> > > >                         touch ${RESULT_DIR}/check_dmesg
> > > >                 fi
> > > > +               _try_wipe_scratch_devs > /dev/null 2>&1
> > > >                 if [ "$DUMP_OUTPUT" = true ]; then
> > > >                         ./$seq 2>&1 | tee $tmp.out
> > > >                         # Because $? would get tee's return code
> > > > diff --git a/common/rc b/common/rc
> > > > index 1c42515f..40eef80f 100644
> > > > --- a/common/rc
> > > > +++ b/common/rc
> > > > @@ -3942,6 +3942,12 @@ _require_fibmap()
> > > >         rm -f $file
> > > >  }
> > > >
> > > > +_try_wipe_scratch_devs()
> > > > +{
> > > > +       _scratch_unmount
> > > 
> > > So I find this change strange, not because it doesn't make sense
> > > to start every test with scratch unmounted, but because today scratch *is*
> > > mounted on test start and there is quite a bit of code to make it mounted
> > > on test start, which seems backwards.
> > 
> > Huh?  On XFS the test device is always mounted, but the scratch device
> > is never mounted before the test starts.
> 
> Yes, as _require_test always mounts TEST_DEV and
> _require_scratch_nocheck always umounts SCRATCH_DEV.

Heh, both of you were right and I was wrong, we format and mount the
scratch device before we start the loop.

> > 
> > Hmmm, maybe this is one of those weird things that different with
> > nonblock filesystems...?
> > 
> > > IOW, instead of unmounting scratch just before the test runs, seems
> > > better to make sure it is unmounted at the end of each test and before
> > > starting the loop.
> > > 
> > > Note that for a test with _require_scratch_nocheck, _check_filesystems()
> > > will unmount scratch, but for a test with _require_scratch, _check_scratch_fs()
> > > will unmount+fsck+mount scratch - inconsistent.
> > > Or am I reading this wrong?
> 
> For tests that don't call _require_scratch{_nocheck}, to make sure
> SCRATCH_DEV is umounted I think we could just call _scratch_unmount
> unconditionally in _check_filesystems(), then we could wipefs before
> test safely.

I think it does this already:

	if [ -f ${RESULT_DIR}/require_scratch ]; then
		_check_scratch_fs || err=true
		rm -f ${RESULT_DIR}/require_scratch*
	else
		_scratch_unmount 2> /dev/null
	fi

Right?  So I think we're covered for unmounting the scratch device after
each test, and it's just the first time through the loop where we have
to unmount the scratch device.

--D

> 
> > > 
> > > 
> > > > +       test -x "$WIPEFS_PROG" && $WIPEFS_PROG -a $SCRATCH_DEV_POOL $SCRATCH_DEV $SCRATCH_LOGDEV $SCRATCH_RTDEV
> > > 
> > > Please check that $SCRATCH_DEV is not a network name (nfs|cifs) and that it
> > > is really a blockdev (overlayfs) before trying to wipefs.
> 
> Yes, good catch!
> 
> Thanks,
> Eryu
> 
> > 
> > I wonder if maybe we only bother with wipefs for filesystems where
> > all tests have been fixed to not stumble over unformatted scratchdev
> > (i.e. xfs and ext* only)
> > 
> > --D
> > 
> > > Thanks,
> > > Amir.

Amir Goldstein March 26, 2019, 4:24 p.m. UTC | #5

On Tue, Mar 26, 2019 at 1:48 AM Darrick J. Wong <[e-mail]> wrote:
>
> On Sat, Mar 23, 2019 at 09:29:48PM +0800, Eryu Guan wrote:
> > On Wed, Mar 20, 2019 at 09:15:44PM -0700, Darrick J. Wong wrote:
> > > On Wed, Mar 20, 2019 at 09:06:08AM +0200, Amir Goldstein wrote:
> > > > On Wed, Mar 20, 2019 at 2:46 AM Darrick J. Wong <[e-mail]> wrote:
> > > > >
> > > > > From: Darrick J. Wong <[e-mail]>
> > > > >
> > > > > Wipe the scratch devices in between each test to ensure that tests are
> > > > > formatting them and not making assumptions about previous contents.
> > > > >
> > > > > Signed-off-by: Darrick J. Wong <[e-mail]>
> > > > > ---
> > > > >  check      |    1 +
> > > > >  common/rc  |    6 ++++++
> > > > >  common/xfs |    1 +
> > > > >  3 files changed, 8 insertions(+)
> > > > >
> > > > >
> > > > > diff --git a/check b/check
> > > > > index a2c5ba21..bcf08dfe 100755
> > > > > --- a/check
> > > > > +++ b/check
> > > > > @@ -737,6 +737,7 @@ for section in $HOST_OPTIONS_SECTIONS; do
> > > > >                         # _check_dmesg depends on this log in dmesg
> > > > >                         touch ${RESULT_DIR}/check_dmesg
> > > > >                 fi
> > > > > +               _try_wipe_scratch_devs > /dev/null 2>&1
> > > > >                 if [ "$DUMP_OUTPUT" = true ]; then
> > > > >                         ./$seq 2>&1 | tee $tmp.out
> > > > >                         # Because $? would get tee's return code
> > > > > diff --git a/common/rc b/common/rc
> > > > > index 1c42515f..40eef80f 100644
> > > > > --- a/common/rc
> > > > > +++ b/common/rc
> > > > > @@ -3942,6 +3942,12 @@ _require_fibmap()
> > > > >         rm -f $file
> > > > >  }
> > > > >
> > > > > +_try_wipe_scratch_devs()
> > > > > +{
> > > > > +       _scratch_unmount
> > > >
> > > > So I find this change strange, not because it doesn't make sense
> > > > to start every test with scratch unmounted, but because today scratch *is*
> > > > mounted on test start and there is quite a bit of code to make it mounted
> > > > on test start, which seems backwards.
> > >
> > > Huh?  On XFS the test device is always mounted, but the scratch device
> > > is never mounted before the test starts.
> >
> > Yes, as _require_test always mounts TEST_DEV and
> > _require_scratch_nocheck always umounts SCRATCH_DEV.
>
> Heh, both of you were right and I was wrong, we format and mount the
> scratch device before we start the loop.
>
> > >
> > > Hmmm, maybe this is one of those weird things that different with
> > > nonblock filesystems...?
> > >
> > > > IOW, instead of unmounting scratch just before the test runs, seems
> > > > better to make sure it is unmounted at the end of each test and before
> > > > starting the loop.
> > > >
> > > > Note that for a test with _require_scratch_nocheck, _check_filesystems()
> > > > will unmount scratch, but for a test with _require_scratch, _check_scratch_fs()
> > > > will unmount+fsck+mount scratch - inconsistent.
> > > > Or am I reading this wrong?
> >
> > For tests that don't call _require_scratch{_nocheck}, to make sure
> > SCRATCH_DEV is umounted I think we could just call _scratch_unmount
> > unconditionally in _check_filesystems(), then we could wipefs before
> > test safely.
>
> I think it does this already:
>
>         if [ -f ${RESULT_DIR}/require_scratch ]; then
>                 _check_scratch_fs || err=true
>                 rm -f ${RESULT_DIR}/require_scratch*
>         else
>                 _scratch_unmount 2> /dev/null
>         fi
>
> Right?  So I think we're covered for unmounting the scratch device after
> each test, and it's just the first time through the loop where we have
> to unmount the scratch device.
>

Right about _require_scratch_nocheck and no _require_scratch.
Test with _require_scratch will call _check_filesystems().
_check_scratch_fs() will unmount scratch; fsck; mount scratch.
So as far as I can see, its not only the first time through the loop.

Thanks,
Amir.

Patch

diff --git a/check b/check
index a2c5ba21..bcf08dfe 100755
--- a/check
+++ b/check
@@ -737,6 +737,7 @@  for section in $HOST_OPTIONS_SECTIONS; do
 			# _check_dmesg depends on this log in dmesg
 			touch ${RESULT_DIR}/check_dmesg
 		fi
+		_try_wipe_scratch_devs > /dev/null 2>&1
 		if [ "$DUMP_OUTPUT" = true ]; then
 			./$seq 2>&1 | tee $tmp.out
 			# Because $? would get tee's return code
diff --git a/common/rc b/common/rc
index 1c42515f..40eef80f 100644
--- a/common/rc
+++ b/common/rc
@@ -3942,6 +3942,12 @@  _require_fibmap()
 	rm -f $file
 }
 
+_try_wipe_scratch_devs()
+{
+	_scratch_unmount
+	test -x "$WIPEFS_PROG" && $WIPEFS_PROG -a $SCRATCH_DEV_POOL $SCRATCH_DEV $SCRATCH_LOGDEV $SCRATCH_RTDEV
+}
+
 init_rc
 
 ################################################################################
diff --git a/common/xfs b/common/xfs
index 24065813..af2b62ba 100644
--- a/common/xfs
+++ b/common/xfs
@@ -295,6 +295,7 @@  _require_xfs_db_command()
 	fi
 	command=$1
 
+	_scratch_mkfs_xfs >/dev/null 2>&1
 	_scratch_xfs_db -x -c "help" | grep $command > /dev/null || \
 		_notrun "xfs_db $command support is missing"
 }