Re: [PATCH] md: dm-verity: Fix to avoid a deadlock in dm-bufio

[Mikulas Patocka <mpatocka@redhat.com>]

On Mon, 4 Mar 2013, Paul Taysom wrote:

> On Mon, Mar 4, 2013 at 9:42 AM, Alasdair G Kergon <agk@redhat.com> wrote:
> > On Mon, Mar 04, 2013 at 08:45:48AM -0800, Paul Taysom wrote:
> >> @@ -449,8 +468,14 @@ static void verity_prefetch_io(struct dm_verity *v, struct dm_verity_io *io)
> >>                               hash_block_end = v->hash_blocks - 1;
> >>               }
> >>  no_prefetch_cluster:
> >> -             dm_bufio_prefetch(v->bufio, hash_block_start,
> >> -                               hash_block_end - hash_block_start + 1);
> >> +             vw = kmalloc(sizeof(*vw), GFP_KERNEL);
> >
> > kmalloc?  mempool? ...
> >
> > Alasdair
> >
> The use of mempool would be a separate patch that would have to be
> measured for performance impact.
> -Paul
> 

You don't have to use mempool. Just avoid prefetching if there is not 
enough memory for the prefetch structure.

I reworked the patch, is uses an allocation that can fail and it generates 
just one workqueue entry for one request (the original patch generated one 
workqueue entry for hash tree level).

Please test the patch and if it works and performs well, let's submit it.

Mikulas

---

Changed the dm-verity prefetching to use a worker thread to avoid
a deadlock in dm-bufio.

If generic_make_request is called recursively, it queues the I/O
request on the current->bio_list without making the I/O request
and returns. The routine making the recursive call cannot wait
for the I/O to complete.

The deadlock occurred when one thread grabbed the bufio_client
mutex and waited for an I/O to complete but the I/O was queued
on another thread's current->bio_list and it was waiting to get
the mutex held by the first thread.

The fix allows only one I/O request from dm-verity to dm-bufio
per thread. To do this, the prefetch requests were queued on worker
threads.

In addition to avoiding the deadlock, this fix made a slight
improvement in performance.

seconds_kernel_to_login:
  with prefetch:    8.43s
  without prefetch: 9.2s
  worker prefetch:  8.28s

Signed-off-by: Paul Taysom <taysom@chromium.org>
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@kernel.org
---
 drivers/md/dm-bufio.c  |    2 ++
 drivers/md/dm-verity.c |   37 +++++++++++++++++++++++++++++++++----
 2 files changed, 35 insertions(+), 4 deletions(-)

Index: linux-3.8-fast/drivers/md/dm-verity.c
===================================================================
--- linux-3.8-fast.orig/drivers/md/dm-verity.c	2013-03-04 22:49:20.000000000 +0100
+++ linux-3.8-fast/drivers/md/dm-verity.c	2013-03-04 23:10:45.000000000 +0100
@@ -93,6 +93,13 @@ struct dm_verity_io {
 	 */
 };
 
+struct dm_verity_prefetch_work {
+	struct work_struct work;
+	struct dm_verity *v;
+	sector_t block;
+	unsigned n_blocks;
+};
+
 static struct shash_desc *io_hash_desc(struct dm_verity *v, struct dm_verity_io *io)
 {
 	return (struct shash_desc *)(io + 1);
@@ -424,15 +431,18 @@ static void verity_end_io(struct bio *bi
  * The root buffer is not prefetched, it is assumed that it will be cached
  * all the time.
  */
-static void verity_prefetch_io(struct dm_verity *v, struct dm_verity_io *io)
+static void verity_prefetch_io(struct work_struct *work)
 {
+	struct dm_verity_prefetch_work *pw =
+		container_of(work, struct dm_verity_prefetch_work, work);
+	struct dm_verity *v = pw->v;
 	int i;
 
 	for (i = v->levels - 2; i >= 0; i--) {
 		sector_t hash_block_start;
 		sector_t hash_block_end;
-		verity_hash_at_level(v, io->block, i, &hash_block_start, NULL);
-		verity_hash_at_level(v, io->block + io->n_blocks - 1, i, &hash_block_end, NULL);
+		verity_hash_at_level(v, pw->block, i, &hash_block_start, NULL);
+		verity_hash_at_level(v, pw->block + pw->n_blocks - 1, i, &hash_block_end, NULL);
 		if (!i) {
 			unsigned cluster = ACCESS_ONCE(dm_verity_prefetch_cluster);
 
@@ -452,6 +462,25 @@ no_prefetch_cluster:
 		dm_bufio_prefetch(v->bufio, hash_block_start,
 				  hash_block_end - hash_block_start + 1);
 	}
+
+	kfree(pw);
+}
+
+static void verity_submit_prefetch(struct dm_verity *v, struct dm_verity_io *io)
+{
+	struct dm_verity_prefetch_work *pw;
+
+	pw = kmalloc(sizeof(struct dm_verity_prefetch_work),
+		GFP_NOIO | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN);
+
+	if (!pw)
+		return;
+
+	INIT_WORK(&pw->work, verity_prefetch_io);
+	pw->v = v;
+	pw->block = io->block;
+	pw->n_blocks = io->n_blocks;
+	queue_work(v->verify_wq, &pw->work);
 }
 
 /*
@@ -498,7 +527,7 @@ static int verity_map(struct dm_target *
 	memcpy(io->io_vec, bio_iovec(bio),
 	       io->io_vec_size * sizeof(struct bio_vec));
 
-	verity_prefetch_io(v, io);
+	verity_submit_prefetch(v, io);
 
 	generic_make_request(bio);
 
Index: linux-3.8-fast/drivers/md/dm-bufio.c
===================================================================
--- linux-3.8-fast.orig/drivers/md/dm-bufio.c	2013-03-04 23:03:14.000000000 +0100
+++ linux-3.8-fast/drivers/md/dm-bufio.c	2013-03-04 23:04:19.000000000 +0100
@@ -1026,6 +1026,8 @@ void dm_bufio_prefetch(struct dm_bufio_c
 {
 	struct blk_plug plug;
 
+	BUG_ON(dm_bufio_in_request());
+
 	blk_start_plug(&plug);
 	dm_bufio_lock(c);