On Mon, Apr 11, 2022 at 04:34:33PM +0800, Zorro Lang wrote:
> 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.
Aha!
> 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 ?
Sounds good to /me.
--D
> 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
> >
>
