Re: [PATCH 2/4] fstests: make sure to unfreeze test and scratch mounts

From: Amir Goldstein <amir73il@gmail.com>
To: Zorro Lang <zlang@redhat.com>
Cc: "Darrick J . Wong" <darrick.wong@oracle.com>,
	Dave Chinner <david@fromorbit.com>,
	Brian Foster <bfoster@redhat.com>,
	fstests <fstests@vger.kernel.org>
Subject: Re: [PATCH 2/4] fstests: make sure to unfreeze test and scratch mounts
Date: Mon, 20 Jun 2022 18:08:47 +0300	[thread overview]
Message-ID: <CAOQ4uxj39eZ_jp1vYbmwHU7hbsdxjO_Q7MP5dpw8UAmvLsKYEA@mail.gmail.com> (raw)
In-Reply-To: <20220620142139.zukdj3nk4lntwtpx@zlang-mailbox>

On Mon, Jun 20, 2022 at 5:21 PM Zorro Lang <zlang@redhat.com> 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@redhat.com> 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@gmail.com>
> > > > ---
> > > >  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.