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

Darrick J. Wong
2019-03-21T04:15:51Z

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 <darrick.wong@oracle.com> wrote:
> >
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > 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 <darrick.wong@oracle.com>
> > ---
> > 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.