Re: [PATCH 3/8] xfs/*: clean up _cleanup override

[Dave Chinner <david@fromorbit.com>]

On Tue, May 24, 2022 at 01:42:15PM +0300, Amir Goldstein wrote:
> On Tue, May 24, 2022 at 12:41 PM Dave Chinner <david@fromorbit.com> wrote:
> >
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > Use a local cleanup function and _register_cleanup properly.
> >
> > Remove local cleanups that just do what the standard _cleanup and
> > test exist does.
> >
> > Remove local cleanups that just remove test files and then do
> > standard _cleanup functionality.
> 
> As I mentioned in response to the cover letter, the call to _cleanup()
> can and I think should be chained implicitly, but I am still waiting
> for your response regarding the tests where generic _cleanup is not
> desired.

Please go an look at the lore archive if vger is having problems
delivering to you - the whole thread is there, as are all my
responses.

[PATCH 6/8] fstests: consolidate no cleanup test setup

https://lore.kernel.org/fstests/20220524073411.1943480-7-david@fromorbit.com/T/#u


> > diff --git a/tests/xfs/006 b/tests/xfs/006
> > index cecceaa3..fd8d8071 100755
> > --- a/tests/xfs/006
> > +++ b/tests/xfs/006
> > @@ -11,11 +11,10 @@
> >  _begin_fstest auto quick mount eio
> >
> >  # Override the default cleanup function.
> > -_cleanup()
> > +_dmerror_cleanup()
> >  {
> > -       cd /
> > -       rm -f $tmp.*
> >         _dmerror_cleanup
> 
> That looks recursive.
> Again, if the call to _cleanup is implicitly chained then
> the local function is not needed and trap can call
> the global _dmerror_cleanup().

Yes, the local name should have been _dm_error_cleanup(), like I
used elsewhere.

> > diff --git a/tests/xfs/008 b/tests/xfs/008
> > index a53f6c92..5bef44bb 100755
> > --- a/tests/xfs/008
> > +++ b/tests/xfs/008
> > @@ -12,13 +12,6 @@ _begin_fstest rw ioctl auto quick
> >  status=0       # success is the default!
> >  pgsize=`$here/src/feature -s`
> >
> > -# Override the default cleanup function.
> > -_cleanup()
> > -{
> > -    rm -f $tmp.*
> > -    rm -rf $TEST_DIR/randholes.$$.*
> 
> It's fine to keep the test files behind, but maybe
> make that change more clear in commit message
> because it was not clear to me that there was a change of
> behavior when I read the commit message.
> 
> Sorry to drop this extra burden on you, but this seems
> important to me and I cannot add this information after the fact.

The  point of TEST_DIR is it gathers cruft from tests so exercises
filesystem aging. If the file is tiny, there's no point in removing
it - all we have to do is ensure it is removed in the test setup so
we have known initial state. So If that's the case, I removed the
cleanup of it altogether.

Like I said, I have time to do a single classification pass across
all these tests, but if I have to document every single little
change I make in doing these cleanups, I'm not going to bother
trying because it's too hard and takes too long.

Do you want perfect commits or do you want better infrastructure?

> Also, it was very hard to see in this review if the test files
> that are not cleaned in cleanup() are cleaned in the beginning
> of the test or if it is not important to clean them because they
> are overwritten.

All tests are required to control their initial state on the test
device as many tests use the same file names and locations and there
is no guarantee that the test device is clean. Hence tests must
clean up during startup to ensure they have a known initial state
to begin the test from.

> I did my best to try and I think there is a missing cleanup of
> ${OUTPUT_DIR} in the beginning of xfs/253.
> Although that may already be considered a test bug, removing
> the cleanup of ${OUTPUT_DIR} from cleanup() will expose this bug.

OUTPUT_DIR="${SCRATCH_MNT}/test_${seq}"

No cleanup is needed for tests that use the scratch device.

> > diff --git a/tests/xfs/051 b/tests/xfs/051
> > index ea70cb50..4718099d 100755
> > --- a/tests/xfs/051
> > +++ b/tests/xfs/051
> > @@ -11,14 +11,13 @@
> >  . ./common/preamble
> >  _begin_fstest shutdown auto log metadata
> >
> > -# Override the default cleanup function.
> > -_cleanup()
> > +_fsstress_cleanup()
> >  {
> > -       cd /
> > -       rm -f $tmp.*
> >         $KILLALL_PROG -9 $FSSTRESS_PROG > /dev/null 2>&1
> > -       _scratch_unmount > /dev/null 2>&1
> > +       wait
> 
> All right. I see that cleanup helpers are not consistent
> in calling wait after kill and in using SIGKILL.
> I guess we could go either way. So be it.

That's the whole point of these cleanups - they will be collapsed
down to a single helper and then we can apply the same behaviour
consistently across them all.

> > diff --git a/tests/xfs/310 b/tests/xfs/310
> > index 3214e04b..c635b39a 100755
> > --- a/tests/xfs/310
> > +++ b/tests/xfs/310
> > @@ -9,14 +9,12 @@
> >  . ./common/preamble
> >  _begin_fstest auto clone rmap
> >
> > -# Override the default cleanup function.
> > -_cleanup()
> > +_dmhuge_cleanup()
> >  {
> > -       cd /
> > -       umount $SCRATCH_MNT > /dev/null 2>&1
> >         _dmhugedisk_cleanup
> > -       rm -rf $tmp.*
> > +       _cleanup
> >  }
> > +_register_cleanup _dmhuge_cleanup
> 
> Maybe it's just me, but if the intention is to maybe make this
> function global someday then the difference with the name
> _dmhugedisk_cleanup() is confusing.

The names I used here are for classification, so I can grep over
the _register_cleanup line and easily determine all the tests use
loop devices, dmflakey, dmerror, etc and then consolidate them all
into a single cleanup function that works for them all. This is just
the first step in that process.

> I'd rather keep the local function name local_cleanup
> in unclear cases like this one.

And that defeats the entire purpose of giving them a descriptive
name at this point in time.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com