Re: [dm-devel] [PATCH v2 0/6] dm verity: optionally use tasklets

[Mike Snitzer <snitzer@redhat.com>]

On Tue, Aug 02 2022 at  9:39P -0400,
Nathan Huckleberry <nhuck@google.com> wrote:

> On Wed, Jul 27, 2022 at 1:23 AM Milan Broz <gmazyland@gmail.com> wrote:
> >
> > On 27/07/2022 01:04, Mike Snitzer wrote:
> > > On Tue, Jul 26 2022 at  5:44P -0400,
> > > Milan Broz <gmazyland@gmail.com> wrote:
> > >
> > >> On 26/07/2022 18:09, Mike Snitzer wrote:
> > >>> Hi,
> > >>>
> > >>> Please see this updated patchset that reflects what I've staged for
> > >>> the 5.20 merge window, see:
> > >>> https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/log/?h=dm-5.20
> > >>>
> > >>> My only outstanding question, from previous v1 patchset, is: should
> > >>> the verify_wq be created using WQ_HIGHPRI instead of WQ_CPU_INTENSIVE?
> > >>> (I doubt it has a significant impact on performance but if you have
> > >>> insight on why you made that change, and if it meaningful, I'd happily
> > >>> apply the change).
> > >>>
> > >>> I've tested using cryptsetup's testsuite (which has dm-verity tests)
> > >>> but I haven't tested the "try_verify_in_tasklet" feature.
> > >
> > > I wasn't lying above: I haven't tested the "try_verify_in_tasklet"
> > > feature.  I just figured I didn't break what Huck posted by cleaning
> > > it up while reviewing closely ;)
> >
> > :) What I am trying to avoid is that these patches reach Linux tree
> > until properly tested even in non-standard configurations (like FEC enabled).
> >
> > Currently we have not even enough HW for GitLab runners for upstream cryptsetup
> > CI testing and regression like these will cause *huge* pain for us later.
> >
> > >> I added new veritysetup option --use-tasklets for testing to a new branch
> > >> https://gitlab.com/cryptsetup/cryptsetup/-/commits/verify-tasklet
> > >>
> > >> I tried to run verity-compat-test (on that branch above), not used the flag yet,
> > >> just in one simple option flag test (see the branch).
> > >
> > > OK, thanks for doing this work, really appreciate it.  How is it I
> > > would initiate this test using your "verify-tasklet" branch?
> >
> > Basically, just checkout that branch, compile it
> > (autogen.sh, configure, make, make check-programs) and then run verity test
> > cd tests; ./verity-compat-test
> >
> > Even without adding the feature, FEC tests are skipped for some reason...
> > (Check for N/A that should not be there.)
> >
> > Then you can easily enable "--use-tasklets" for every open, I would just
> > comment this line:
> >
> > --- a/src/veritysetup.c
> > +++ b/src/veritysetup.c
> > @@ -184,7 +184,7 @@ static int _activate(const char *dm_device,
> >                  activate_flags |= CRYPT_ACTIVATE_IGNORE_ZERO_BLOCKS;
> >          if (ARG_SET(OPT_CHECK_AT_MOST_ONCE_ID))
> >                  activate_flags |= CRYPT_ACTIVATE_CHECK_AT_MOST_ONCE;
> > -       if (ARG_SET(OPT_USE_TASKLETS_ID))
> > +//     if (ARG_SET(OPT_USE_TASKLETS_ID))
> >                  activate_flags |= CRYPT_ACTIVATE_TASKLETS;
> >
> >
> > compile it, and run the verity-compat-test again.
> >
> 
> > For me, it stucks on the first in-kernel verify (non-FEC) line.
> > Some trace below...
> 
> I was able to fix this. There was a problem with falling back to a
> work-queue after FEC fails. This caused an infinite loop. I have
> dm-verity passing verity-compat-test with --use-tasklets; I'll send a
> v3 soon.
> 
> Thanks,
> Huck

Great news. If your fix is confined to a small incremental fix that
needs folding into patch 3 of the v2 patchset: please just send the
incremental patch.

Thanks,
Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel