| Message ID | 20230711160434.248868-1-ross.lagerwall@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-mq: Fix stall due to recursive flush plug | expand |
On Tue, Jul 11, 2023 at 05:04:34PM +0100, Ross Lagerwall wrote: > We have seen rare IO stalls as follows: > > * blk_mq_plug_issue_direct() is entered with an mq_list containing two > requests. > * For the first request, it sets last == false and enters the driver's > queue_rq callback. > * The driver queue_rq callback indirectly calls schedule() which calls > blk_flush_plug(). -> this assumes BLK_MQ_F_BLOCKING is set, as otherwise ->queue_rq can't sleep. > * blk_flush_plug() handles the remaining request in the mq_list. mq_list > is now empty. > * The original call to queue_rq resumes (with last == false). > * The loop in blk_mq_plug_issue_direct() terminates because there are no > remaining requests in mq_list. > > The IO is now stalled because the last request submitted to the driver > had last == false and there was no subsequent call to commit_rqs(). > > Fix this by returning early in blk_mq_flush_plug_list() if rq_count is 0 > which it will be in the recursive case, rather than checking if the > mq_list is empty. At the same time, adjust one of the callers to skip > the mq_list empty check as it is not necessary. From what I can tell this looks correct, but at the same time very fragile. At least we need a comment on learing plug->rq_count early in blk_mq_flush_plug_list about this recursion potential, probably paired with another one where we're checking rq_count instead of the list now. I wonder if there is a better way to do this in a more explicit way, but I can't think of one right now.
diff --git a/block/blk-core.c b/block/blk-core.c index 99d8b9812b18..90de50082146 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -1144,8 +1144,7 @@ void __blk_flush_plug(struct blk_plug *plug, bool from_schedule) { if (!list_empty(&plug->cb_list)) flush_plug_callbacks(plug, from_schedule); - if (!rq_list_empty(plug->mq_list)) - blk_mq_flush_plug_list(plug, from_schedule); + blk_mq_flush_plug_list(plug, from_schedule); /* * Unconditionally flush out cached requests, even if the unplug * event came from schedule. Since we know hold references to the diff --git a/block/blk-mq.c b/block/blk-mq.c index 5504719b970d..d5a7f36e634f 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -2742,7 +2742,7 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule) { struct request *rq; - if (rq_list_empty(plug->mq_list)) + if (plug->rq_count == 0) return; plug->rq_count = 0;
We have seen rare IO stalls as follows: * blk_mq_plug_issue_direct() is entered with an mq_list containing two requests. * For the first request, it sets last == false and enters the driver's queue_rq callback. * The driver queue_rq callback indirectly calls schedule() which calls blk_flush_plug(). * blk_flush_plug() handles the remaining request in the mq_list. mq_list is now empty. * The original call to queue_rq resumes (with last == false). * The loop in blk_mq_plug_issue_direct() terminates because there are no remaining requests in mq_list. The IO is now stalled because the last request submitted to the driver had last == false and there was no subsequent call to commit_rqs(). Fix this by returning early in blk_mq_flush_plug_list() if rq_count is 0 which it will be in the recursive case, rather than checking if the mq_list is empty. At the same time, adjust one of the callers to skip the mq_list empty check as it is not necessary. Fixes: dc5fc361d891 ("block: attempt direct issue of plug list") Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> --- I'm not 100% sure if I've got the correct Fixes commit. block/blk-core.c | 3 +-- block/blk-mq.c | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-)