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

From: Paul Taysom
Date: Tue Mar 12 2013 - 10:46:47 EST

Looks good to me.

On Tue, Mar 5, 2013 at 3:30 PM, Paul Taysom <taysom@xxxxxxxxxx> wrote:
> seconds_kernel_to_login: 8.17s
> Passed all my other tests
>
> On Mon, Mar 4, 2013 at 2:15 PM, Mikulas Patocka <mpatocka@xxxxxxxxxx> wrote:
>>
>>
>> On Mon, 4 Mar 2013, Paul Taysom wrote:
>>
>>> On Mon, Mar 4, 2013 at 9:42 AM, Alasdair G Kergon <agk@xxxxxxxxxx> 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@xxxxxxxxxxxx>
>> Signed-off-by: Mikulas Patocka <mpatocka@xxxxxxxxxx>
>> Cc: stable@xxxxxxxxxx
>> ---
>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/