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

[Paul Taysom]

On Tue, Mar 12, 2013 at 7:46 AM, Paul Taysom <taysom@xxxxxxxxxx> wrote:
> 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);
>>>

Signed-off-by: Paul Taysom <taysom@xxxxxxxxxxxx>
--
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/