On Fri, Aug 20, 2021 at 11:04:32AM -0700, Bart Van Assche wrote:
> 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;
Hello Bart,
Thank you for your patch!
I tested it, and it does solve my problem.
I've been thinking more about this problem.
The problem is seen on a SATA zoned drive.
These drives have mq-deadline set as default by the
blk_queue_required_elevator_features(q, ELEVATOR_F_ZBD_SEQ_WRITE) call in
drivers/scsi/sd_zbc.c:sd_zbc_read_zones()
This triggers block/elevator.c:elevator_init_mq() to initialize
"mq-deadline" as default scheduler for these devices.
I think that the problem might because that drivers/scsi/sd_zbc.c
has created the request_queue and submitted requests, before the call
to elevator_init_mq() is done.
elevator_init_mq() will set q->elevator->type->ops, so once that is set,
blk_mq_free_request() will call e->type->ops.finish_request(rq),
regardless if the request was inserted through the recently initialized
scheduler or not.
While I'm perfectly happy with your fix, would it perhaps be possible
to do the fix in block/elevator.c instead, so that we don't need to
do the same type of check that you did, in each and every single
io scheduler?
Looking at block/elevator.c:elevator_init_mq(), it seems to do:
blk_mq_freeze_queue()
blk_mq_quiesce_queue()
blk_mq_init_sched()
blk_mq_unquiesce_queue()
blk_mq_unfreeze_queue()
This obviously isn't enough to avoid the bug that we are seeing,
but could perhaps a more general fix be to flush/wait until all
in-flight requests have completed, and then free them, and then
set q->elevator->type->ops. That way, all requests inserted after
the io scheduler has been initialized, will have gone through the
io scheduler. So all finish_request() calls should have a
matching insert_request() call. What do you think?
Kind regards,
Niklas
