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.
