Re: [PATCH 1/3] block: unify elevator tags and type xarrays into struct elv_change_ctx

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

On Thu, Oct 16, 2025 at 11:00:47AM +0530, Nilay Shroff wrote:
> Currently, the nr_hw_queues update path manages two disjoint xarrays —
> one for elevator tags and another for elevator type — both used during
> elevator switching. Maintaining these two parallel structures for the
> same purpose adds unnecessary complexity and potential for mismatched
> state.
> 
> This patch unifies both xarrays into a single structure, struct
> elv_change_ctx, which holds all per-queue elevator change context. A
> single xarray, named elv_tbl, now maps each queue (q->id) in a tagset
> to its corresponding elv_change_ctx entry, encapsulating the elevator
> tags, type and name references.
> 
> This unification simplifies the code, improves maintainability, and
> clarifies ownership of per-queue elevator state.
> 
> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
> ---
>  block/blk-mq-sched.c | 47 ++++++++++++++++++++++++++++++++++------
>  block/blk-mq-sched.h | 13 +++++++++++
>  block/blk-mq.c       | 51 ++++++++++++++++++++++++++------------------
>  block/blk.h          |  7 +++---
>  block/elevator.c     | 31 ++++++---------------------
>  block/elevator.h     | 15 +++++++++++++
>  6 files changed, 108 insertions(+), 56 deletions(-)
> 
> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> index d06bb137a743..1c9571136a30 100644
> --- a/block/blk-mq-sched.c
> +++ b/block/blk-mq-sched.c
> @@ -453,6 +453,33 @@ void blk_mq_free_sched_tags_batch(struct xarray *et_table,
>  	}
>  }
>  
> +int blk_mq_alloc_sched_ctx_batch(struct xarray *elv_tbl,
> +		struct blk_mq_tag_set *set)
> +{
> +	struct request_queue *q;
> +	struct elv_change_ctx *ctx;
> +
> +	lockdep_assert_held_write(&set->update_nr_hwq_lock);
> +
> +	list_for_each_entry(q, &set->tag_list, tag_set_list) {
> +		ctx = kzalloc(sizeof(struct elv_change_ctx), GFP_KERNEL);
> +		if (!ctx)
> +			goto out_unwind;
> +
> +		if (xa_insert(elv_tbl, q->id, ctx, GFP_KERNEL)) {
> +			kfree(ctx);
> +			goto out_unwind;
> +		}
> +	}
> +	return 0;
> +out_unwind:
> +	list_for_each_entry_continue_reverse(q, &set->tag_list, tag_set_list) {
> +		ctx = xa_load(elv_tbl, q->id);
> +		kfree(ctx);
> +	}

No need to unwind, you can let blk_mq_free_sched_ctx_batch cover cleanup from
callsite. Not mention you leave freed `ctx` into xarray, which is fragile.

> +	return -ENOMEM;
> +}
> +
>  struct elevator_tags *blk_mq_alloc_sched_tags(struct blk_mq_tag_set *set,
>  		unsigned int nr_hw_queues, unsigned int nr_requests)
>  {
> @@ -498,12 +525,13 @@ 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 *et_table,
> +int blk_mq_alloc_sched_tags_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;
> -	gfp_t gfp = GFP_NOIO | __GFP_ZERO | __GFP_NOWARN | __GFP_NORETRY;
> +	int ret = -ENOMEM;
>  
>  	lockdep_assert_held_write(&set->update_nr_hwq_lock);
>  
> @@ -520,8 +548,13 @@ int blk_mq_alloc_sched_tags_batch(struct xarray *et_table,
>  					blk_mq_default_nr_requests(set));
>  			if (!et)
>  				goto out_unwind;
> -			if (xa_insert(et_table, q->id, et, gfp))
> +
> +			ctx = xa_load(elv_tbl, q->id);
> +			if (WARN_ON_ONCE(!ctx)) {
> +				ret = -ENOENT;
>  				goto out_free_tags;
> +			}
> +			ctx->et = et;
>  		}
>  	}
>  	return 0;
> @@ -530,12 +563,12 @@ int blk_mq_alloc_sched_tags_batch(struct xarray *et_table,
>  out_unwind:
>  	list_for_each_entry_continue_reverse(q, &set->tag_list, tag_set_list) {
>  		if (q->elevator) {
> -			et = xa_load(et_table, q->id);
> -			if (et)
> -				blk_mq_free_sched_tags(et, set);
> +			ctx = xa_load(elv_tbl, q->id);
> +			if (ctx && ctx->et)
> +				blk_mq_free_sched_tags(ctx->et, set);

please clear ctx->et when it is freed.

>  		}
>  	}
> -	return -ENOMEM;
> +	return ret;
>  }
>  
>  /* caller must have a reference to @e, will grab another one if successful */
> diff --git a/block/blk-mq-sched.h b/block/blk-mq-sched.h
> index 8e21a6b1415d..ba67e4e2447b 100644
> --- a/block/blk-mq-sched.h
> +++ b/block/blk-mq-sched.h
> @@ -27,11 +27,24 @@ struct elevator_tags *blk_mq_alloc_sched_tags(struct blk_mq_tag_set *set,
>  		unsigned int nr_hw_queues, unsigned int nr_requests);
>  int blk_mq_alloc_sched_tags_batch(struct xarray *et_table,
>  		struct blk_mq_tag_set *set, unsigned int nr_hw_queues);
> +int blk_mq_alloc_sched_ctx_batch(struct xarray *elv_tbl,
> +		struct blk_mq_tag_set *set);
>  void blk_mq_free_sched_tags(struct elevator_tags *et,
>  		struct blk_mq_tag_set *set);
>  void blk_mq_free_sched_tags_batch(struct xarray *et_table,
>  		struct blk_mq_tag_set *set);
>  
> +static inline void blk_mq_free_sched_ctx_batch(struct xarray *elv_tbl)
> +{
> +	unsigned long i;
> +	struct elv_change_ctx *ctx;
> +
> +	xa_for_each(elv_tbl, i, ctx) {
> +		xa_erase(elv_tbl, i);
> +		kfree(ctx);
> +	}
> +}
> +

It could be more readable to move blk_mq_free_sched_ctx_batch() with
blk_mq_alloc_sched_ctx_batch() together.



Thanks, 
Ming