Re: [PATCH V3 13/20] block: unifying elevator change

[Christoph Hellwig <hch@lst.de>]

On Thu, Apr 24, 2025 at 11:21:36PM +0800, Ming Lei wrote:
> elevator change is one well-define behavior:

Start the sentence captitalized.

> +static bool use_default_elevator(struct request_queue *q)
>  {
>  	if (q->tag_set->flags & BLK_MQ_F_NO_SCHED_BY_DEFAULT)
> +		return false;
>  
>  	if (q->nr_hw_queues != 1 &&
>  	    !blk_mq_is_shared_tags(q->tag_set->flags))
> +		return false;
>  
> +	return true;

looking at the only caller I think this is better open coded there.

> @@ -694,7 +651,7 @@ static int __elevator_change(struct request_queue *q,
>  	lockdep_assert_held(&q->tag_set->update_nr_hwq_sema);
>  
>  	/* Make sure queue is not in the middle of being removed */
> -	if (!blk_queue_registered(q))
> +	if (!ctx->init && !blk_queue_registered(q))

This init flag is confusingly also set by the teardown, so it is quite
misnamed.  But given that none of the locks held here protected the
queue registered flag, why not just move the check out to
elv_iosched_store and remove the entire need for the flag?

> +/*
> + * Use the default elevator settings. If the chosen elevator initialization
> + * fails, fall back to the "none" elevator (no elevator).
> + */
> +void elevator_set_default(struct request_queue *q)
> +{
> +	struct elv_change_ctx ctx = {
> +		.init = true,
> +	};
> +	int err;
> +
> +	if (!queue_is_mq(q))
> +		return;

Please keep this in the caller.  Same for the set_none side.

> +
> +	ctx.name = use_default_elevator(q) ? "mq-deadline" : "none";
> +	if (!q->elevator && !strcmp(ctx.name, "none"))
> +		return;
> +	err = elevator_change(q, &ctx);

q->elevator can't be set here.  And the extra none strcmp is weird
because we have the boolean information avaiable.

Just make this:

	struct elv_change_ctx ctx = {
		.name = "mq-deadline",
	};

	...

 	if (!(q->tag_set->flags & BLK_MQ_F_NO_SCHED_BY_DEFAULT) &&
	    (q->nr_hw_queues == 1 ||
	     blk_mq_is_shared_tags(q->tag_set->flags))) {
		err = elevator_change(q, &ctx);

> +	if (err < 0)
> +		pr_warn("\"%s\" set elevator failed %d, "
> +			"falling back to \"none\"\n", ctx.name, err);

Please keep the more useful message from the current code.