On Tue, Jun 21, 2022 at 1:06 AM Dave Chinner <david@xxxxxxxxxxxxx> wrote:
>
> On Sun, Jun 19, 2022 at 04:46:55PM +0300, Amir Goldstein wrote:
> > Almost all of the tests that _require_freeze() fail to unfreeze
> > scratch mount in case the test is interrupted while fs is frozen.
> >
> > Move the handling of unfreeze to generic check code.
> > For now, tests only freeze scratch fs, but to be more robust, unfreeze
> > both test and scratch fs following a call to _require_freeze().
> >
> > Tests could still hang if thier private _cleanup() routine tries
> > to modify the frozen fs or wait for a blocked process. Fix the
> > _cleanup() routine of xfs/011 to avoid that.
> >
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > ---
> > check | 14 ++++++++------
> > common/rc | 5 +++--
> > tests/generic/390 | 2 --
> > tests/xfs/011 | 2 --
> > tests/xfs/517 | 1 -
> > 5 files changed, 11 insertions(+), 13 deletions(-)
> >
> > diff --git a/check b/check
> > index de11b37e..d6ee71aa 100755
> > --- a/check
> > +++ b/check
> > @@ -527,17 +527,21 @@ _check_filesystems()
> > {
> > local ret=0
> >
> > + # Make sure both test and scratch are unfrozen post _require_freeze()
> > + if [ -f ${RESULT_DIR}/require_freeze ]; then
> > + xfs_freeze -u "$TEST_DIR" >/dev/null 2>&1
> > + xfs_freeze -u "$SCRATCH_MNT" >/dev/null 2>&1
> > + fi
>
> A test leaving a filesystem frozen on exit is a test bug. There can
> still be background test processes sitting blocked on a frozen
> filesystem when the test exits with a frozen filesystem, and that
> has the potential to cause problems in the next few operations
> because of "busy filesystem" errors trying to unmount the fs...
>
> IOWs, think this is the wrong way to address this problem. tests
> that freeze filesystems need to ensure that everything is cleaned up
> properly in the test _cleanup() function where the right thing can
> be done and blocked processes can be waited on once the fs has been
> thawed.
>
ok.
> > diff --git a/tests/generic/390 b/tests/generic/390
> > index 20c66e22..0f2b86fa 100755
> > --- a/tests/generic/390
> > +++ b/tests/generic/390
> > @@ -14,8 +14,6 @@ _begin_fstest auto freeze stress
> > _cleanup()
> > {
> > cd /
> > - # Make sure $SCRATCH_MNT is unfreezed
> > - xfs_freeze -u $SCRATCH_MNT 2>/dev/null
> > rm -f $tmp.*
> > }
>
> This test is already doing the right thing.
>
> > diff --git a/tests/xfs/011 b/tests/xfs/011
> > index d6e9099e..351a574e 100755
> > --- a/tests/xfs/011
> > +++ b/tests/xfs/011
> > @@ -17,9 +17,7 @@ _begin_fstest auto freeze log metadata quick
> > _cleanup()
> > {
> > $KILLALL_PROG -9 fsstress 2>/dev/null
> > - wait
> > cd /
> > - _scratch_unmount 2>/dev/null
> > rm -f $tmp.*
> > }
>
> This is wrong. We have to wait for background fsstress processes to
> exit, otherwise unmount can fail randomly. What it is missing is the
> thaw before killing the fsstress processes and waiting for them to
> complete.
>
I didn't remember if your position was that _cleanup()
should wait or not . I guess from your answer that you think
that it should and it makes sense to me, so I will also fix xfs/517
to wait.
Thanks,
Amir.
