On Mon, Jun 20, 2022 at 03:13:33PM +0300, Amir Goldstein wrote:
> 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 think Dave doesn't like to add common steps to thousands of cases, if without
a critical reason. So I hope to get more review points for this kind of changes.
>
> [*] 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.
Agree, I think dm suspend through different userspace and kernel interface with
fsfreeze, so that require doesn't make sense.
>
> 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.
>From the functional side, I think this change makes sense. But if think about
the performance, let's get more opinions at first. If there's not objection,
we can have it.
Thanks,
Zorro
>
> Thanks,
> Amir.
>
