On 11/06/12 21:35, Jens Axboe wrote:
> On 2012-11-06 13:14, Jun'ichi Nomura wrote:
>> (Cc to dm-devel)
>>
>> On 11/06/12 20:29, Jens Axboe wrote:
>>> I recently fixed a deadlock for a customer we have with
>>> the below patch. I have queued it up in my tree as not
>>> to lose it. Can I have an ack from you, or do you want
>>> to submit it yourself? I've marked it stable as well.
>>
>> Could you tell details about how the deadlock happened?
>>
>> dm's end_io always throws the completion handling to softirq:
>> end_clone_request()
>> dm_complete_request()
>> blk_complete_request()
>>
>> then it is processed in softirq context:
>> dm_softirq_done()
>> dm_done()
>> dm_end_request()
>> rq_completed(run_queue=true)
>> blk_run_queue()
>>
>> Since queue_lock is always held with interrupt disabled,
>> I couldn't see why it could deadlock.
>>
>> Request-based dm followed the completion model of scsi
>> mid layer. So similar code path exists in scsi, too.
>> For example:
>> scsi_request_fn()
>> scsi_kill_request()
>> blk_complete_request()
>> then:
>> scsi_softirq_done()
>> scsi_io_completion()
>> scsi_next_command()
>> scsi_run_queue()
>> spin_lock queue_lock
>> __blk_run_queue()
>>
>> If calling run-queue from softirq_done_fn() can cause deadlock,
>> I'm afraid the problem is not limited to dm.
>
> dm_softirq_done()
> rq_completed()
> blk_run_queue()
^^ this is for dm's queue
> dm_request_fn()
> dm_dispatch_request()
> blk_insert_cloned_request()
> __elv_add_request()
> elv_insert()
> blk_run_queue()
^^ this is for lower device's queue
> ...
>
> Basically you recurse back into the request handler, since it ends up
> running the queue.
But the queues are different as commented inline above.
So it should be ok. (from deadlock point of view)
> But I see I've been too focused on the older release,
> since we don't actually do that anymore after the plugging rewrite. So
> it should actually be safe in current kernels and hence the patch only
> needed for the stable series where that is not the case.
BTW, using blk_run_queue_async() in softirq_done_fn() might be good
from performance point of view? It may reduce latency of the softirq
and have an effect of batching request_fn calls.
--
Jun'ichi Nomura, NEC Corporation
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
