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

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

[Mike Snitzer]

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'd welcome review and focused "try_verify_in_tasklet" testing.

Thanks,
Mike

Mike Snitzer (3):
  dm verity: allow optional args to alter primary args handling
  dm bufio: conditionally enable branching for DM_BUFIO_CLIENT_NO_SLEEP
  dm verity: conditionally enable branching for "try_verify_in_tasklet"

Nathan Huckleberry (3):
  dm bufio: Add flags argument to dm_bufio_client_create
  dm bufio: Add DM_BUFIO_CLIENT_NO_SLEEP flag
  dm verity: Add optional "try_verify_in_tasklet" feature

 drivers/md/dm-bufio.c                         |  32 ++++-
 drivers/md/dm-ebs-target.c                    |   3 +-
 drivers/md/dm-integrity.c                     |   2 +-
 drivers/md/dm-snap-persistent.c               |   2 +-
 drivers/md/dm-verity-fec.c                    |   4 +-
 drivers/md/dm-verity-target.c                 | 121 +++++++++++++++---
 drivers/md/dm-verity.h                        |   7 +-
 drivers/md/persistent-data/dm-block-manager.c |   3 +-
 include/linux/dm-bufio.h                      |   8 +-
 9 files changed, 154 insertions(+), 28 deletions(-)

-- 
2.32.1 (Apple Git-133)

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

[dm-devel] [PATCH v2 1/6] dm bufio: Add flags argument to dm_bufio_client_create

[Mike Snitzer]

From: Nathan Huckleberry <nhuck@google.com>

Add a flags argument to dm_bufio_client_create and update all the
callers. This is in preparation to add the DM_BUFIO_NO_SLEEP flag.

Signed-off-by: Nathan Huckleberry <nhuck@google.com>
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 drivers/md/dm-bufio.c                         | 3 ++-
 drivers/md/dm-ebs-target.c                    | 3 ++-
 drivers/md/dm-integrity.c                     | 2 +-
 drivers/md/dm-snap-persistent.c               | 2 +-
 drivers/md/dm-verity-fec.c                    | 4 ++--
 drivers/md/dm-verity-target.c                 | 2 +-
 drivers/md/persistent-data/dm-block-manager.c | 3 ++-
 include/linux/dm-bufio.h                      | 3 ++-
 8 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
index 5ffa1dcf84cf..ad5603eb12e3 100644
--- a/drivers/md/dm-bufio.c
+++ b/drivers/md/dm-bufio.c
@@ -1717,7 +1717,8 @@ static unsigned long dm_bufio_shrink_count(struct shrinker *shrink, struct shrin
 struct dm_bufio_client *dm_bufio_client_create(struct block_device *bdev, unsigned block_size,
 					       unsigned reserved_buffers, unsigned aux_size,
 					       void (*alloc_callback)(struct dm_buffer *),
-					       void (*write_callback)(struct dm_buffer *))
+					       void (*write_callback)(struct dm_buffer *),
+					       unsigned int flags)
 {
 	int r;
 	struct dm_bufio_client *c;
diff --git a/drivers/md/dm-ebs-target.c b/drivers/md/dm-ebs-target.c
index 0221fa63f888..04a1f56d6588 100644
--- a/drivers/md/dm-ebs-target.c
+++ b/drivers/md/dm-ebs-target.c
@@ -312,7 +312,8 @@ static int ebs_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 		goto bad;
 	}
 
-	ec->bufio = dm_bufio_client_create(ec->dev->bdev, to_bytes(ec->u_bs), 1, 0, NULL, NULL);
+	ec->bufio = dm_bufio_client_create(ec->dev->bdev, to_bytes(ec->u_bs), 1,
+					   0, NULL, NULL, 0);
 	if (IS_ERR(ec->bufio)) {
 		ti->error = "Cannot create dm bufio client";
 		r = PTR_ERR(ec->bufio);
diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index 67dc35b32264..a537fb8e6459 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -4434,7 +4434,7 @@ static int dm_integrity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	}
 
 	ic->bufio = dm_bufio_client_create(ic->meta_dev ? ic->meta_dev->bdev : ic->dev->bdev,
-			1U << (SECTOR_SHIFT + ic->log2_buffer_sectors), 1, 0, NULL, NULL);
+			1U << (SECTOR_SHIFT + ic->log2_buffer_sectors), 1, 0, NULL, NULL, 0);
 	if (IS_ERR(ic->bufio)) {
 		r = PTR_ERR(ic->bufio);
 		ti->error = "Cannot initialize dm-bufio";
diff --git a/drivers/md/dm-snap-persistent.c b/drivers/md/dm-snap-persistent.c
index 3bb5cff5d6fc..aaa699749c3b 100644
--- a/drivers/md/dm-snap-persistent.c
+++ b/drivers/md/dm-snap-persistent.c
@@ -494,7 +494,7 @@ static int read_exceptions(struct pstore *ps,
 
 	client = dm_bufio_client_create(dm_snap_cow(ps->store->snap)->bdev,
 					ps->store->chunk_size << SECTOR_SHIFT,
-					1, 0, NULL, NULL);
+					1, 0, NULL, NULL, 0);
 
 	if (IS_ERR(client))
 		return PTR_ERR(client);
diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index cea2b3789736..23cffce56403 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -749,7 +749,7 @@ int verity_fec_ctr(struct dm_verity *v)
 
 	f->bufio = dm_bufio_client_create(f->dev->bdev,
 					  f->io_size,
-					  1, 0, NULL, NULL);
+					  1, 0, NULL, NULL, 0);
 	if (IS_ERR(f->bufio)) {
 		ti->error = "Cannot initialize FEC bufio client";
 		return PTR_ERR(f->bufio);
@@ -765,7 +765,7 @@ int verity_fec_ctr(struct dm_verity *v)
 
 	f->data_bufio = dm_bufio_client_create(v->data_dev->bdev,
 					       1 << v->data_dev_block_bits,
-					       1, 0, NULL, NULL);
+					       1, 0, NULL, NULL, 0);
 	if (IS_ERR(f->data_bufio)) {
 		ti->error = "Cannot initialize FEC data bufio client";
 		return PTR_ERR(f->data_bufio);
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index 75b66dd67633..95db3a7ee3c7 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -1265,7 +1265,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 
 	v->bufio = dm_bufio_client_create(v->hash_dev->bdev,
 		1 << v->hash_dev_block_bits, 1, sizeof(struct buffer_aux),
-		dm_bufio_alloc_callback, NULL);
+		dm_bufio_alloc_callback, NULL, 0);
 	if (IS_ERR(v->bufio)) {
 		ti->error = "Cannot initialize dm-bufio";
 		r = PTR_ERR(v->bufio);
diff --git a/drivers/md/persistent-data/dm-block-manager.c b/drivers/md/persistent-data/dm-block-manager.c
index 54c089a50b15..11935864f50f 100644
--- a/drivers/md/persistent-data/dm-block-manager.c
+++ b/drivers/md/persistent-data/dm-block-manager.c
@@ -391,7 +391,8 @@ struct dm_block_manager *dm_block_manager_create(struct block_device *bdev,
 	bm->bufio = dm_bufio_client_create(bdev, block_size, max_held_per_thread,
 					   sizeof(struct buffer_aux),
 					   dm_block_manager_alloc_callback,
-					   dm_block_manager_write_callback);
+					   dm_block_manager_write_callback,
+					   0);
 	if (IS_ERR(bm->bufio)) {
 		r = PTR_ERR(bm->bufio);
 		kfree(bm);
diff --git a/include/linux/dm-bufio.h b/include/linux/dm-bufio.h
index 90bd558a17f5..e21480715255 100644
--- a/include/linux/dm-bufio.h
+++ b/include/linux/dm-bufio.h
@@ -24,7 +24,8 @@ struct dm_bufio_client *
 dm_bufio_client_create(struct block_device *bdev, unsigned block_size,
 		       unsigned reserved_buffers, unsigned aux_size,
 		       void (*alloc_callback)(struct dm_buffer *),
-		       void (*write_callback)(struct dm_buffer *));
+		       void (*write_callback)(struct dm_buffer *),
+		       unsigned int flags);
 
 /*
  * Release a buffered IO cache.
-- 
2.32.1 (Apple Git-133)

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

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

[Mike Snitzer]

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

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

[Mikulas Patocka]

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

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

[Mike Snitzer]

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

Re: [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@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

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

[Mike Snitzer]

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

[dm-devel] [PATCH v2 3/6] dm verity: Add optional "try_verify_in_tasklet" feature

[Mike Snitzer]

From: Nathan Huckleberry <nhuck@google.com>

Using tasklets for disk verification can reduce IO latency. When there
are accelerated hash instructions it is often better to compute the
hash immediately using a tasklet rather than deferring verification to
a work-queue. This reduces time spent waiting to schedule work-queue
jobs, but requires spending slightly more time in interrupt context.

If the dm-bufio cache does not have the required hashes we fallback to
the work-queue implementation. FEC is only possible using work-queue
because code to support the FEC feature may sleep.

The following shows a speed comparison of random reads on a dm-verity
device. The dm-verity device uses a 1G ramdisk for data and a 1G
ramdisk for hashes. One test was run using tasklets and one test was
run using the existing work-queue solution. Both tests were run when
the dm-bufio cache was hot. The tasklet implementation performs
significantly better since there is no time spent waiting for
work-queue jobs to be scheduled.

   READ: bw=181MiB/s (190MB/s), 181MiB/s-181MiB/s (190MB/s-190MB/s), io=512MiB
   (537MB), run=2827-2827msec
   READ: bw=23.6MiB/s (24.8MB/s), 23.6MiB/s-23.6MiB/s (24.8MB/s-24.8MB/s),
   io=512MiB (537MB), run=21688-21688msec

Signed-off-by: Nathan Huckleberry <nhuck@google.com>
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 drivers/md/dm-verity-target.c | 90 ++++++++++++++++++++++++++++++-----
 drivers/md/dm-verity.h        |  7 ++-
 2 files changed, 83 insertions(+), 14 deletions(-)

diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index 95db3a7ee3c7..3b566077a74e 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -34,6 +34,7 @@
 #define DM_VERITY_OPT_PANIC		"panic_on_corruption"
 #define DM_VERITY_OPT_IGN_ZEROES	"ignore_zero_blocks"
 #define DM_VERITY_OPT_AT_MOST_ONCE	"check_at_most_once"
+#define DM_VERITY_OPT_TASKLET_VERIFY	"try_verify_in_tasklet"
 
 #define DM_VERITY_OPTS_MAX		(3 + DM_VERITY_OPTS_FEC + \
 					 DM_VERITY_ROOT_HASH_VERIFICATION_OPTS)
@@ -220,7 +221,7 @@ static int verity_handle_err(struct dm_verity *v, enum verity_block_type type,
 	struct mapped_device *md = dm_table_get_md(v->ti->table);
 
 	/* Corruption should be visible in device status in all modes */
-	v->hash_failed = 1;
+	v->hash_failed = true;
 
 	if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS)
 		goto out;
