Re: [PATCHv4 2/5] block: move elevator tags into struct elevator_resources

[Ming Lei <ming.lei@redhat.com>]

On Mon, Nov 10, 2025 at 01:44:49PM +0530, Nilay Shroff wrote:
> This patch introduces a new structure, struct elevator_resources, to
> group together all elevator-related resources that share the same
> lifetime. As a first step, this change moves the elevator tag pointer
> from struct elv_change_ctx into the new struct elevator_resources.
> 
> Additionally, rename blk_mq_alloc_sched_tags_batch() and
> blk_mq_free_sched_tags_batch() to blk_mq_alloc_sched_res_batch() and
> blk_mq_free_sched_res_batch(), respectively. Introduce two new wrapper
> helpers, blk_mq_alloc_sched_res() and blk_mq_free_sched_res(), around
> blk_mq_alloc_sched_tags() and blk_mq_free_sched_tags().
> 
> These changes pave the way for consolidating the allocation and freeing
> of elevator-specific resources into common helper functions. This
> refactoring improves encapsulation and prepares the code for future
> extensions, allowing additional elevator-specific data to be added to
> struct elevator_resources without cluttering struct elv_change_ctx.
> 
> Subsequent patches will extend struct elevator_resources to include
> other elevator-related data.
> 
> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
> ---
>  block/blk-mq-sched.c | 55 ++++++++++++++++++++++++++++----------------
>  block/blk-mq-sched.h | 10 +++++---
>  block/blk-mq.c       |  2 +-
>  block/elevator.c     | 31 +++++++++++++------------
>  block/elevator.h     |  9 ++++++--
>  5 files changed, 66 insertions(+), 41 deletions(-)
> 
> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> index 3d9386555a50..c7091ea4dccd 100644
> --- a/block/blk-mq-sched.c
> +++ b/block/blk-mq-sched.c
> @@ -427,7 +427,16 @@ void blk_mq_free_sched_tags(struct elevator_tags *et,
>  	kfree(et);
>  }
>  
> -void blk_mq_free_sched_tags_batch(struct xarray *elv_tbl,
> +void blk_mq_free_sched_res(struct elevator_resources *res,
> +		struct blk_mq_tag_set *set)
> +{
> +	if (res->et) {
> +		blk_mq_free_sched_tags(res->et, set);
> +		res->et = NULL;
> +	}
> +}
> +
> +void blk_mq_free_sched_res_batch(struct xarray *elv_tbl,
>  		struct blk_mq_tag_set *set)
>  {
>  	struct request_queue *q;
> @@ -445,12 +454,11 @@ void blk_mq_free_sched_tags_batch(struct xarray *elv_tbl,
>  		 */
>  		if (q->elevator) {
>  			ctx = xa_load(elv_tbl, q->id);
> -			if (!ctx || !ctx->et) {
> +			if (!ctx) {
>  				WARN_ON_ONCE(1);
>  				continue;
>  			}
> -			blk_mq_free_sched_tags(ctx->et, set);
> -			ctx->et = NULL;
> +			blk_mq_free_sched_res(&ctx->res, set);
>  		}
>  	}
>  }
> @@ -532,12 +540,22 @@ struct elevator_tags *blk_mq_alloc_sched_tags(struct blk_mq_tag_set *set,
>  	return NULL;
>  }
>  
> -int blk_mq_alloc_sched_tags_batch(struct xarray *elv_tbl,
> +int blk_mq_alloc_sched_res(struct elevator_resources *res,
> +		struct blk_mq_tag_set *set, unsigned int nr_hw_queues)

In patch 4, `struct request_queue *` is added to parameter of
blk_mq_alloc_sched_res(), so why not add it from beginning?
Then ` struct blk_mq_tag_set *set` can be avoided.

Similar with blk_mq_free_sched_res().

This way is more readable, because scheduler is request_queue
wide.

> +{
> +	res->et = blk_mq_alloc_sched_tags(set, nr_hw_queues,
> +			blk_mq_default_nr_requests(set));
> +	if (!res->et)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +int blk_mq_alloc_sched_res_batch(struct xarray *elv_tbl,
>  		struct blk_mq_tag_set *set, unsigned int nr_hw_queues)
>  {
>  	struct elv_change_ctx *ctx;
>  	struct request_queue *q;
> -	struct elevator_tags *et;
>  	int ret = -ENOMEM;
>  
>  	lockdep_assert_held_write(&set->update_nr_hwq_lock);
> @@ -557,11 +575,10 @@ int blk_mq_alloc_sched_tags_batch(struct xarray *elv_tbl,
>  				goto out_unwind;
>  			}
>  
> -			ctx->et = blk_mq_alloc_sched_tags(set, nr_hw_queues,
> -					blk_mq_default_nr_requests(set));
> -			if (!ctx->et)
> +			ret = blk_mq_alloc_sched_res(&ctx->res, set,
> +					nr_hw_queues);
> +			if (ret)
>  				goto out_unwind;
> -
>  		}
>  	}
>  	return 0;
> @@ -569,10 +586,8 @@ int blk_mq_alloc_sched_tags_batch(struct xarray *elv_tbl,
>  	list_for_each_entry_continue_reverse(q, &set->tag_list, tag_set_list) {
>  		if (q->elevator) {
>  			ctx = xa_load(elv_tbl, q->id);
> -			if (ctx && ctx->et) {
> -				blk_mq_free_sched_tags(ctx->et, set);
> -				ctx->et = NULL;
> -			}
> +			if (ctx)
> +				blk_mq_free_sched_res(&ctx->res, set);
>  		}
>  	}
>  	return ret;
> @@ -580,7 +595,7 @@ int blk_mq_alloc_sched_tags_batch(struct xarray *elv_tbl,
>  
>  /* caller must have a reference to @e, will grab another one if successful */
>  int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e,
> -		struct elevator_tags *et)
> +		struct elevator_resources *res)
>  {
>  	unsigned int flags = q->tag_set->flags;
>  	struct blk_mq_hw_ctx *hctx;
> @@ -588,23 +603,23 @@ int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e,
>  	unsigned long i;
>  	int ret;
>  
> -	eq = elevator_alloc(q, e, et);
> +	eq = elevator_alloc(q, e, res->et);
>  	if (!eq)
>  		return -ENOMEM;
>  
> -	q->nr_requests = et->nr_requests;
> +	q->nr_requests = res->et->nr_requests;
>  
>  	if (blk_mq_is_shared_tags(flags)) {
>  		/* Shared tags are stored at index 0 in @et->tags. */
> -		q->sched_shared_tags = et->tags[0];
> -		blk_mq_tag_update_sched_shared_tags(q, et->nr_requests);
> +		q->sched_shared_tags = res->et->tags[0];
> +		blk_mq_tag_update_sched_shared_tags(q, res->et->nr_requests);
>  	}
>  
>  	queue_for_each_hw_ctx(q, hctx, i) {
>  		if (blk_mq_is_shared_tags(flags))
>  			hctx->sched_tags = q->sched_shared_tags;
>  		else
> -			hctx->sched_tags = et->tags[i];
> +			hctx->sched_tags = res->et->tags[i];
>  	}

Adding one local variable of 'et' could kill all above changes, looks you like
big patch, but up to you.


Thanks, 
Ming