From: Niklas Cassel <Niklas.Cassel@wdc.com>
To: Bart Van Assche <firstname.lastname@example.org>
Cc: Jens Axboe <email@example.com>,
Christoph Hellwig <firstname.lastname@example.org>, Jaegeuk Kim <email@example.com>,
Damien Le Moal <Damien.LeMoal@wdc.com>,
Hannes Reinecke <firstname.lastname@example.org>
Subject: Re: [PATCH] mq-deadline: Fix request accounting
Date: Fri, 27 Aug 2021 09:20:30 +0000 [thread overview]
Message-ID: <YSiuXNolj2STI1DP@x1-carbon> (raw)
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 <email@example.com>
> > Tested-by: Niklas Cassel <firstname.lastname@example.org>
> 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
Looking more closely on how it is solved in BFQ:
they have both .finish_request and .requeue_request pointing to the same
bfq_finish_requeue_request() also sets rq->elv.priv = NULL; at the end
of the function.
Also see the big note about how this really should be solved in blk-mq:
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.
prev parent reply other threads:[~2021-08-27 9:20 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-24 17:05 [PATCH] mq-deadline: Fix request accounting Bart Van Assche
2021-08-24 17:09 ` Jens Axboe
2021-08-24 17:13 ` Bart Van Assche
2021-08-24 17:18 ` Jens Axboe
2021-08-24 21:35 ` Niklas Cassel
2021-08-27 2:22 ` Bart Van Assche
2021-08-27 9:20 ` Niklas Cassel [this message]
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).