On Mon, Apr 11, 2022 at 03:31:35PM +1000, Dave Chinner wrote:
> On Sun, Apr 10, 2022 at 08:11:03PM +0800, Zorro Lang wrote:
> > On Fri, Apr 08, 2022 at 09:00:56PM -0700, Darrick J. Wong wrote:
> > > On Sat, Apr 09, 2022 at 01:08:47AM +0800, Zorro Lang wrote:
> > > > From: Eric Sandeen <sandeen@xxxxxxxxxx>
> > > >
> > > > This tests a longstanding bug where xfsdumps are not properly
> > > > created when an inode is present on the filesytsem which has
> > > > a lower number than the root inode.
> > > >
> > > > Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> > > > Signed-off-by: Zorro Lang <zlang@xxxxxxxxxx>
> > > > ---
> > > >
> > > > Friendly ping for reviewing.
> > > >
> > > > Thanks,
> > > > Zorro
> > > >
> > > > common/dump | 1 +
> > > > tests/xfs/543 | 63 +++++++++++++++++++++++++++++++++++++++++++++++
> > > > tests/xfs/543.out | 47 +++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 111 insertions(+)
> > > > create mode 100755 tests/xfs/543
> > > > create mode 100644 tests/xfs/543.out
> > > >
> > > > diff --git a/common/dump b/common/dump
> > > > index ea16d442..ab48cd13 100644
> > > > --- a/common/dump
> > > > +++ b/common/dump
> > > > @@ -219,6 +219,7 @@ _require_tape()
> > > >
> > > > _wipe_fs()
> > > > {
> > > > + [[ "$WIPE_FS" = "no" ]] && return
> > >
> > > Is WIPE_FS intended as a general means to prevent wipefs -a between
> > > tests? If so, it ought to be in the README.
> >
> > Actually I don't know how to if I should say WIPE_FS means to prevent
> > wipefs -a between tests. Due to:
> >
> > 1) We have a _wipe_fs() in common/dump[1], it's very old, and it only
> > try to make a new xfs, although it calls wipe "fs"... And it's only used
> > for tests/xfs/* cases to call _create_dumpdir_*() function.
> >
> > 2) Then we have _try_wipe_scratch_devs() in common/rc [2], which calls
> > _try_wipe_scratch_xfs() in common/xfs [3] (which is newer), it's only
> > used in ./check script now.
> >
> > So I'm wondering if we'd better to change the _wipe_fs() to:
> > _wipe_dump()
> > {
> > [[ "$WIPE_DUMP" = "no" ]] && return
> > _require_scratch
> >
> > _try_wipe_scratch_devs >>$seqres.full
> > _scratch_mount >>$seqres.full
> > }
> >
> > What do you think?
>
> common/dump::_wipe_fs() is effectively just _scratch_mkfs() followed
> by mounting it. It does not need to exist at all.
Oh! I was really confused why dumpdir need to wipefs at first, so that's
the reason. I just checked all cases which call _create_dumpdir_*, and
yes, they just hope to omit below common steps by running _create_dumpdir_*:
_require_scratch
_scratch_mkfs
_scratch_mount
I'd like to remove the common/dump::_wipe_fs(), and help all related cases
call _require_scratch && _scratch_mkfs && _scratch_mount by themselves. Due
to it's match the current xfstests case format.
Is that good to you? Anymore suggestions or notice ?
Thanks,
Zorro
>
> The test(s) need to have run _require_scratch() in it's setup, it
> shouldn't be hidden deep inside random functions that fill the
> scratch device.
>
> They should also run scratch_mkfs/mount themselves to prep the
> scratch device, and then _wipe_fs() can go away entirely.
>
> And the new test simply calls the function to populate the scratch
> device at the place where you want to preserve the previous contents
> of the scratch device without first running scratch_mkfs....
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
