On Mon, Jun 20, 2022 at 2:17 PM Zorro Lang <zlang@xxxxxxxxxx> 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
>
> Hi Amir,
>
> I'm wondering why not let each freeze related test cases take care of "unfreeze"
> by itself? If a case freeze a filesystem which isn't on TEST_DEV or SCRATCH_DEV
> (e.g. a loop device base on a file in $TEST_DIR), or other situations, then the
> case still can call _require_freeze, but "unfreeze TEST_DIR or SCRATCH_MNT" can't
> help.
That's also an option, but:
1. It is less robust, leaving room for human mistakes. Why is that better?
2. Leftover frozen fs is quite harmful to subsequent test runs, so it
is important
to avoid it
3. Tests that freeze loop/dm (generic/459, xfs/428, [*]) can and should cleanup
themselves. I will add that too, but in any case...
4. unfreeze of tests and scratch fs is harmless even when it is not needed -
we may even want to do that at the start of tests run(?)
[*] I noticed that generic/085 which _require_freeze() does not even
use xfs_freeze (intentionally) - it uses dmsetup suspend/resume,
so I guess _require_freeze() should be removed from that test.
Anyway, if you and others insist against this auto-unfreeze approach,
I will post the patch to unfreeze fs in individual tests, but I won't be
happy about it.
Thanks,
Amir.
