On Mon, Jun 20, 2022 at 5:21 PM Zorro Lang <zlang@xxxxxxxxxx> wrote:
>
> 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.
>
Maybe the change was not clear.
Only the tests that declare _require_freeze() in the beginning of the test
(14 tests) are going to be affected by this change.
The rest of the tests have no impact at all.
Thanks,
Amir.
