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

[Nathan Huckleberry]

On Wed, Jul 27, 2022 at 8:48 AM Mike Snitzer <snitzer at redhat.com> wrote:
>
> On Wed, Jul 27 2022 at 11:25P -0400,
> Mikulas Patocka <mpatocka at 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.

Thanks,
Huck

> Thanks,
> Mike
>
>
>
> > On Tue, 26 Jul 2022, Mike Snitzer wrote:
> >
> > > From: Nathan Huckleberry <nhuck at google.com>
> > >
> > > Add an optional flag that ensures dm_bufio_client does not sleep
> > > (primary focus is to service dm_bufio_get without sleeping). This
> > > allows the dm-bufio cache to be queried from interrupt context.
> > >
> > > To ensure that dm-bufio does not sleep, dm-bufio must use a spinlock
> > > instead of a mutex. Additionally, to avoid deadlocks, special care
> > > must be taken so that dm-bufio does not sleep while holding the
> > > spinlock.
> > >
> > > But again: the scope of this no_sleep is initially confined to
> > > dm_bufio_get, so __alloc_buffer_wait_no_callback is _not_ changed to
> > > avoid sleeping because __bufio_new avoids allocation for NF_GET.
> > >
> > > Signed-off-by: Nathan Huckleberry <nhuck at google.com>
> > > Signed-off-by: Mike Snitzer <snitzer at kernel.org>
> > > ---
> > >  drivers/md/dm-bufio.c    | 22 +++++++++++++++++++---
> > >  include/linux/dm-bufio.h |  5 +++++
> > >  2 files changed, 24 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
> > > index ad5603eb12e3..486179d1831c 100644
> > > --- a/drivers/md/dm-bufio.c
> > > +++ b/drivers/md/dm-bufio.c
> > > @@ -81,6 +81,8 @@
> > >   */
> > >  struct dm_bufio_client {
> > >     struct mutex lock;
> > > +   spinlock_t spinlock;
> > > +   unsigned long spinlock_flags;
> > >
> > >     struct list_head lru[LIST_SIZE];
> > >     unsigned long n_buffers[LIST_SIZE];
> > > @@ -90,6 +92,7 @@ struct dm_bufio_client {
> > >     s8 sectors_per_block_bits;
> > >     void (*alloc_callback)(struct dm_buffer *);
> > >     void (*write_callback)(struct dm_buffer *);
> > > +   bool no_sleep;
> > >
> > >     struct kmem_cache *slab_buffer;
> > >     struct kmem_cache *slab_cache;
> > > @@ -167,17 +170,26 @@ struct dm_buffer {
> > >
> > >  static void dm_bufio_lock(struct dm_bufio_client *c)
> > >  {
> > > -   mutex_lock_nested(&c->lock, dm_bufio_in_request());
> > > +   if (c->no_sleep)
> > > +           spin_lock_irqsave_nested(&c->spinlock, c->spinlock_flags, dm_bufio_in_request());
> > > +   else
> > > +           mutex_lock_nested(&c->lock, dm_bufio_in_request());
> > >  }
> > >
> > >  static int dm_bufio_trylock(struct dm_bufio_client *c)
> > >  {
> > > -   return mutex_trylock(&c->lock);
> > > +   if (c->no_sleep)
> > > +           return spin_trylock_irqsave(&c->spinlock, c->spinlock_flags);
> > > +   else
> > > +           return mutex_trylock(&c->lock);
> > >  }
> > >
> > >  static void dm_bufio_unlock(struct dm_bufio_client *c)
> > >  {
> > > -   mutex_unlock(&c->lock);
> > > +   if (c->no_sleep)
> > > +           spin_unlock_irqrestore(&c->spinlock, c->spinlock_flags);
> > > +   else
> > > +           mutex_unlock(&c->lock);
> > >  }
> > >
> > >  /*----------------------------------------------------------------*/
> > > @@ -1748,12 +1760,16 @@ struct dm_bufio_client *dm_bufio_client_create(struct block_device *bdev, unsign
> > >     c->alloc_callback = alloc_callback;
> > >     c->write_callback = write_callback;
> > >
> > > +   if (flags & DM_BUFIO_CLIENT_NO_SLEEP)
> > > +           c->no_sleep = true;
> > > +
> > >     for (i = 0; i < LIST_SIZE; i++) {
> > >             INIT_LIST_HEAD(&c->lru[i]);
> > >             c->n_buffers[i] = 0;
> > >     }
> > >
> > >     mutex_init(&c->lock);
> > > +   spin_lock_init(&c->spinlock);
> > >     INIT_LIST_HEAD(&c->reserved_buffers);
> > >     c->need_reserved_buffers = reserved_buffers;
> > >
> > > diff --git a/include/linux/dm-bufio.h b/include/linux/dm-bufio.h
> > > index e21480715255..15d9e15ca830 100644
> > > --- a/include/linux/dm-bufio.h
> > > +++ b/include/linux/dm-bufio.h
> > > @@ -17,6 +17,11 @@
> > >  struct dm_bufio_client;
> > >  struct dm_buffer;
> > >
> > > +/*
> > > + * Flags for dm_bufio_client_create
> > > + */
> > > +#define DM_BUFIO_CLIENT_NO_SLEEP 0x1
> > > +
> > >  /*
> > >   * Create a buffered IO cache on a given device
> > >   */
> > > --
> > > 2.32.1 (Apple Git-133)
> > >
> > > --
> > > dm-devel mailing list
> > > dm-devel at redhat.com
> > > https://listman.redhat.com/mailman/listinfo/dm-devel
> > >
> > --
> > dm-devel mailing list
> > dm-devel at redhat.com
> > https://listman.redhat.com/mailman/listinfo/dm-devel
> >
>