Re: [PATCH] mq-deadline: Fix request accounting

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Thu, Aug 26, 2021 at 07:22:38PM -0700, Bart Van Assche wrote:
> On 8/24/21 2:35 PM, Niklas Cassel wrote:
> > Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
> > Tested-by: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Hi Niklas,
> 
> Thank you for the help, that's really appreciated. Earlier today I discovered
> that this patch does not handle requeuing correctly so I have started testing
> the patch below.

Since Jens has reverted "block/mq-deadline: Prioritize high-priority requests",
which was the culprit for the 10 second slowdowns in the first case, there is
no longer any urgency for you to fix the counters to behave properly on
requeues, since without that patch, the counters are not used for decision
making anyway.

Looking more closely on how it is solved in BFQ:
they have both .finish_request and .requeue_request pointing to the same
function: bfq_finish_requeue_request()

bfq_finish_requeue_request() also sets rq->elv.priv[0] = NULL; at the end
of the function.

Also see the big note about how this really should be solved in blk-mq:
https://github.com/torvalds/linux/blob/master/block/bfq-iosched.c#L6456-L6462

Is seems wrong to clutter multiple callback functions in both BFQ and
mq-deadline because of shortcomings in blk-mq.


It seems like the way forward is to add a RQF_ELEVATOR request flag, which
only gets set in req->rq_flags if e->type->ops.insert_requests() gets called
(by blk_mq_sched_insert_request() or blk_mq_sched_insert_requests()).
Then blk_mq_free_request() only calls ops.finish_request if RQF_ELEVATOR is
set, and similarly blk_mq_sched_requeue_request() only calls
ops.requeue_request if RQF_ELEVATOR is set (and perhaps also
blk_mq_sched_completed_request() for ops.completed_request).

This should enable us to remove ugly if-statements from .insert_requests and
.finish_request for both mq-deadline and BFQ. (And BFQ can also remove ugly
if-statements and comments from their .requeue_request callback.)


And no, we cannot reuse RQF_ELVPRIV, since that flag gets set by
__blk_mq_alloc_request(), which is at a way too early stage, since at that
point, we don't know if the request will bypass the scheduler or not,
the flag has to get set at insertion time.
(Since right now, a request that has RQF_ELVPRIV, and have thus been prepared
by ops.prepare_request, might still not get inserted into the scheduler.)

We should probably just remove RQF_ELVPRIV, and as a first step, simply have
blk_mq_sched_insert_request() and blk_mq_sched_insert_requests() call
ops.prepare_request immediately before ops.insert_requests. Because currently,
it seems that all elevators only use .prepare_request to clear private fields,
no elevator has any real logic in their .prepare_request callback.


Kind regards,
Niklas