Re: [PATCH 3/4] xfs/{422,517}: add missing killall to _cleanup()

From: Amir Goldstein <amir73il@gmail.com>
To: Zorro Lang <zlang@redhat.com>
Cc: fstests <fstests@vger.kernel.org>
Subject: Re: [PATCH 3/4] xfs/{422,517}: add missing killall to _cleanup()
Date: Fri, 24 Jun 2022 09:41:34 +0300	[thread overview]
Message-ID: <CAOQ4uxitptR97n9fV5vPEd_HEVKABg-L=R-2kRNLoi8Ds7eK5A@mail.gmail.com> (raw)
In-Reply-To: <20220624063149.4ssefnfi5tboogp4@zlang-mailbox>

On Fri, Jun 24, 2022 at 9:31 AM Zorro Lang <zlang@redhat.com> wrote:
>
> On Fri, Jun 24, 2022 at 07:36:51AM +0300, Amir Goldstein wrote:
> > On Thu, Jun 23, 2022 at 9:04 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > >
> > > On Tue, Jun 21, 2022 at 07:02:02AM +0300, Amir Goldstein wrote:
> > > > On Sun, Jun 19, 2022 at 4:47 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > > >
> > > > > Those tests failed to cleanup background jobs after test
> > > > > is interrupted.
> > > > >
> > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > > ---
> > > > >  tests/xfs/422 | 8 ++++++++
> > > > >  tests/xfs/517 | 1 +
> > > > >  2 files changed, 9 insertions(+)
> > > > >
> > > > > diff --git a/tests/xfs/422 b/tests/xfs/422
> > > > > index a83a66df..8e9a3576 100755
> > > > > --- a/tests/xfs/422
> > > > > +++ b/tests/xfs/422
> > > > > @@ -13,6 +13,14 @@ _begin_fstest dangerous_scrub dangerous_online_repair freeze
> > > > >
> > > > >  _register_cleanup "_cleanup" BUS
> > > > >
> > > > > +# Override the default cleanup function.
> > > > > +_cleanup()
> > > > > +{
> > > > > +       $KILLALL_PROG -9 $XFS_IO_PROG $FSSTRESS_PROG > /dev/null 2>&1
> > > > > +       cd /
> > > > > +       rm -rf $tmp.*
> > > > > +}
> > > > > +
> > > > >  # Import common functions.
> > > > >  . ./common/filter
> > > > >  . ./common/fuzzy
> > > > > diff --git a/tests/xfs/517 b/tests/xfs/517
> > > > > index 961668e3..18404248 100755
> > > > > --- a/tests/xfs/517
> > > > > +++ b/tests/xfs/517
> > > > > @@ -14,6 +14,7 @@ _register_cleanup "_cleanup" BUS
> > > > >  # Override the default cleanup function.
> > > > >  _cleanup()
> > > > >  {
> > > > > +       $KILLALL_PROG -9 $XFS_IO_PROG $FSSTRESS_PROG > /dev/null 2>&1
> > > >
> > > > Besides the missing wait, this cleanup is still racy, because stress_loop
> > > > can spawn a new fstress process after killall /proc iteration.
> > > >
> > > > Worst, freeze_loop can freeze after killall /proc iteration.
> > > >
> > > > The correct solution (I think) is to record the pid of the XXX_loop
> > > > sub-shells and kill those, which should also solve the "Terminated"
> > > > false positive error.
> > > >
> > > > Will give that a shot.
> > >
> > > FWIW I already have patches reworking 422 and all the other
> > > scrub-related stress tests:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfstests-dev.git/log/?h=race-scrub-and-mount-state-changes
> > >
> >
> > That looks very nice.
> > The cleanup looks very similar to the one I posted for v2,
> > but instead of "killall xfs_io fsstress" you would be better off with
> > "kill  $__SCRUB_STRESS_FREEZE_PID"
> >
> > killing the child process and waiting for its parent bash
> > process has the side effect of the bash parent emitting
> > "Terminated" or "Killed" message with breaks golden output
> > if the inner loop did not already exit, which is the behavior
> > that got me started on doing this cleanup.
> >
> > > I swear I'll send all these some day, if I ever get caught up...
> > > Delegating LTS maintenance is a big help, but as it is I still can't
> > > stay abreast of all the mainline patchsets /and/ send my own stuff. :(
> > >
> >
> > I will respond to that in another email.
>
> Hi Amir,
>
> So ... will you singly resend a V3 patch to remove the xfs/422 changes in this patch?
>

I don't understand.

V2 patch should be reviewed

https://lore.kernel.org/fstests/20220621173729.2135249-1-amir73il@gmail.com/

The fact that Darrick has future work pending to cleanup xfs/422
should not stop a cleanup patch to be applied now.

Am I missing something?

Thanks,
Amir.