@@ -286,7 +287,19 @@ static int verity_verify_level(struct dm_verity *v, struct dm_verity_io *io,
 
 	verity_hash_at_level(v, block, level, &hash_block, &offset);
 
-	data = dm_bufio_read(v->bufio, hash_block, &buf);
+	if (io->in_tasklet) {
+		data = dm_bufio_get(v->bufio, hash_block, &buf);
+		if (data == NULL) {
+			/*
+			 * In tasklet and the hash was not in the bufio cache.
+			 * Return early and resume execution from a work-queue
+			 * to read the hash from disk.
+			 */
+			return -EAGAIN;
+		}
+	} else
+		data = dm_bufio_read(v->bufio, hash_block, &buf);
+
 	if (IS_ERR(data))
 		return PTR_ERR(data);
 
@@ -307,6 +320,14 @@ static int verity_verify_level(struct dm_verity *v, struct dm_verity_io *io,
 		if (likely(memcmp(verity_io_real_digest(v, io), want_digest,
 				  v->digest_size) == 0))
 			aux->hash_verified = 1;
+		else if (io->in_tasklet) {
+			/*
+			 * FEC code cannot be run in a tasklet since it may
+			 * sleep, so fallback to using a work-queue.
+			 */
+			r = -EAGAIN;
+			goto release_ret_r;
+		}
 		else if (verity_fec_decode(v, io,
 					   DM_VERITY_BLOCK_TYPE_METADATA,
 					   hash_block, data, NULL) == 0)
@@ -474,13 +495,12 @@ static int verity_verify_io(struct dm_verity_io *io)
 	bool is_zero;
 	struct dm_verity *v = io->v;
 	struct bvec_iter start;
-	unsigned b;
 	struct crypto_wait wait;
 	struct bio *bio = dm_bio_from_per_bio_data(io, v->ti->per_io_data_size);
 
-	for (b = 0; b < io->n_blocks; b++) {
+	for (; io->block_idx < io->n_blocks; io->block_idx++) {
 		int r;
-		sector_t cur_block = io->block + b;
+		sector_t cur_block = io->block + io->block_idx;
 		struct ahash_request *req = verity_io_hash_req(v, io);
 
 		if (v->validated_blocks &&
@@ -527,8 +547,14 @@ static int verity_verify_io(struct dm_verity_io *io)
 			if (v->validated_blocks)
 				set_bit(cur_block, v->validated_blocks);
 			continue;
+		} else if (io->in_tasklet) {
+			/*
+			 * FEC code cannot be run in a tasklet since it may
+			 * sleep, so fallback to using a work-queue.
+			 */
+			return -EAGAIN;
 		} else if (verity_fec_decode(v, io, DM_VERITY_BLOCK_TYPE_DATA,
-					   cur_block, NULL, &start) == 0) {
+					     cur_block, NULL, &start) == 0) {
 			continue;
 		} else {
 			if (bio->bi_status) {
@@ -566,7 +592,8 @@ static void verity_finish_io(struct dm_verity_io *io, blk_status_t status)
 	bio->bi_end_io = io->orig_bi_end_io;
 	bio->bi_status = status;
 
-	verity_fec_finish_io(io);
+	if (!io->in_tasklet)
+		verity_fec_finish_io(io);
 
 	bio_endio(bio);
 }
@@ -578,6 +605,24 @@ static void verity_work(struct work_struct *w)
 	verity_finish_io(io, errno_to_blk_status(verity_verify_io(io)));
 }
 
+static void verity_tasklet(unsigned long data)
+{
+	struct dm_verity_io *io = (struct dm_verity_io *)data;
+	int err;
+
+	io->in_tasklet = true;
+	err = verity_verify_io(io);
+	if (err == -EAGAIN) {
+		/* fallback to retrying with work-queue */
+		io->in_tasklet = false;
+		INIT_WORK(&io->work, verity_work);
+		queue_work(io->v->verify_wq, &io->work);
+		return;
+	}
+
+	verity_finish_io(io, errno_to_blk_status(err));
+}
+
 static void verity_end_io(struct bio *bio)
 {
 	struct dm_verity_io *io = bio->bi_private;
@@ -588,8 +633,14 @@ static void verity_end_io(struct bio *bio)
 		return;
 	}
 
-	INIT_WORK(&io->work, verity_work);
-	queue_work(io->v->verify_wq, &io->work);
+	io->block_idx = 0;
+	if (io->v->use_tasklet) {
+		tasklet_init(&io->tasklet, verity_tasklet, (unsigned long)io);
+		tasklet_schedule(&io->tasklet);
+	} else {
+		INIT_WORK(&io->work, verity_work);
+		queue_work(io->v->verify_wq, &io->work);
+	}
 }
 
 /*
@@ -751,6 +802,8 @@ static void verity_status(struct dm_target *ti, status_type_t type,
 			args++;
 		if (v->validated_blocks)
 			args++;
+		if (v->use_tasklet)
+			args++;
 		if (v->signature_key_desc)
 			args += DM_VERITY_ROOT_HASH_VERIFICATION_OPTS;
 		if (!args)
@@ -776,6 +829,8 @@ static void verity_status(struct dm_target *ti, status_type_t type,
 			DMEMIT(" " DM_VERITY_OPT_IGN_ZEROES);
 		if (v->validated_blocks)
 			DMEMIT(" " DM_VERITY_OPT_AT_MOST_ONCE);
+		if (v->use_tasklet)
+			DMEMIT(" " DM_VERITY_OPT_TASKLET_VERIFY);
 		sz = verity_fec_status_table(v, sz, result, maxlen);
 		if (v->signature_key_desc)
 			DMEMIT(" " DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY
@@ -1011,11 +1066,16 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
 				return r;
 			continue;
 
+		} else if (!strcasecmp(arg_name, DM_VERITY_OPT_TASKLET_VERIFY)) {
+			v->use_tasklet = true;
+			continue;
+
 		} else if (verity_is_fec_opt_arg(arg_name)) {
 			r = verity_fec_parse_opt_args(as, v, &argc, arg_name);
 			if (r)
 				return r;
 			continue;
+
 		} else if (verity_verify_is_sig_opt_arg(arg_name)) {
 			r = verity_verify_sig_parse_opt_args(as, v,
 							     verify_args,
@@ -1023,7 +1083,6 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
 			if (r)
 				return r;
 			continue;
-
 		}
 
 		ti->error = "Unrecognized verity feature request";
@@ -1155,7 +1214,11 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		goto bad;
 	}
 
-	v->tfm = crypto_alloc_ahash(v->alg_name, 0, 0);
+	/*
+	 * FIXME: CRYPTO_ALG_ASYNC should be conditional on v->use_tasklet
+	 * but verity_parse_opt_args() happens below and has data dep on tfm.
+	 */
+	v->tfm = crypto_alloc_ahash(v->alg_name, 0, CRYPTO_ALG_ASYNC);
 	if (IS_ERR(v->tfm)) {
 		ti->error = "Cannot initialize hash function";
 		r = PTR_ERR(v->tfm);
@@ -1265,7 +1328,8 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 
 	v->bufio = dm_bufio_client_create(v->hash_dev->bdev,
 		1 << v->hash_dev_block_bits, 1, sizeof(struct buffer_aux),
-		dm_bufio_alloc_callback, NULL, 0);
+		dm_bufio_alloc_callback, NULL,
+		v->use_tasklet ? DM_BUFIO_CLIENT_NO_SLEEP : 0);
 	if (IS_ERR(v->bufio)) {
 		ti->error = "Cannot initialize dm-bufio";
 		r = PTR_ERR(v->bufio);
@@ -1312,7 +1376,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 static struct target_type verity_target = {
 	.name		= "verity",
 	.features	= DM_TARGET_IMMUTABLE,
-	.version	= {1, 8, 0},
+	.version	= {1, 9, 0},
 	.module		= THIS_MODULE,
 	.ctr		= verity_ctr,
 	.dtr		= verity_dtr,
diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
index 4e769d13473a..e73a7972c3e9 100644
--- a/drivers/md/dm-verity.h
+++ b/drivers/md/dm-verity.h
@@ -13,6 +13,7 @@
 
 #include <linux/dm-bufio.h>
 #include <linux/device-mapper.h>
+#include <linux/interrupt.h>
 #include <crypto/hash.h>
 
 #define DM_VERITY_MAX_LEVELS		63
@@ -51,9 +52,10 @@ struct dm_verity {
 	unsigned char hash_per_block_bits;	/* log2(hashes in hash block) */
 	unsigned char levels;	/* the number of tree levels */
 	unsigned char version;
+	bool hash_failed:1;	/* set if hash of any block failed */
+	bool use_tasklet:1;	/* try to verify in tasklet before work-queue */
 	unsigned digest_size;	/* digest size for the current hash algorithm */
 	unsigned int ahash_reqsize;/* the size of temporary space for crypto */
-	int hash_failed;	/* set to 1 if hash of any block failed */
 	enum verity_mode mode;	/* mode for handling verification errors */
 	unsigned corrupted_errs;/* Number of errors for corrupted blocks */
 
@@ -76,10 +78,13 @@ struct dm_verity_io {
 
 	sector_t block;
 	unsigned n_blocks;
+	unsigned int block_idx;
+	bool in_tasklet;
 
 	struct bvec_iter iter;
 
 	struct work_struct work;
+	struct tasklet_struct tasklet;
 
 	/*
 	 * Three variably-size fields follow this struct:
-- 
2.32.1 (Apple Git-133)

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

[dm-devel] [PATCH v2 4/6] dm verity: allow optional args to alter primary args handling

[Mike Snitzer]

Commit 6d891d0978a2 ("dm verity: Add optional "try_verify_in_tasklet"
feature") imposed that CRYPTO_ALG_ASYNC mask be used even if the
optional "try_verify_in_tasklet" feature was not specified. This was
because verity_parse_opt_args() was called after handling the primary
args (due to it having data dependencies on having first parsed all
primary args).

Enhance verity_ctr() so that simple optional args, that don't have a
data dependency on primary args parsing, can alter how the primary
args are handled. In practice this means verity_parse_opt_args() gets
called twice. First with the new 'only_modifier_opts' arg set to true,
then again with it set to false _after_ parsing all primary args.

This allows the v->use_tasklet flag to be properly set and then used
when verity_ctr() parses the primary args and then calls
crypto_alloc_ahash().

Fixes: 6d891d0978a2 ("dm verity: Add optional "try_verify_in_tasklet" feature")
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 drivers/md/dm-verity-target.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index 3b566077a74e..054095c2052b 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -1022,7 +1022,8 @@ static int verity_parse_verity_mode(struct dm_verity *v, const char *arg_name)
 }
 
 static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
-				 struct dm_verity_sig_opts *verify_args)
+				 struct dm_verity_sig_opts *verify_args,
+				 bool only_modifier_opts)
 {
 	int r;
 	unsigned argc;
@@ -1045,6 +1046,8 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
 		argc--;
 
 		if (verity_is_verity_mode(arg_name)) {
+			if (only_modifier_opts)
+				continue;
 			r = verity_parse_verity_mode(v, arg_name);
 			if (r) {
 				ti->error = "Conflicting error handling parameters";
@@ -1053,6 +1056,8 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
 			continue;
 
 		} else if (!strcasecmp(arg_name, DM_VERITY_OPT_IGN_ZEROES)) {
+			if (only_modifier_opts)
+				continue;
 			r = verity_alloc_zero_digest(v);
 			if (r) {
 				ti->error = "Cannot allocate zero digest";
@@ -1061,6 +1066,8 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
 			continue;
 
 		} else if (!strcasecmp(arg_name, DM_VERITY_OPT_AT_MOST_ONCE)) {
+			if (only_modifier_opts)
+				continue;
 			r = verity_alloc_most_once(v);
 			if (r)
 				return r;
@@ -1071,12 +1078,16 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
 			continue;
 
 		} else if (verity_is_fec_opt_arg(arg_name)) {
+			if (only_modifier_opts)
+				continue;
 			r = verity_fec_parse_opt_args(as, v, &argc, arg_name);
 			if (r)
 				return r;
 			continue;
 
 		} else if (verity_verify_is_sig_opt_arg(arg_name)) {
+			if (only_modifier_opts)
+				continue;
 			r = verity_verify_sig_parse_opt_args(as, v,
 							     verify_args,
 							     &argc, arg_name);
@@ -1143,6 +1154,15 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		goto bad;
 	}
 
+	/* Parse optional parameters that modify primary args */
+	if (argc > 10) {
+		as.argc = argc - 10;
+		as.argv = argv + 10;
+		r = verity_parse_opt_args(&as, v, &verify_args, true);
+		if (r < 0)
+			goto bad;
+	}
+
 	if (sscanf(argv[0], "%u%c", &num, &dummy) != 1 ||
 	    num > 1) {
 		ti->error = "Invalid version";
@@ -1214,11 +1234,8 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		goto bad;
 	}
 
-	/*
-	 * FIXME: CRYPTO_ALG_ASYNC should be conditional on v->use_tasklet
-	 * but verity_parse_opt_args() happens below and has data dep on tfm.
-	 */
-	v->tfm = crypto_alloc_ahash(v->alg_name, 0, CRYPTO_ALG_ASYNC);
+	v->tfm = crypto_alloc_ahash(v->alg_name, 0,
+				    v->use_tasklet ? CRYPTO_ALG_ASYNC : 0);
 	if (IS_ERR(v->tfm)) {
 		ti->error = "Cannot initialize hash function";
 		r = PTR_ERR(v->tfm);
@@ -1280,8 +1297,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	if (argc) {
 		as.argc = argc;
 		as.argv = argv;
-
-		r = verity_parse_opt_args(&as, v, &verify_args);
+		r = verity_parse_opt_args(&as, v, &verify_args, false);
 		if (r < 0)
 			goto bad;
 	}
-- 
2.32.1 (Apple Git-133)

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

[dm-devel] [PATCH v2 5/6] dm bufio: conditionally enable branching for DM_BUFIO_CLIENT_NO_SLEEP

[Mike Snitzer]

Use jump_label to limit the need for branching unless the optional
DM_BUFIO_CLIENT_NO_SLEEP is used.

Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 drivers/md/dm-bufio.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
index 486179d1831c..0a38a7aef49c 100644
--- a/drivers/md/dm-bufio.c
+++ b/drivers/md/dm-bufio.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/rbtree.h>
 #include <linux/stacktrace.h>
+#include <linux/jump_label.h>
 
 #define DM_MSG_PREFIX "bufio"
 
@@ -164,13 +165,15 @@ struct dm_buffer {
 #endif
 };
 
+static DEFINE_STATIC_KEY_FALSE(no_sleep_enabled);
+
 /*----------------------------------------------------------------*/
 
 #define dm_bufio_in_request()	(!!current->bio_list)
 
 static void dm_bufio_lock(struct dm_bufio_client *c)
 {
-	if (c->no_sleep)
+	if (static_branch_unlikely(&no_sleep_enabled) && 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());
@@ -178,7 +181,7 @@ static void dm_bufio_lock(struct dm_bufio_client *c)
 
 static int dm_bufio_trylock(struct dm_bufio_client *c)
 {
-	if (c->no_sleep)
+	if (static_branch_unlikely(&no_sleep_enabled) && c->no_sleep)
 		return spin_trylock_irqsave(&c->spinlock, c->spinlock_flags);
 	else
 		return mutex_trylock(&c->lock);
@@ -186,7 +189,7 @@ static int dm_bufio_trylock(struct dm_bufio_client *c)
 
 static void dm_bufio_unlock(struct dm_bufio_client *c)
 {
-	if (c->no_sleep)
+	if (static_branch_unlikely(&no_sleep_enabled) && c->no_sleep)
 		spin_unlock_irqrestore(&c->spinlock, c->spinlock_flags);
 	else
 		mutex_unlock(&c->lock);
@@ -1760,8 +1763,10 @@ 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)
+	if (flags & DM_BUFIO_CLIENT_NO_SLEEP) {
 		c->no_sleep = true;
+		static_branch_inc(&no_sleep_enabled);
+	}
 
 	for (i = 0; i < LIST_SIZE; i++) {
 		INIT_LIST_HEAD(&c->lru[i]);
@@ -1895,6 +1900,8 @@ void dm_bufio_client_destroy(struct dm_bufio_client *c)
 	kmem_cache_destroy(c->slab_buffer);
 	dm_io_client_destroy(c->dm_io);
 	mutex_destroy(&c->lock);
+	if (c->no_sleep)
+		static_branch_dec(&no_sleep_enabled);
 	kfree(c);
 }
 EXPORT_SYMBOL_GPL(dm_bufio_client_destroy);
-- 
2.32.1 (Apple Git-133)

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

[dm-devel] [PATCH v2 6/6] dm verity: conditionally enable branching for "try_verify_in_tasklet"

[Mike Snitzer]

Use jump_label to limit the need for branching unless the optional
"try_verify_in_tasklet" feature is used.

Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 drivers/md/dm-verity-target.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index 054095c2052b..f747eb874cfb 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/reboot.h>
 #include <linux/scatterlist.h>
+#include <linux/jump_label.h>
 
 #define DM_MSG_PREFIX			"verity"
 
@@ -43,6 +44,8 @@ static unsigned dm_verity_prefetch_cluster = DM_VERITY_DEFAULT_PREFETCH_SIZE;
 
 module_param_named(prefetch_cluster, dm_verity_prefetch_cluster, uint, S_IRUGO | S_IWUSR);
 
+static DEFINE_STATIC_KEY_FALSE(use_tasklet_enabled);
+
 struct dm_verity_prefetch_work {
 	struct work_struct work;
 	struct dm_verity *v;
@@ -287,7 +290,7 @@ static int verity_verify_level(struct dm_verity *v, struct dm_verity_io *io,
 
 	verity_hash_at_level(v, block, level, &hash_block, &offset);
 
-	if (io->in_tasklet) {
+	if (static_branch_unlikely(&use_tasklet_enabled) && io->in_tasklet) {
 		data = dm_bufio_get(v->bufio, hash_block, &buf);
 		if (data == NULL) {
 			/*
@@ -320,7 +323,8 @@ static int verity_verify_level(struct dm_verity *v, struct dm_verity_io *io,
 		if (likely(memcmp(verity_io_real_digest(v, io), want_digest,
 				  v->digest_size) == 0))
 			aux->hash_verified = 1;
-		else if (io->in_tasklet) {
+		else if (static_branch_unlikely(&use_tasklet_enabled) &&
+			 io->in_tasklet) {
 			/*
 			 * FEC code cannot be run in a tasklet since it may
 			 * sleep, so fallback to using a work-queue.
@@ -547,7 +551,8 @@ static int verity_verify_io(struct dm_verity_io *io)
 			if (v->validated_blocks)
 				set_bit(cur_block, v->validated_blocks);
 			continue;
-		} else if (io->in_tasklet) {
+		} else if (static_branch_unlikely(&use_tasklet_enabled) &&
+			   io->in_tasklet) {
 			/*
 			 * FEC code cannot be run in a tasklet since it may
 			 * sleep, so fallback to using a work-queue.
@@ -592,7 +597,7 @@ static void verity_finish_io(struct dm_verity_io *io, blk_status_t status)
 	bio->bi_end_io = io->orig_bi_end_io;
 	bio->bi_status = status;
 
-	if (!io->in_tasklet)
+	if (!static_branch_unlikely(&use_tasklet_enabled) || !io->in_tasklet)
 		verity_fec_finish_io(io);
 
 	bio_endio(bio);
@@ -634,7 +639,7 @@ static void verity_end_io(struct bio *bio)
 	}
 
 	io->block_idx = 0;
-	if (io->v->use_tasklet) {
+	if (static_branch_unlikely(&use_tasklet_enabled) && io->v->use_tasklet) {
 		tasklet_init(&io->tasklet, verity_tasklet, (unsigned long)io);
 		tasklet_schedule(&io->tasklet);
 	} else {
@@ -944,6 +949,9 @@ static void verity_dtr(struct dm_target *ti)
 
 	kfree(v->signature_key_desc);
 
+	if (v->use_tasklet)
+		static_branch_dec(&use_tasklet_enabled);
+
 	kfree(v);
 }
 
@@ -1075,6 +1083,7 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
 
 		} else if (!strcasecmp(arg_name, DM_VERITY_OPT_TASKLET_VERIFY)) {
 			v->use_tasklet = true;
+			static_branch_inc(&use_tasklet_enabled);
 			continue;
 
 		} else if (verity_is_fec_opt_arg(arg_name)) {
-- 
2.32.1 (Apple Git-133)

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

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

[Nathan Huckleberry]

Hey Mike,

On Tue, Jul 26, 2022 at 9:10 AM Mike Snitzer <snitzer@kernel.org> 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).

In my testing, WQ_HIGHPRI reduced latency in both cases. I tested how
long each configuration spent waiting for work-queue jobs to be
scheduled. The numbers look consistent across the three runs I did.
See below.

Total verity work-queue wait times (ms):
Normal WQ: 880.960, 789.517, 898.852
High Priority WQ: 528.824, 439.191, 433.300
Tasklets + Normal WQ: 242.594, 145.106, 272.642
Tasklets + High Priority WQ: 85.343, 60.787, 70.620

WQ_HIGHPRI is useful even if try_verify_in_tasklet is not used.

Thanks,
Huck


>
> I've tested using cryptsetup's testsuite (which has dm-verity tests)
> but I haven't tested the "try_verify_in_tasklet" feature.
>
> I'd welcome review and focused "try_verify_in_tasklet" testing.
>
> Thanks,
> Mike
>
> Mike Snitzer (3):
>   dm verity: allow optional args to alter primary args handling
>   dm bufio: conditionally enable branching for DM_BUFIO_CLIENT_NO_SLEEP
>   dm verity: conditionally enable branching for "try_verify_in_tasklet"
>
> Nathan Huckleberry (3):
>   dm bufio: Add flags argument to dm_bufio_client_create
>   dm bufio: Add DM_BUFIO_CLIENT_NO_SLEEP flag
>   dm verity: Add optional "try_verify_in_tasklet" feature
>
>  drivers/md/dm-bufio.c                         |  32 ++++-
>  drivers/md/dm-ebs-target.c                    |   3 +-
>  drivers/md/dm-integrity.c                     |   2 +-
>  drivers/md/dm-snap-persistent.c               |   2 +-
>  drivers/md/dm-verity-fec.c                    |   4 +-
>  drivers/md/dm-verity-target.c                 | 121 +++++++++++++++---
>  drivers/md/dm-verity.h                        |   7 +-
>  drivers/md/persistent-data/dm-block-manager.c |   3 +-
>  include/linux/dm-bufio.h                      |   8 +-
>  9 files changed, 154 insertions(+), 28 deletions(-)
>
> --
> 2.32.1 (Apple Git-133)
>

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

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

[Milan Broz]

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.

Hi Mike,

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).

But with your patches (on top of 5.19.0-rc8) and my testing 32bit VM:

- FEC tests are skipped even if FEC is enabled in kernel.
I think the whole FEC is broken with your patches.
(Beware, test will skip FEC quietly! Usually CONFIG_DM_VERITY_FEC is disabled,
so we have to ignore it.)

- Also I see this warning (in that simple option test I added).

: =====================================================
: WARNING: SOFTIRQ-safe -> SOFTIRQ-unsafe lock order detected
: 5.19.0-rc8+ #767 Not tainted
: -----------------------------------------------------
: kworker/u16:6/2488 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
: f7a38090 (global_spinlock){+.+.}-{2:2}, at: adjust_total_allocated+0x95/0x120 [dm_bufio]
: \x0aand this task is already holding:
: c555086c (&c->spinlock){..-.}-{2:2}, at: dm_bufio_lock+0x54/0x60 [dm_bufio]
: which would create a new lock dependency:
:  (&c->spinlock){..-.}-{2:2} -> (global_spinlock){+.+.}-{2:2}
: \x0abut this new dependency connects a SOFTIRQ-irq-safe lock:
:  (&c->spinlock){..-.}-{2:2}
: \x0a... which became SOFTIRQ-irq-safe at:
:   lock_acquire+0xb2/0x2b0
:   _raw_spin_lock_irqsave_nested+0x3b/0x90
:   dm_bufio_lock+0x54/0x60 [dm_bufio]
:   new_read+0x32/0x120 [dm_bufio]
:   dm_bufio_get+0xd/0x10 [dm_bufio]
:   verity_verify_level+0x199/0x220 [dm_verity]
:   verity_hash_for_block+0x26/0xf0 [dm_verity]
:   verity_verify_io+0x134/0x490 [dm_verity]
:   verity_tasklet+0xf/0x7f [dm_verity]
:   tasklet_action_common.constprop.0+0xd0/0xf0
:   tasklet_action+0x21/0x30
:   __do_softirq+0xb4/0x4c5
:   run_ksoftirqd+0x35/0x50
:   smpboot_thread_fn+0x174/0x230
:   kthread+0xd2/0x100
:   ret_from_fork+0x19/0x24
: \x0ato a SOFTIRQ-irq-unsafe lock:
:  (global_spinlock){+.+.}-{2:2}
: \x0a... which became SOFTIRQ-irq-unsafe at:
: ...
:   lock_acquire+0xb2/0x2b0
:   _raw_spin_lock+0x28/0x70
:   adjust_total_allocated+0x95/0x120 [dm_bufio]
:   __link_buffer+0xb2/0xf0 [dm_bufio]
:   __bufio_new+0x20b/0x2b0 [dm_bufio]
:   dm_bufio_prefetch+0x90/0x1f0 [dm_bufio]
:   verity_prefetch_io+0x142/0x180 [dm_verity]
:   process_one_work+0x246/0x530
:   worker_thread+0x47/0x3e0
:   kthread+0xd2/0x100
:   ret_from_fork+0x19/0x24
: \x0aother info that might help us debug this:\x0a
:  Possible interrupt unsafe locking scenario:\x0a
:        CPU0                    CPU1
:        ----                    ----
:   lock(global_spinlock);
:                                local_irq_disable();
:                                lock(&c->spinlock);
:                                lock(global_spinlock);
:   <Interrupt>
:     lock(&c->spinlock);
: \x0a *** DEADLOCK ***\x0a
: 3 locks held by kworker/u16:6/2488:
:  #0: c55494b8 ((wq_completion)kverityd){+.+.}-{0:0}, at: process_one_work+0x1d0/0x530
:  #1: c6c49f38 ((work_completion)(&pw->work)){+.+.}-{0:0}, at: process_one_work+0x1d0/0x530
:  #2: c555086c (&c->spinlock){..-.}-{2:2}, at: dm_bufio_lock+0x54/0x60 [dm_bufio]
: \x0athe dependencies between SOFTIRQ-irq-safe lock and the holding lock:
: -> (&c->spinlock){..-.}-{2:2} ops: 2 {
:    IN-SOFTIRQ-W at:
:                     lock_acquire+0xb2/0x2b0
:                     _raw_spin_lock_irqsave_nested+0x3b/0x90
:                     dm_bufio_lock+0x54/0x60 [dm_bufio]
:                     new_read+0x32/0x120 [dm_bufio]
:                     dm_bufio_get+0xd/0x10 [dm_bufio]
:                     verity_verify_level+0x199/0x220 [dm_verity]
:                     verity_hash_for_block+0x26/0xf0 [dm_verity]
:                     verity_verify_io+0x134/0x490 [dm_verity]
:                     verity_tasklet+0xf/0x7f [dm_verity]
:                     tasklet_action_common.constprop.0+0xd0/0xf0
:                     tasklet_action+0x21/0x30
:                     __do_softirq+0xb4/0x4c5
:                     run_ksoftirqd+0x35/0x50
:                     smpboot_thread_fn+0x174/0x230
:                     kthread+0xd2/0x100
:                     ret_from_fork+0x19/0x24
:    INITIAL USE at:
:                    lock_acquire+0xb2/0x2b0
:                    _raw_spin_lock_irqsave_nested+0x3b/0x90
:                    dm_bufio_lock+0x54/0x60 [dm_bufio]
:                    dm_bufio_prefetch+0x4b/0x1f0 [dm_bufio]
:                    verity_prefetch_io+0x3c/0x180 [dm_verity]
:                    process_one_work+0x246/0x530
:                    worker_thread+0x47/0x3e0
:                    kthread+0xd2/0x100
:                    ret_from_fork+0x19/0x24
:  }
:  ... key      at: [<f7a384e8>] __key.24+0x0/0xffffeb18 [dm_bufio]
: \x0athe dependencies between the lock to be acquired
:  and SOFTIRQ-irq-unsafe lock:
: -> (global_spinlock){+.+.}-{2:2} ops: 129329 {
:    HARDIRQ-ON-W at:
:                     lock_acquire+0xb2/0x2b0
:                     _raw_spin_lock+0x28/0x70
:                     adjust_total_allocated+0x95/0x120 [dm_bufio]
:                     __link_buffer+0xb2/0xf0 [dm_bufio]
:                     __bufio_new+0x20b/0x2b0 [dm_bufio]
:                     dm_bufio_prefetch+0x90/0x1f0 [dm_bufio]
:                     verity_prefetch_io+0x142/0x180 [dm_verity]
:                     process_one_work+0x246/0x530
:                     worker_thread+0x47/0x3e0
:                     kthread+0xd2/0x100
:                     ret_from_fork+0x19/0x24
:    SOFTIRQ-ON-W at:
:                     lock_acquire+0xb2/0x2b0
:                     _raw_spin_lock+0x28/0x70
:                     adjust_total_allocated+0x95/0x120 [dm_bufio]
:                     __link_buffer+0xb2/0xf0 [dm_bufio]
:                     __bufio_new+0x20b/0x2b0 [dm_bufio]
:                     dm_bufio_prefetch+0x90/0x1f0 [dm_bufio]
:                     verity_prefetch_io+0x142/0x180 [dm_verity]
:                     process_one_work+0x246/0x530
:                     worker_thread+0x47/0x3e0
:                     kthread+0xd2/0x100
:                     ret_from_fork+0x19/0x24
:    INITIAL USE at:
:                    lock_acquire+0xb2/0x2b0
:                    _raw_spin_lock+0x28/0x70
:                    adjust_total_allocated+0x95/0x120 [dm_bufio]
:                    __link_buffer+0xb2/0xf0 [dm_bufio]
:                    __bufio_new+0x20b/0x2b0 [dm_bufio]
:                    dm_bufio_prefetch+0x90/0x1f0 [dm_bufio]
:                    verity_prefetch_io+0x142/0x180 [dm_verity]
:                    process_one_work+0x246/0x530
:                    worker_thread+0x47/0x3e0
:                    kthread+0xd2/0x100
:                    ret_from_fork+0x19/0x24
:  }
:  ... key      at: [<f7a38090>] global_spinlock+0x10/0xffffef80 [dm_bufio]
:  ... acquired at:
:    lock_acquire+0xb2/0x2b0
:    _raw_spin_lock+0x28/0x70
:    adjust_total_allocated+0x95/0x120 [dm_bufio]
:    __link_buffer+0xb2/0xf0 [dm_bufio]
:    __bufio_new+0x20b/0x2b0 [dm_bufio]
:    dm_bufio_prefetch+0x90/0x1f0 [dm_bufio]
:    verity_prefetch_io+0x3c/0x180 [dm_verity]
:    process_one_work+0x246/0x530
:    worker_thread+0x47/0x3e0
:    kthread+0xd2/0x100
:    ret_from_fork+0x19/0x24
:
: \x0astack backtrace:
: CPU: 1 PID: 2488 Comm: kworker/u16:6 Not tainted 5.19.0-rc8+ #767
: Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 11/12/2020
: Workqueue: kverityd verity_prefetch_io [dm_verity]
: Call Trace:
:  dump_stack_lvl+0x68/0x98
:  dump_stack+0xd/0x10
:  print_bad_irq_dependency.cold+0x1f2/0x1f8
:  __lock_acquire+0x2522/0x2840
:  ? __this_cpu_preempt_check+0xf/0x11
:  lock_acquire+0xb2/0x2b0
:  ? adjust_total_allocated+0x95/0x120 [dm_bufio]
:  ? __this_cpu_preempt_check+0xf/0x11
:  _raw_spin_lock+0x28/0x70
:  ? adjust_total_allocated+0x95/0x120 [dm_bufio]
:  adjust_total_allocated+0x95/0x120 [dm_bufio]
:  __link_buffer+0xb2/0xf0 [dm_bufio]
:  ? alloc_buffer+0xc3/0x100 [dm_bufio]
:  __bufio_new+0x20b/0x2b0 [dm_bufio]
:  dm_bufio_prefetch+0x90/0x1f0 [dm_bufio]
:  verity_prefetch_io+0x3c/0x180 [dm_verity]
:  process_one_work+0x246/0x530
:  ? 0xc1000000
:  worker_thread+0x47/0x3e0
:  kthread+0xd2/0x100
:  ? process_one_work+0x530/0x530
:  ? kthread_complete_and_exit+0x20/0x20
:  ret_from_fork+0x19/0x24


m.

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

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

[Mike Snitzer]

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 ;)

> Hi Mike,
> 
> 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?

(I only ever run "make check", I'm not a power user with cryptsetup's
testing. So any pointers on how best to focus on dm-verity testing
using the existing make targets and options would be welcomed.

But I looked at your new test and if I understand it correctly: it
just loads a test device with "try_verify_in_tasklet" set in the
optional args.

(safe to assume any IO issued to the device is a sideffect of udev
scripts?)

But yeah, you haven't even tried to break it yet.. and it fell over ;)

> But with your patches (on top of 5.19.0-rc8) and my testing 32bit VM:
> 
> - FEC tests are skipped even if FEC is enabled in kernel.
> I think the whole FEC is broken with your patches.
> (Beware, test will skip FEC quietly! Usually CONFIG_DM_VERITY_FEC is disabled,
> so we have to ignore it.)

with tasklets the FEC code path should always punt to the workqueue.
Which is free to sleep.  But the following lockdep tracing you
provided seems to be concerned about the dm_bufio_get in tasklet
(having locked w/ irqsave) vs dm_bufio_prefetch in workqueue locking
global_spinlock with spin_lock (not _irqsave).

I'm just missing the link between the two, as in: how would the
dm_bufio_get directly get to dm_bufio_prefetch without first bouncing
through the workqueue? (answer should always be: you can't).

If the lockdep report shows how, I'm missing it (with suboptimal
reading ability of lockdep traces).

Oh.. I think it's just saying that since all the locking is with a
spinlock, no matter the context, there is potential for issues, with
inconsistent locking.  It isn't saying it actually triggered the
deadlock. Basically seems like false-positive (workqueue won't ever be
in interrupt context).

I wonder if adjust_total_allocated() should just grow some lockdep
annotation to silence lockdep rather than go to the extreme of it
using _irqsave (but I'll go with that first).

Thanks,
Mike

> - Also I see this warning (in that simple option test I added).
> 
> : =====================================================
> : WARNING: SOFTIRQ-safe -> SOFTIRQ-unsafe lock order detected
> : 5.19.0-rc8+ #767 Not tainted
> : -----------------------------------------------------
> : kworker/u16:6/2488 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
> : f7a38090 (global_spinlock){+.+.}-{2:2}, at: adjust_total_allocated+0x95/0x120 [dm_bufio]
> : \x0aand this task is already holding:
> : c555086c (&c->spinlock){..-.}-{2:2}, at: dm_bufio_lock+0x54/0x60 [dm_bufio]
> : which would create a new lock dependency:
> :  (&c->spinlock){..-.}-{2:2} -> (global_spinlock){+.+.}-{2:2}
> : \x0abut this new dependency connects a SOFTIRQ-irq-safe lock:
> :  (&c->spinlock){..-.}-{2:2}
> : \x0a... which became SOFTIRQ-irq-safe at:
> :   lock_acquire+0xb2/0x2b0
> :   _raw_spin_lock_irqsave_nested+0x3b/0x90
> :   dm_bufio_lock+0x54/0x60 [dm_bufio]
> :   new_read+0x32/0x120 [dm_bufio]
> :   dm_bufio_get+0xd/0x10 [dm_bufio]
> :   verity_verify_level+0x199/0x220 [dm_verity]
> :   verity_hash_for_block+0x26/0xf0 [dm_verity]
> :   verity_verify_io+0x134/0x490 [dm_verity]
> :   verity_tasklet+0xf/0x7f [dm_verity]
> :   tasklet_action_common.constprop.0+0xd0/0xf0
> :   tasklet_action+0x21/0x30
> :   __do_softirq+0xb4/0x4c5
> :   run_ksoftirqd+0x35/0x50
> :   smpboot_thread_fn+0x174/0x230
> :   kthread+0xd2/0x100
> :   ret_from_fork+0x19/0x24
> : \x0ato a SOFTIRQ-irq-unsafe lock:
> :  (global_spinlock){+.+.}-{2:2}
> : \x0a... which became SOFTIRQ-irq-unsafe at:
> : ...
> :   lock_acquire+0xb2/0x2b0
> :   _raw_spin_lock+0x28/0x70
> :   adjust_total_allocated+0x95/0x120 [dm_bufio]
> :   __link_buffer+0xb2/0xf0 [dm_bufio]
> :   __bufio_new+0x20b/0x2b0 [dm_bufio]
> :   dm_bufio_prefetch+0x90/0x1f0 [dm_bufio]
> :   verity_prefetch_io+0x142/0x180 [dm_verity]
> :   process_one_work+0x246/0x530
> :   worker_thread+0x47/0x3e0
> :   kthread+0xd2/0x100
> :   ret_from_fork+0x19/0x24
> : \x0aother info that might help us debug this:\x0a
> :  Possible interrupt unsafe locking scenario:\x0a
> :        CPU0                    CPU1
> :        ----                    ----
> :   lock(global_spinlock);
> :                                local_irq_disable();
> :                                lock(&c->spinlock);
> :                                lock(global_spinlock);
> :   <Interrupt>
> :     lock(&c->spinlock);
> : \x0a *** DEADLOCK ***\x0a
> : 3 locks held by kworker/u16:6/2488:
> :  #0: c55494b8 ((wq_completion)kverityd){+.+.}-{0:0}, at: process_one_work+0x1d0/0x530
> :  #1: c6c49f38 ((work_completion)(&pw->work)){+.+.}-{0:0}, at: process_one_work+0x1d0/0x530
> :  #2: c555086c (&c->spinlock){..-.}-{2:2}, at: dm_bufio_lock+0x54/0x60 [dm_bufio]
> : \x0athe dependencies between SOFTIRQ-irq-safe lock and the holding lock:
> : -> (&c->spinlock){..-.}-{2:2} ops: 2 {
> :    IN-SOFTIRQ-W at:
> :                     lock_acquire+0xb2/0x2b0
> :                     _raw_spin_lock_irqsave_nested+0x3b/0x90
> :                     dm_bufio_lock+0x54/0x60 [dm_bufio]
> :                     new_read+0x32/0x120 [dm_bufio]
> :                     dm_bufio_get+0xd/0x10 [dm_bufio]
> :                     verity_verify_level+0x199/0x220 [dm_verity]
> :                     verity_hash_for_block+0x26/0xf0 [dm_verity]
> :                     verity_verify_io+0x134/0x490 [dm_verity]
> :                     verity_tasklet+0xf/0x7f [dm_verity]
> :                     tasklet_action_common.constprop.0+0xd0/0xf0
> :                     tasklet_action+0x21/0x30
> :                     __do_softirq+0xb4/0x4c5
> :                     run_ksoftirqd+0x35/0x50
> :                     smpboot_thread_fn+0x174/0x230
> :                     kthread+0xd2/0x100
> :                     ret_from_fork+0x19/0x24
> :    INITIAL USE at:
> :                    lock_acquire+0xb2/0x2b0
> :                    _raw_spin_lock_irqsave_nested+0x3b/0x90
> :                    dm_bufio_lock+0x54/0x60 [dm_bufio]
> :                    dm_bufio_prefetch+0x4b/0x1f0 [dm_bufio]
> :                    verity_prefetch_io+0x3c/0x180 [dm_verity]
> :                    process_one_work+0x246/0x530
> :                    worker_thread+0x47/0x3e0
> :                    kthread+0xd2/0x100
> :                    ret_from_fork+0x19/0x24
> :  }
> :  ... key      at: [<f7a384e8>] __key.24+0x0/0xffffeb18 [dm_bufio]
> : \x0athe dependencies between the lock to be acquired
> :  and SOFTIRQ-irq-unsafe lock:
> : -> (global_spinlock){+.+.}-{2:2} ops: 129329 {
> :    HARDIRQ-ON-W at:
> :                     lock_acquire+0xb2/0x2b0
> :                     _raw_spin_lock+0x28/0x70
> :                     adjust_total_allocated+0x95/0x120 [dm_bufio]
> :                     __link_buffer+0xb2/0xf0 [dm_bufio]
> :                     __bufio_new+0x20b/0x2b0 [dm_bufio]
> :                     dm_bufio_prefetch+0x90/0x1f0 [dm_bufio]
> :                     verity_prefetch_io+0x142/0x180 [dm_verity]
> :                     process_one_work+0x246/0x530
> :                     worker_thread+0x47/0x3e0
> :                     kthread+0xd2/0x100
> :                     ret_from_fork+0x19/0x24
> :    SOFTIRQ-ON-W at:
> :                     lock_acquire+0xb2/0x2b0
> :                     _raw_spin_lock+0x28/0x70
> :                     adjust_total_allocated+0x95/0x120 [dm_bufio]
> :                     __link_buffer+0xb2/0xf0 [dm_bufio]
> :                     __bufio_new+0x20b/0x2b0 [dm_bufio]
> :                     dm_bufio_prefetch+0x90/0x1f0 [dm_bufio]
> :                     verity_prefetch_io+0x142/0x180 [dm_verity]
> :                     process_one_work+0x246/0x530
> :                     worker_thread+0x47/0x3e0
> :                     kthread+0xd2/0x100
> :                     ret_from_fork+0x19/0x24
> :    INITIAL USE at:
> :                    lock_acquire+0xb2/0x2b0
> :                    _raw_spin_lock+0x28/0x70
> :                    adjust_total_allocated+0x95/0x120 [dm_bufio]
> :                    __link_buffer+0xb2/0xf0 [dm_bufio]
> :                    __bufio_new+0x20b/0x2b0 [dm_bufio]
> :                    dm_bufio_prefetch+0x90/0x1f0 [dm_bufio]
> :                    verity_prefetch_io+0x142/0x180 [dm_verity]
> :                    process_one_work+0x246/0x530
> :                    worker_thread+0x47/0x3e0
> :                    kthread+0xd2/0x100
> :                    ret_from_fork+0x19/0x24
> :  }
> :  ... key      at: [<f7a38090>] global_spinlock+0x10/0xffffef80 [dm_bufio]
> :  ... acquired at:
> :    lock_acquire+0xb2/0x2b0
> :    _raw_spin_lock+0x28/0x70
> :    adjust_total_allocated+0x95/0x120 [dm_bufio]
> :    __link_buffer+0xb2/0xf0 [dm_bufio]
> :    __bufio_new+0x20b/0x2b0 [dm_bufio]
> :    dm_bufio_prefetch+0x90/0x1f0 [dm_bufio]
> :    verity_prefetch_io+0x3c/0x180 [dm_verity]
> :    process_one_work+0x246/0x530
> :    worker_thread+0x47/0x3e0
> :    kthread+0xd2/0x100
> :    ret_from_fork+0x19/0x24
> :
> : \x0astack backtrace:
> : CPU: 1 PID: 2488 Comm: kworker/u16:6 Not tainted 5.19.0-rc8+ #767
> : Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 11/12/2020
> : Workqueue: kverityd verity_prefetch_io [dm_verity]
> : Call Trace:
> :  dump_stack_lvl+0x68/0x98
> :  dump_stack+0xd/0x10
> :  print_bad_irq_dependency.cold+0x1f2/0x1f8
> :  __lock_acquire+0x2522/0x2840
> :  ? __this_cpu_preempt_check+0xf/0x11
> :  lock_acquire+0xb2/0x2b0
> :  ? adjust_total_allocated+0x95/0x120 [dm_bufio]
> :  ? __this_cpu_preempt_check+0xf/0x11
> :  _raw_spin_lock+0x28/0x70
> :  ? adjust_total_allocated+0x95/0x120 [dm_bufio]
> :  adjust_total_allocated+0x95/0x120 [dm_bufio]
> :  __link_buffer+0xb2/0xf0 [dm_bufio]
> :  ? alloc_buffer+0xc3/0x100 [dm_bufio]
> :  __bufio_new+0x20b/0x2b0 [dm_bufio]
> :  dm_bufio_prefetch+0x90/0x1f0 [dm_bufio]
> :  verity_prefetch_io+0x3c/0x180 [dm_verity]
> :  process_one_work+0x246/0x530
> :  ? 0xc1000000
> :  worker_thread+0x47/0x3e0
> :  kthread+0xd2/0x100
> :  ? process_one_work+0x530/0x530
> :  ? kthread_complete_and_exit+0x20/0x20
> :  ret_from_fork+0x19/0x24
> 
> 
> m.
> 
> --
> 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

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

[Milan Broz]

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...

(Side note: we do not have any easy way to check that dm-verity
is compiled without FEC. Target version is the same, and I do not
want to introduce any config parsing in libcryptsetup....
The same perhaps applies to other targets, any idea?
I would help us to error the test in that case more clearly.)

Milan

Jul 27 10:10:20 : device-mapper: verity: 7:2: reached maximum errors
Jul 27 10:10:21 : loop1: detected capacity change from 0 to 1094
Jul 27 10:10:21 : device-mapper: verity: sha256 using implementation "sha256-generic"
Jul 27 10:10:22 : loop1: detected capacity change from 0 to 1094
Jul 27 10:10:22 : device-mapper: verity: sha256 using implementation "sha256-generic"
Jul 27 10:15:29 : INFO: task systemd-udevd:6842 blocked for more than 122 seconds.
Jul 27 10:15:29 :       Not tainted 5.19.0-rc8+ #767
Jul 27 10:15:29 : "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
Jul 27 10:15:29 : task:systemd-udevd   state:D stack:    0 pid: 6842 ppid:   540 flags:0x00004006
Jul 27 10:15:29 : Call Trace:
Jul 27 10:15:29 :  __schedule+0x3f9/0xcc0
Jul 27 10:15:29 :  schedule+0x40/0xb0
Jul 27 10:15:29 :  io_schedule+0x3c/0x60
Jul 27 10:15:29 :  folio_wait_bit_common+0xe2/0x310
Jul 27 10:15:29 :  ? filemap_invalidate_unlock_two+0x40/0x40
Jul 27 10:15:29 :  __filemap_get_folio+0x4d8/0x500
Jul 27 10:15:29 :  truncate_inode_pages_range+0x1c2/0x4e0
Jul 27 10:15:29 :  ? lockdep_hardirqs_on+0xbf/0x150
Jul 27 10:15:29 :  ? smp_call_function_many_cond+0x2e5/0x310
Jul 27 10:15:29 :  ? on_each_cpu_cond_mask+0x32/0x60
Jul 27 10:15:29 :  ? trace_hardirqs_on+0x35/0xd0
Jul 27 10:15:29 :  ? smp_call_function_many_cond+0xe9/0x310
Jul 27 10:15:29 :  ? buffer_exit_cpu_dead+0x80/0x80
Jul 27 10:15:29 :  ? buffer_exit_cpu_dead+0x80/0x80
Jul 27 10:15:29 :  ? generic_remap_file_range_prep+0xcb0/0xcb0
Jul 27 10:15:29 :  ? on_each_cpu_cond_mask+0x3c/0x60
Jul 27 10:15:29 :  ? generic_remap_file_range_prep+0xcb0/0xcb0
Jul 27 10:15:29 :  truncate_inode_pages+0xc/0x10
Jul 27 10:15:29 :  blkdev_flush_mapping+0x66/0x100
Jul 27 10:15:29 :  blkdev_put_whole+0x38/0x40
Jul 27 10:15:29 :  blkdev_put+0x92/0x1c0
Jul 27 10:15:29 :  blkdev_close+0x13/0x20
Jul 27 10:15:29 :  __fput+0x80/0x270
Jul 27 10:15:29 :  ? lockdep_hardirqs_on+0xbf/0x150
Jul 27 10:15:29 :  ____fput+0x8/0x10
Jul 27 10:15:29 :  task_work_run+0x4f/0x80
Jul 27 10:15:29 :  do_exit+0x315/0x9a0
Jul 27 10:15:29 :  ? lockdep_hardirqs_on+0xbf/0x150
Jul 27 10:15:29 :  do_group_exit+0x26/0x90
Jul 27 10:15:29 :  get_signal+0xa2d/0xa60
Jul 27 10:15:29 :  arch_do_signal_or_restart+0x1e/0x5c0
Jul 27 10:15:29 :  ? __this_cpu_preempt_check+0xf/0x11
Jul 27 10:15:29 :  ? lockdep_hardirqs_on+0xbf/0x150
Jul 27 10:15:29 :  ? exit_to_user_mode_prepare+0x10f/0x250
Jul 27 10:15:29 :  ? syscall_exit_to_user_mode+0x1a/0x50
Jul 27 10:15:29 :  exit_to_user_mode_prepare+0x129/0x250
Jul 27 10:15:29 :  syscall_exit_to_user_mode+0x1a/0x50
Jul 27 10:15:29 :  do_int80_syscall_32+0x3c/0x90
Jul 27 10:15:29 :  entry_INT80_32+0xf0/0xf0
Jul 27 10:15:29 : EIP: 0xb7f61022
Jul 27 10:15:29 : EAX: fffffffc EBX: 00000005 ECX: 0054cbdc EDX: 00000100
Jul 27 10:15:29 : ESI: b7ea8000 EDI: 00570820 EBP: 00570868 ESP: bfc0890c
Jul 27 10:15:29 : DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 007b EFLAGS: 00000246
Jul 27 10:15:29 : \x0aShowing all locks held in the system:
Jul 27 10:15:29 : 1 lock held by khungtaskd/34:
Jul 27 10:15:29 :  #0: c1b0bc98 (rcu_read_lock){....}-{1:2}, at: debug_show_all_locks+0x1f/0x152
Jul 27 10:15:29 : 2 locks held by kworker/u16:5/6148:
Jul 27 10:15:29 : 1 lock held by systemd-udevd/6842:
Jul 27 10:15:29 :  #0: c51344bc (&disk->open_mutex){+.+.}-{3:3}, at: blkdev_put+0x30/0x1c0
Jul 27 10:15:29 :
Jul 27 10:15:29 : =============================================\x0a

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

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

[Nathan Huckleberry]

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




>
> (Side note: we do not have any easy way to check that dm-verity
> is compiled without FEC. Target version is the same, and I do not
> want to introduce any config parsing in libcryptsetup....
> The same perhaps applies to other targets, any idea?
> I would help us to error the test in that case more clearly.)
>
> Milan
>
> Jul 27 10:10:20 : device-mapper: verity: 7:2: reached maximum errors
> Jul 27 10:10:21 : loop1: detected capacity change from 0 to 1094
> Jul 27 10:10:21 : device-mapper: verity: sha256 using implementation "sha256-generic"
> Jul 27 10:10:22 : loop1: detected capacity change from 0 to 1094
> Jul 27 10:10:22 : device-mapper: verity: sha256 using implementation "sha256-generic"
> Jul 27 10:15:29 : INFO: task systemd-udevd:6842 blocked for more than 122 seconds.
> Jul 27 10:15:29 :       Not tainted 5.19.0-rc8+ #767
> Jul 27 10:15:29 : "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> Jul 27 10:15:29 : task:systemd-udevd   state:D stack:    0 pid: 6842 ppid:   540 flags:0x00004006
> Jul 27 10:15:29 : Call Trace:
> Jul 27 10:15:29 :  __schedule+0x3f9/0xcc0
> Jul 27 10:15:29 :  schedule+0x40/0xb0
> Jul 27 10:15:29 :  io_schedule+0x3c/0x60
> Jul 27 10:15:29 :  folio_wait_bit_common+0xe2/0x310
> Jul 27 10:15:29 :  ? filemap_invalidate_unlock_two+0x40/0x40
> Jul 27 10:15:29 :  __filemap_get_folio+0x4d8/0x500
> Jul 27 10:15:29 :  truncate_inode_pages_range+0x1c2/0x4e0
> Jul 27 10:15:29 :  ? lockdep_hardirqs_on+0xbf/0x150
> Jul 27 10:15:29 :  ? smp_call_function_many_cond+0x2e5/0x310
> Jul 27 10:15:29 :  ? on_each_cpu_cond_mask+0x32/0x60
> Jul 27 10:15:29 :  ? trace_hardirqs_on+0x35/0xd0
> Jul 27 10:15:29 :  ? smp_call_function_many_cond+0xe9/0x310
> Jul 27 10:15:29 :  ? buffer_exit_cpu_dead+0x80/0x80
> Jul 27 10:15:29 :  ? buffer_exit_cpu_dead+0x80/0x80
> Jul 27 10:15:29 :  ? generic_remap_file_range_prep+0xcb0/0xcb0
> Jul 27 10:15:29 :  ? on_each_cpu_cond_mask+0x3c/0x60
> Jul 27 10:15:29 :  ? generic_remap_file_range_prep+0xcb0/0xcb0
> Jul 27 10:15:29 :  truncate_inode_pages+0xc/0x10
> Jul 27 10:15:29 :  blkdev_flush_mapping+0x66/0x100
> Jul 27 10:15:29 :  blkdev_put_whole+0x38/0x40
> Jul 27 10:15:29 :  blkdev_put+0x92/0x1c0
> Jul 27 10:15:29 :  blkdev_close+0x13/0x20
> Jul 27 10:15:29 :  __fput+0x80/0x270
> Jul 27 10:15:29 :  ? lockdep_hardirqs_on+0xbf/0x150
> Jul 27 10:15:29 :  ____fput+0x8/0x10
> Jul 27 10:15:29 :  task_work_run+0x4f/0x80
> Jul 27 10:15:29 :  do_exit+0x315/0x9a0
> Jul 27 10:15:29 :  ? lockdep_hardirqs_on+0xbf/0x150
> Jul 27 10:15:29 :  do_group_exit+0x26/0x90
> Jul 27 10:15:29 :  get_signal+0xa2d/0xa60
> Jul 27 10:15:29 :  arch_do_signal_or_restart+0x1e/0x5c0
> Jul 27 10:15:29 :  ? __this_cpu_preempt_check+0xf/0x11
> Jul 27 10:15:29 :  ? lockdep_hardirqs_on+0xbf/0x150
> Jul 27 10:15:29 :  ? exit_to_user_mode_prepare+0x10f/0x250
> Jul 27 10:15:29 :  ? syscall_exit_to_user_mode+0x1a/0x50
> Jul 27 10:15:29 :  exit_to_user_mode_prepare+0x129/0x250
> Jul 27 10:15:29 :  syscall_exit_to_user_mode+0x1a/0x50
> Jul 27 10:15:29 :  do_int80_syscall_32+0x3c/0x90
> Jul 27 10:15:29 :  entry_INT80_32+0xf0/0xf0
> Jul 27 10:15:29 : EIP: 0xb7f61022
> Jul 27 10:15:29 : EAX: fffffffc EBX: 00000005 ECX: 0054cbdc EDX: 00000100
> Jul 27 10:15:29 : ESI: b7ea8000 EDI: 00570820 EBP: 00570868 ESP: bfc0890c
> Jul 27 10:15:29 : DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 007b EFLAGS: 00000246
> Jul 27 10:15:29 : \x0aShowing all locks held in the system:
> Jul 27 10:15:29 : 1 lock held by khungtaskd/34:
> Jul 27 10:15:29 :  #0: c1b0bc98 (rcu_read_lock){....}-{1:2}, at: debug_show_all_locks+0x1f/0x152
> Jul 27 10:15:29 : 2 locks held by kworker/u16:5/6148:
> Jul 27 10:15:29 : 1 lock held by systemd-udevd/6842:
> Jul 27 10:15:29 :  #0: c51344bc (&disk->open_mutex){+.+.}-{3:3}, at: blkdev_put+0x30/0x1c0
> Jul 27 10:15:29 :
> Jul 27 10:15:29 : =============================================\x0a

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

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

[Mike Snitzer]

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

Re: [dm-devel] [PATCH] Fixes 6890e9b8c7d0a1062bbf4f854b6be3723836ad9a

[Nathan Huckleberry]

The previous patch had a bug when hash verification failed in the
tasklet.  This happened because the tasklet has already advanced the
bvec_iter when it falls back to the work-queue implementation.  When the
work-queue job begins running, it attempts to restart verifiying at
block i, but the bvec_iter is at block i+1.

This causes several failures, the most noticeable is that there are not
enough bytes in the bvec_iter to finish verification.  Since there are
not enough bytes to finish, the work queue gets stuck in an infinite
loop in verity_for_io_block.

To fix this, we can make a local copy of the bvec_iter struct in
verity_verify_io.  This requires that we restart verification from the
first block when deferring to a work-queue rather than starting from the
last block verified in the tasklet.

This patch also fixes what appears to be a memory leak when FEC is
enabled.  The call to verity_fec_init_io should only be made if we are
in a work-queue since a tasklet does not use FEC and is unable to free
the memory.

Signed-off-by: Nathan Huckleberry <nhuck@google.com>
---
 drivers/md/dm-verity-target.c | 23 ++++++++++++++---------
 drivers/md/dm-verity.h        |  1 -
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index 144889ba43ea..0677795454d7 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -499,17 +499,23 @@ static int verity_verify_io(struct dm_verity_io *io)
 	bool is_zero;
 	struct dm_verity *v = io->v;
 	struct bvec_iter start;
+	/*
+	 * Copy the iterator in case we need to restart verification in a
+	 * work-queue.
+	 */
+	struct bvec_iter iter_copy = io->iter;
 	struct crypto_wait wait;
 	struct bio *bio = dm_bio_from_per_bio_data(io, v->ti->per_io_data_size);
+	unsigned int b;
 
-	for (; io->block_idx < io->n_blocks; io->block_idx++) {
+	for (b = 0; b < io->n_blocks; b++) {
 		int r;
-		sector_t cur_block = io->block + io->block_idx;
+		sector_t cur_block = io->block + b;
 		struct ahash_request *req = verity_io_hash_req(v, io);
 
 		if (v->validated_blocks &&
 		    likely(test_bit(cur_block, v->validated_blocks))) {
-			verity_bv_skip_block(v, io, &io->iter);
+			verity_bv_skip_block(v, io, &iter_copy);
 			continue;
 		}
 
@@ -524,7 +530,7 @@ static int verity_verify_io(struct dm_verity_io *io)
 			 * If we expect a zero block, don't validate, just
 			 * return zeros.
 			 */
-			r = verity_for_bv_block(v, io, &io->iter,
+			r = verity_for_bv_block(v, io, &iter_copy,
 						verity_bv_zero);
 			if (unlikely(r < 0))
 				return r;
@@ -536,8 +542,8 @@ static int verity_verify_io(struct dm_verity_io *io)
 		if (unlikely(r < 0))
 			return r;
 
-		start = io->iter;
-		r = verity_for_io_block(v, io, &io->iter, &wait);
+		start = iter_copy;
+		r = verity_for_io_block(v, io, &iter_copy, &wait);
 		if (unlikely(r < 0))
 			return r;
 
@@ -608,6 +614,8 @@ static void verity_work(struct work_struct *w)
 	struct dm_verity_io *io = container_of(w, struct dm_verity_io, work);
 
 	io->in_tasklet = false;
+	verity_fec_init_io(io);
+
 	verity_finish_io(io, errno_to_blk_status(verity_verify_io(io)));
 }
 
@@ -638,7 +646,6 @@ static void verity_end_io(struct bio *bio)
 		return;
 	}
 
-	io->block_idx = 0;
 	if (static_branch_unlikely(&use_tasklet_enabled) && io->v->use_tasklet) {
 		tasklet_init(&io->tasklet, verity_tasklet, (unsigned long)io);
 		tasklet_schedule(&io->tasklet);
@@ -756,8 +763,6 @@ static int verity_map(struct dm_target *ti, struct bio *bio)
 	bio->bi_private = io;
 	io->iter = bio->bi_iter;
 
-	verity_fec_init_io(io);
-
 	verity_submit_prefetch(v, io);
 
 	submit_bio_noacct(bio);
diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
index e73a7972c3e9..fb92b22a6404 100644
--- a/drivers/md/dm-verity.h
+++ b/drivers/md/dm-verity.h
@@ -78,7 +78,6 @@ struct dm_verity_io {
 
 	sector_t block;
 	unsigned n_blocks;
-	unsigned int block_idx;
 	bool in_tasklet;
 
 	struct bvec_iter iter;
-- 
2.37.1.455.g008518b4e5-goog

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

Re: [dm-devel] Fixes 6890e9b8c7d0a1062bbf4f854b6be3723836ad9a

[Mike Snitzer]

On Wed, Aug 03 2022 at  2:29P -0400,
Nathan Huckleberry <nhuck@google.com> wrote:

> The previous patch had a bug when hash verification failed in the
> tasklet.  This happened because the tasklet has already advanced the
> bvec_iter when it falls back to the work-queue implementation.  When the
> work-queue job begins running, it attempts to restart verifiying at
> block i, but the bvec_iter is at block i+1.
> 
> This causes several failures, the most noticeable is that there are not
> enough bytes in the bvec_iter to finish verification.  Since there are
> not enough bytes to finish, the work queue gets stuck in an infinite
> loop in verity_for_io_block.
> 
> To fix this, we can make a local copy of the bvec_iter struct in
> verity_verify_io.  This requires that we restart verification from the
> first block when deferring to a work-queue rather than starting from the
> last block verified in the tasklet.
> 
> This patch also fixes what appears to be a memory leak when FEC is
> enabled.  The call to verity_fec_init_io should only be made if we are
> in a work-queue since a tasklet does not use FEC and is unable to free
> the memory.
> 
> Signed-off-by: Nathan Huckleberry <nhuck@google.com>

Thanks for the detailed header, helped me appreciate the details of
your incremental fixes. I folded your fixes in to the original commit
that adds the "try_verify_in_tasklet" feature. I also layered on some
verity_verify_io() optimizations in new commits. Lastly, I added the
use of WQ_HIGHPRI if "try_verify_in_tasklet" feature is used.

The end result passes Milan's testsuite with and without --use-tasklets.

I've staged the changes in linux-next, see:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/log/?h=for-next

There is a chance I'll send another 6.0 pull request to Linus with
these changes tomorrow. We've done quite a bit of work here (_before_
the 6.0 merge window opened) so I do think it'd be best to get these
changes upstream sooner rather than later.

If anyone disagrees with sending these changes upstream now please
speak-up!

Thanks,
Mike

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