Re: [dm-devel] [PATCH v2 2/6] dm bufio: Add DM_BUFIO_CLIENT_NO_SLEEP flag

From: Mike Snitzer <snitzer@redhat.com>
To: Nathan Huckleberry <nhuck@google.com>
Cc: Eric Biggers <ebiggers@kernel.org>,
	Mikulas Patocka <mpatocka@redhat.com>,
	dm-devel@redhat.com, Mike Snitzer <snitzer@kernel.org>,
	Sami Tolvanen <samitolvanen@google.com>
Subject: Re: [dm-devel] [PATCH v2 2/6] dm bufio: Add DM_BUFIO_CLIENT_NO_SLEEP flag
Date: Thu, 28 Jul 2022 18:37:09 -0400	[thread overview]
Message-ID: <YuMPlUzCvhEJIQXh@redhat.com> (raw)
In-Reply-To: <CAJkfWY7_W-o68=xvKga7=eTkDa4ygod2CV9MFdZbGw7xwE_U0w@mail.gmail.com>

On Wed, Jul 27 2022 at  3:53P -0400,
Nathan Huckleberry <nhuck@google.com> wrote:

> On Wed, Jul 27, 2022 at 8:48 AM Mike Snitzer <snitzer@redhat.com> wrote:
> >
> > On Wed, Jul 27 2022 at 11:25P -0400,
> > Mikulas Patocka <mpatocka@redhat.com> wrote:
> >
> > > Hi
> > >
> > > I'd like to ask - why not use dm_bufio_trylock from an interrupt context?
> > >
> > > I would just add a new function "dm_bufio_get_trylock" that is equivalent
> > > to "dm_bufio_get", except that it uses dm_bufio_trylock - and if it fails,
> > > fallback to process context.
> > >
> > > I think using dm_bufio_trylock would be less hacky than introducing a
> > > new dm_bufio flag that changes mutex to spinlock.
> >
> > OK, I can drop the bufio hacks (patches 1 and 2) and replace with a
> > dm_bufio_get_trylock and see if that resolves the cryptsetup testing
> > issues Milan is reporting.  But on the flip side I feel like trylock
> > is far more prone to fail for at least one of a series of cached
> > buffers needed (via _get). And so it'll punt to workqueue more often
> > and _not_ provide the desired performance improvement.  BUT.. we shall
> > see.
> >
> > All said, I'm now dropping this patchset from the upcoming 5.20 merge.
> > This all clearly needs more development time.
> >
> > Huck: I'll run with Mikulas's suggestion and try to get the cryptsetup
> > tests passing. But I'll leave performance testing to you.
> >
> 
> Sounds like a reasonable fix. I expect that dm_bufio_get_trylock with
> WQ_HIGHPRI will give similar performance gains.

Hi,

Just wanted to share where I am with this line of work:

1) I tried to use a semaphore instead of spinlock in bufio along with
adding a dm_bufio_get_nowait that used dm_bufio_trylock. Turns out
that wasn't valid because dm_bufio_release used down() and schedule,
which isn't valid in tasklet. Mikulas and I discussed this situation
and he proposed one way forward to still use semaphore but it'd
require new dm-verity code that prepared a cookie (struct) pointer and
issued a callback then drop the lock (so it'd avoid excessive
locking).  But I abandoned that due to the uncharted territory it was
bringing us down.

2) Using Milan's cryptsetup branch and test procedure documented here:
https://listman.redhat.com/archives/dm-devel/2022-July/051666.html
I got the first "./verity-compat-test" to work with this branch:
https://git.kernel.org/pub/scm/linux/kernel/git/snitzer/linux.git/log/?h=dm-5.21
But then when I took the next step to always --use-tasklets I got
hangs in the verity workqueue.

I'm not sure how you got your code working, I cannot see any material
difference between my dm-5.21 branch and your original patchset.

If you'd like to have a look at the dm-5.21 branch to see if you can
make it work that'd be appreciated.

But I cannot put more time to this verity tasklet effort any more this
week.

Thanks,
Mike

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