Re: [PATCH 05/10] block: remove per-queue plugging
From: NeilBrown <hidden>
Date: 2011-04-11 11:55:36
Also in:
dm-devel, lkml
On Mon, 11 Apr 2011 20:59:28 +1000 NeilBrown [off-list ref] wrote:
On Mon, 11 Apr 2011 11:19:58 +0200 Jens Axboe [off-list ref] wrote:quoted
On 2011-04-11 06:50, NeilBrown wrote:quoted
quoted
The only explanation I can come up with is that very occasionally schedule on 2 separate cpus calls blk_flush_plug for the same task. I don't understand the scheduler nearly well enough to know if or how that can happen. However with this patch in place I can write to a RAID1 constantly for half an hour, and without it, the write rarely lasts for 3 minutes.Or perhaps if the request_fn blocks, that would be problematic. So the patch is likely a good idea even for that case. I'll merge it, changing it to list_splice_init() as I think that would be more clear.OK - though I'm not 100% the patch fixes the problem - just that it hides the symptom for me. I might try instrumenting the code a bit more and see if I can find exactly where it is re-entering flush_plug_list - as that seems to be what is happening.
OK, I found how it re-enters.
The request_fn doesn't exactly block, but when scsi_request_fn calls
spin_unlock_irq, this calls preempt_enable which can call schedule, which is
a recursive call.
The patch I provided will stop that from recursing again as the blk_plug.list
will be empty.
So it is almost what you suggested, however the request_fn doesn't block, it
just enabled preempt.
So the comment I would put at the top of that patch would be something like:
From: NeilBrown <redacted>
As request_fn called by __blk_run_queue is allowed to 'schedule()' (after
dropping the queue lock of course), it is possible to get a recursive call:
schedule -> blk_flush_plug -> __blk_finish_plug -> flush_plug_list
-> __blk_run_queue -> request_fn -> schedule
We must make sure that the second schedule does not call into blk_flush_plug
again. So instead of leaving the list of requests on blk_plug->list, move
them to a separate list leaving blk_plug->list empty.
Signed-off-by: NeilBrown <redacted>
Thanks,
NeilBrown