| Message ID | 20220726160959.89247-3-snitzer@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | dm verity: optionally use tasklets | expand |
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. Mikulas On Tue, 26 Jul 2022, Mike Snitzer wrote: > From: Nathan Huckleberry <nhuck@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@google.com> > Signed-off-by: Mike Snitzer <snitzer@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@redhat.com > https://listman.redhat.com/mailman/listinfo/dm-devel > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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. Thanks, Mike > On Tue, 26 Jul 2022, Mike Snitzer wrote: > > > From: Nathan Huckleberry <nhuck@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@google.com> > > Signed-off-by: Mike Snitzer <snitzer@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@redhat.com > > https://listman.redhat.com/mailman/listinfo/dm-devel > > > -- > dm-devel mailing list > dm-devel@redhat.com > https://listman.redhat.com/mailman/listinfo/dm-devel > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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. Thanks, Huck > Thanks, > Mike > > > > > On Tue, 26 Jul 2022, Mike Snitzer wrote: > > > > > From: Nathan Huckleberry <nhuck@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@google.com> > > > Signed-off-by: Mike Snitzer <snitzer@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@redhat.com > > > https://listman.redhat.com/mailman/listinfo/dm-devel > > > > > -- > > dm-devel mailing list > > dm-devel@redhat.com > > https://listman.redhat.com/mailman/listinfo/dm-devel > > > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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
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 */