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.
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
