On 8/19/21 5:45 PM, Niklas Cassel wrote:
> dd_queued() calls dd_sum() which has this comment:
>
> /*
> * Returns the total number of dd_count(dd, event_type, prio) calls across all
> * CPUs. No locking or barriers since it is fine if the returned sum is slightly
> * outdated.
> */
>
> Perhaps not so got to use an accounting that is not accurate to determine
> if we should process IOs belonging to a certain priority class or not.
>
> Perhaps we could use e.g. atomics instead of per cpu counters without
> locking?
First of all, thanks for the detailed report.
Using atomics is an option but an option we should only choose if there are no
better options since every atomic operation in the hot path has a measurable
negative performance impact.
> kworker/u64:11-628 [026] .... 13.650123: dd_finish_request: dd prio: 1 prio class: 0
> kworker/u64:11-628 [026] .... 13.650125: dd_queued_print: ins: 0 comp: 1 queued: 4294967295
4294967295 is the unsigned representation of -1. This indicates a bug - the
"queued" number should never be negative.
> What appears to be happening here is that dd_finish_request() gets called a bunch of times,
> without any preceeding dd_insert_requests() call.
>
> Reading the comment above dd_finish_request():
>
> * Callback from inside blk_mq_free_request().
>
> Could it be that this callback is done on certain requests that was never
> sent down to mq-deadline?
> Perhaps blk_mq_request_bypass_insert() or blk_mq_try_issue_directly() was
> called, and therefore dd_insert_requests() was never called for some of the
> ealiest requests in the system, but since e->type->ops.finish_request() is
> set, dd_finish_request() gets called on free anyway.
>
> Since dd_queued() is defined as:
> return dd_sum(dd, inserted, prio) - dd_sum(dd, completed, prio);
> And since we can see that we have several calls to dd_finish_request()
> that has increased the completed counter, dd_queued() returns a
> very high value, since 0 - 19 = 4294967277.
>
> This is probably the bug that causes the bogus accouting of BE reqs.
> However, looking at the comment for dd_sum(), it also doesn't feel good
> to rely on something that is "slightly outdated" to determine if we
> should process a whole io class or not.
> Letting requests wait for 10 seconds when there are no other outstanding
> requests in the scheduler doesn't seem like the right thing to do.
The "slightly outdated" in that comment is not what causes the I/O delays -
these are caused by updating statistics in dd_finish_request() for requests
that have not been seen by dd_insert_requests(). Please note that
dd_insert_request() and dd_dispatch_request() access the I/O statistics
while dd->lock is held. Only dd_finish_request() updates the I/O statistics
without holding dd->lock. So the dd_queued() call from inside
dd_dispatch_request() can return a number that is too big but not a number
that is too small. Hence, I don't think that updating the I/O statistics
without locking in the deadline scheduler can cause an I/O delay.
Does the patch below help?
Thanks,
Bart.
Subject: [PATCH] mq-deadline: Fix request accounting
The block layer may call the I/O scheduler .finish_request() callback
without having called the .insert_requests() callback. Make sure that the
mq-deadline I/O statistics are correct if the block layer inserts an I/O
request that bypasses the I/O scheduler. This patch prevents that lower
priority I/O is delayed longer than necessary for mixed I/O priority
workloads.
Fixes: 08a9ad8bf607 ("block/mq-deadline: Add cgroup support")
Reported-by: Niklas Cassel <Niklas.Cassel@xxxxxxx>
Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
---
block/mq-deadline-main.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/block/mq-deadline-main.c b/block/mq-deadline-main.c
index 294be0c0db65..933be9c82ec4 100644
--- a/block/mq-deadline-main.c
+++ b/block/mq-deadline-main.c
@@ -743,6 +743,7 @@ static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
blkcg = dd_blkcg_from_bio(rq->bio);
ddcg_count(blkcg, inserted, ioprio_class);
rq->elv.priv[0] = blkcg;
+ rq->elv.priv[1] = (void *)(uintptr_t)1;
if (blk_mq_sched_try_insert_merge(q, rq, &free)) {
blk_mq_free_requests(&free);
@@ -795,6 +796,7 @@ static void dd_insert_requests(struct blk_mq_hw_ctx *hctx,
static void dd_prepare_request(struct request *rq)
{
rq->elv.priv[0] = NULL;
+ rq->elv.priv[1] = NULL;
}
/*
@@ -822,8 +824,16 @@ static void dd_finish_request(struct request *rq)
const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
struct dd_per_prio *per_prio = &dd->per_prio[prio];
- dd_count(dd, completed, prio);
- ddcg_count(blkcg, completed, ioprio_class);
+ /*
+ * The block layer core may call dd_finish_request() without having
+ * called dd_insert_requests(). Hence only update statistics for
+ * requests for which dd_insert_requests() has been called. See also
+ * blk_mq_request_bypass_insert().
+ */
+ if (rq->elv.priv[1]) {
+ dd_count(dd, completed, prio);
+ ddcg_count(blkcg, completed, ioprio_class);
+ }
if (blk_queue_is_zoned(q)) {
unsigned long flags;
