Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755550Ab3CLOqm (ORCPT ); Tue, 12 Mar 2013 10:46:42 -0400 Received: from mail-bk0-f41.google.com ([209.85.214.41]:50923 "EHLO mail-bk0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755462Ab3CLOqj (ORCPT ); Tue, 12 Mar 2013 10:46:39 -0400 MIME-Version: 1.0 In-Reply-To: References: <1362415549-18653-1-git-send-email-taysom@chromium.org> <20130304174254.GF8913@agk.fab.redhat.com> Date: Tue, 12 Mar 2013 07:46:37 -0700 Message-ID: Subject: Re: [PATCH] md: dm-verity: Fix to avoid a deadlock in dm-bufio From: Paul Taysom To: Mikulas Patocka Cc: Paul Taysom , agk@redhat.com, dm-devel@redhat.com, neilb@suse.de, linux-raid@vger.kernel.org, linux-kernel@vger.kernel.org, Mandeep Singh Baines , Olof Johansson Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6386 Lines: 169 Looks good to me. On Tue, Mar 5, 2013 at 3:30 PM, Paul Taysom wrote: > seconds_kernel_to_login: 8.17s > Passed all my other tests > > On Mon, Mar 4, 2013 at 2:15 PM, Mikulas Patocka wrote: >> >> >> On Mon, 4 Mar 2013, Paul Taysom wrote: >> >>> On Mon, Mar 4, 2013 at 9:42 AM, Alasdair G Kergon 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 >> Signed-off-by: Mikulas Patocka >> 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); >> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/