Re: [PATCH v2 4/5] blk-mq-sched: refactor __blk_mq_do_dispatch_sched()

[Bart Van Assche <bvanassche@acm.org>]

On 7/30/25 1:22 AM, Yu Kuai wrote:
> Introduce struct sched_dispatch_ctx, and split the helper into
> elevator_dispatch_one_request() and elevator_finish_dispatch(). Also
> and comments about the non-error return value.

and -> add

> +struct sched_dispatch_ctx {
> +	struct blk_mq_hw_ctx *hctx;
> +	struct elevator_queue *e;
> +	struct request_queue *q;

'e' is always equal to q->elevator so I'm not sure whether it's worth to
have the member 'e'?

> +static bool elevator_can_dispatch(struct sched_dispatch_ctx *ctx)
> +{
> +	if (ctx->e->type->ops.has_work &&
> +	    !ctx->e->type->ops.has_work(ctx->hctx))
> +		return false;
>   
> -		if (!list_empty_careful(&hctx->dispatch)) {
> -			busy = true;
> -			break;
> -		}
> +	if (!list_empty_careful(&ctx->hctx->dispatch)) {
> +		ctx->busy = true;
> +		return false;
> +	}
>   
> -		budget_token = blk_mq_get_dispatch_budget(q);
> -		if (budget_token < 0)
> -			break;
> +	return true;
> +}

Shouldn't all function names in this file start with the blk_mq_ prefix?

Additionally, please rename elevator_can_dispatch() into
elevator_should_dispatch(). I think the latter name better reflects the
purpose of this function.

> +	if (sq_sched)
> +		spin_lock_irq(&ctx->e->lock);
> +	rq = ctx->e->type->ops.dispatch_request(ctx->hctx);
> +	if (sq_sched)
> +		spin_unlock_irq(&ctx->e->lock);

Same comment here as on patch 1/5: code like the above makes it
harder than necessary for static analyzers to verify this code.

>   
> +	if (!rq) {
> +		blk_mq_put_dispatch_budget(ctx->q, budget_token);
>   		/*
> -		 * If we cannot get tag for the request, stop dequeueing
> -		 * requests from the IO scheduler. We are unlikely to be able
> -		 * to submit them anyway and it creates false impression for
> -		 * scheduling heuristics that the device can take more IO.
> +		 * We're releasing without dispatching. Holding the
> +		 * budget could have blocked any "hctx"s with the
> +		 * same queue and if we didn't dispatch then there's
> +		 * no guarantee anyone will kick the queue.  Kick it
> +		 * ourselves.
>   		 */

Please keep the original comment. To me the new comment seems less clear
than the existing comment.

> +static int __blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
> +{
> +	unsigned int max_dispatch;
> +	struct sched_dispatch_ctx ctx = {
> +		.hctx	= hctx,
> +		.q	= hctx->queue,
> +		.e	= hctx->queue->elevator,
> +	};
> +
> +	INIT_LIST_HEAD(&ctx.rq_list);

Please remove the INIT_LIST_HEAD() invocation and add the following in
the ctx declaration:

	.rq_list = LIST_HEAD_INIT(ctx.rq_list),

This is a common pattern in kernel code. The following grep command
yields about 200 results:

$ git grep -nH '= LIST_HEAD_INIT.*\.'

Otherwise this patch looks good to me.

Thanks,

Bart.