[PATCH V2 00/20] block: unify elevator changing and fix lockdep warning

[PATCH V2 00/20] block: unify elevator changing and fix lockdep warning

[Ming Lei]

Hello Jens,

This patchset cleans up elevator change code, and unifying it via single
helper, meantime moves kobject_add/del & debugfs register/unregister out of
queue freezing & elevator_lock. This way fixes many lockdep warnings
reported recently, especially since fs_reclaim is connected with freeze lock
manually by commit ffa1e7ada456 ("block: Make request_queue lockdep splats
show up earlier").


Thanks,

V2:
	- retry add/del disk when blk_mq_update_nr_hw_queues() is in-progress

	- swap blk_mq_add_queue_tag_set() with blk_mq_map_swqueue() in
	blk_mq_init_allocated_queue() (Nilay Shroff)

	- move ELEVATOR_FLAG_DISABLE_WBT to request queue's flags (Nilay Shoff) 

	- fix race because of delaying elevator unregister

	- define flags of `elv_change_ctx` as `bool` (Christoph)

	- improve comment and commit log (Christoph)

Ming Lei (20):
  block: move blk_mq_add_queue_tag_set() after blk_mq_map_swqueue()
  block: move ELEVATOR_FLAG_DISABLE_WBT as request queue flag
  block: don't call freeze queue in elevator_switch() and
    elevator_disable()
  block: add two helpers for registering/un-registering sched debugfs
  block: move sched debugfs register into elvevator_register_queue
  block: add & pass 'struct gendisk_data' for retrying add/del disk in
    updating nr_hw_queues
  block: prevent adding/deleting disk during updating nr_hw_queues
  block: don't allow to switch elevator if updating nr_hw_queues is
    in-progress
  block: simplify elevator rebuild for updating nr_hw_queues
  block: add helper of elevator_change()
  block: move blk_unregister_queue() & device_del() after freeze wait
  block: add `struct elv_change_ctx` for unifying elevator_change
  block: unifying elevator change
  block: pass elevator_queue to elv_register_queue & unregister_queue
  block: fail to show/store elevator sysfs attribute if elevator is
    dying
  block: move elv_register[unregister]_queue out of elevator_lock
  block: move debugfs/sysfs register out of freezing queue
  block: remove several ->elevator_lock
  block: move hctx cpuhp add/del out of queue freezing
  block: move wbt_enable_default() out of queue freezing from sched
    ->exit()

 block/bfq-iosched.c    |   6 +-
 block/blk-mq-debugfs.c |  12 +--
 block/blk-mq-sched.c   |  41 +++++---
 block/blk-mq.c         | 172 ++++++++++--------------------
 block/blk-sysfs.c      |  18 ++--
 block/blk-wbt.c        |   3 +-
 block/blk.h            |  10 +-
 block/elevator.c       | 231 +++++++++++++++++++++++++++--------------
 block/elevator.h       |  19 +++-
 block/genhd.c          | 158 +++++++++++++++++-----------
 include/linux/blk-mq.h |   5 +
 include/linux/blkdev.h |   3 +
 12 files changed, 374 insertions(+), 304 deletions(-)

-- 
2.47.0

[PATCH V2 01/20] block: move blk_mq_add_queue_tag_set() after blk_mq_map_swqueue()

[Ming Lei]

Move blk_mq_add_queue_tag_set() after blk_mq_map_swqueue(), and publish
this request queue to tagset after everything is setup.

This way is safe because BLK_MQ_F_TAG_QUEUE_SHARED isn't used by
blk_mq_map_swqueue(), and this flag is mainly checked in fast IO code
path.

Prepare for removing ->elevator_lock from blk_mq_map_swqueue() which
is supposed to be called when elevator switch isn't possible.

Reported-by: Nilay Shroff <nilay@linux.ibm.com>
Closes: https://lore.kernel.org/linux-block/567cb7ab-23d6-4cee-a915-c8cdac903ddd@linux.ibm.com/
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index e0fe12f1320f..7cda919fafba 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4561,8 +4561,8 @@ int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
 	q->nr_requests = set->queue_depth;
 
 	blk_mq_init_cpu_queues(q, set->nr_hw_queues);
-	blk_mq_add_queue_tag_set(set, q);
 	blk_mq_map_swqueue(q);
+	blk_mq_add_queue_tag_set(set, q);
 	return 0;
 
 err_hctxs:
-- 
2.47.0

Re: [PATCH V2 01/20] block: move blk_mq_add_queue_tag_set() after blk_mq_map_swqueue()

[Yu Kuai]

Hi,

在 2025/04/19 0:36, Ming Lei 写道:
> Move blk_mq_add_queue_tag_set() after blk_mq_map_swqueue(), and publish
> this request queue to tagset after everything is setup.
> 
> This way is safe because BLK_MQ_F_TAG_QUEUE_SHARED isn't used by
> blk_mq_map_swqueue(), and this flag is mainly checked in fast IO code
> path.
> 
> Prepare for removing ->elevator_lock from blk_mq_map_swqueue() which
> is supposed to be called when elevator switch isn't possible.

I think you mean *is* possible? Or to protect against switching
elevator concurrently?
> 
> Reported-by: Nilay Shroff <nilay@linux.ibm.com>
> Closes: https://lore.kernel.org/linux-block/567cb7ab-23d6-4cee-a915-c8cdac903ddd@linux.ibm.com/
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/blk-mq.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index e0fe12f1320f..7cda919fafba 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -4561,8 +4561,8 @@ int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
>   	q->nr_requests = set->queue_depth;
>   
>   	blk_mq_init_cpu_queues(q, set->nr_hw_queues);
> -	blk_mq_add_queue_tag_set(set, q);
>   	blk_mq_map_swqueue(q);
> +	blk_mq_add_queue_tag_set(set, q);
>   	return 0;
>   
>   err_hctxs:
> 

Re: [PATCH V2 01/20] block: move blk_mq_add_queue_tag_set() after blk_mq_map_swqueue()

[Nilay Shroff]

On 4/18/25 10:06 PM, Ming Lei wrote:
> Move blk_mq_add_queue_tag_set() after blk_mq_map_swqueue(), and publish
> this request queue to tagset after everything is setup.
> 
> This way is safe because BLK_MQ_F_TAG_QUEUE_SHARED isn't used by
> blk_mq_map_swqueue(), and this flag is mainly checked in fast IO code
> path.
> 
> Prepare for removing ->elevator_lock from blk_mq_map_swqueue() which
> is supposed to be called when elevator switch isn't possible.
> 
> Reported-by: Nilay Shroff <nilay@linux.ibm.com>
> Closes: https://lore.kernel.org/linux-block/567cb7ab-23d6-4cee-a915-c8cdac903ddd@linux.ibm.com/
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Looks good to me:
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

Re: [PATCH V2 01/20] block: move blk_mq_add_queue_tag_set() after blk_mq_map_swqueue()

[Christoph Hellwig]

Looks good (modulo the little commit log update):

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH V2 01/20] block: move blk_mq_add_queue_tag_set() after blk_mq_map_swqueue()

[Hannes Reinecke]

On 4/18/25 18:36, Ming Lei wrote:
> Move blk_mq_add_queue_tag_set() after blk_mq_map_swqueue(), and publish
> this request queue to tagset after everything is setup.
> 
> This way is safe because BLK_MQ_F_TAG_QUEUE_SHARED isn't used by
> blk_mq_map_swqueue(), and this flag is mainly checked in fast IO code
> path.
> 
> Prepare for removing ->elevator_lock from blk_mq_map_swqueue() which
> is supposed to be called when elevator switch isn't possible.
> 
> Reported-by: Nilay Shroff <nilay@linux.ibm.com>
> Closes: https://lore.kernel.org/linux-block/567cb7ab-23d6-4cee-a915-c8cdac903ddd@linux.ibm.com/
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/blk-mq.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index e0fe12f1320f..7cda919fafba 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -4561,8 +4561,8 @@ int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
>   	q->nr_requests = set->queue_depth;
>   
>   	blk_mq_init_cpu_queues(q, set->nr_hw_queues);
> -	blk_mq_add_queue_tag_set(set, q);
>   	blk_mq_map_swqueue(q);
> +	blk_mq_add_queue_tag_set(set, q);
>   	return 0;
>   
>   err_hctxs:

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH V2 02/20] block: move ELEVATOR_FLAG_DISABLE_WBT as request queue flag

[Ming Lei]

ELEVATOR_FLAG_DISABLE_WBT is only used by BFQ to disallow wbt when BFQ is
in use. The flag is set in BFQ's init(), and cleared in BFQ's exit().

Making it as request queue flag, so that we can avoid to deal with elevator
switch race. Also it isn't graceful to checking one scheduler flag in
wbt_enable_default().

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/bfq-iosched.c    | 4 ++--
 block/blk-mq-debugfs.c | 1 +
 block/blk-wbt.c        | 3 +--
 block/elevator.h       | 1 -
 include/linux/blkdev.h | 3 +++
 5 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index abd80dc13562..40e4106a71e7 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -7210,7 +7210,7 @@ static void bfq_exit_queue(struct elevator_queue *e)
 #endif
 
 	blk_stat_disable_accounting(bfqd->queue);
-	clear_bit(ELEVATOR_FLAG_DISABLE_WBT, &e->flags);
+	blk_queue_flag_clear(QUEUE_FLAG_DISABLE_WBT, bfqd->queue);
 	wbt_enable_default(bfqd->queue->disk);
 
 	kfree(bfqd);
@@ -7397,7 +7397,7 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e)
 	/* We dispatch from request queue wide instead of hw queue */
 	blk_queue_flag_set(QUEUE_FLAG_SQ_SCHED, q);
 
-	set_bit(ELEVATOR_FLAG_DISABLE_WBT, &eq->flags);
+	blk_queue_flag_set(QUEUE_FLAG_DISABLE_WBT, q);
 	wbt_disable_default(q->disk);
 	blk_stat_enable_accounting(q);
 
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 3421b5521fe2..31e249a18407 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -93,6 +93,7 @@ static const char *const blk_queue_flag_name[] = {
 	QUEUE_FLAG_NAME(RQ_ALLOC_TIME),
 	QUEUE_FLAG_NAME(HCTX_ACTIVE),
 	QUEUE_FLAG_NAME(SQ_SCHED),
+	QUEUE_FLAG_NAME(DISABLE_WBT),
 };
 #undef QUEUE_FLAG_NAME
 
diff --git a/block/blk-wbt.c b/block/blk-wbt.c
index f1754d07f7e0..29cd2e33666f 100644
--- a/block/blk-wbt.c
+++ b/block/blk-wbt.c
@@ -704,8 +704,7 @@ void wbt_enable_default(struct gendisk *disk)
 	struct rq_qos *rqos;
 	bool enable = IS_ENABLED(CONFIG_BLK_WBT_MQ);
 
-	if (q->elevator &&
-	    test_bit(ELEVATOR_FLAG_DISABLE_WBT, &q->elevator->flags))
+	if (blk_queue_disable_wbt(q))
 		enable = false;
 
 	/* Throttling already enabled? */
diff --git a/block/elevator.h b/block/elevator.h
index e4e44dfac503..e27af5492cdb 100644
--- a/block/elevator.h
+++ b/block/elevator.h
@@ -121,7 +121,6 @@ struct elevator_queue
 };
 
 #define ELEVATOR_FLAG_REGISTERED	0
-#define ELEVATOR_FLAG_DISABLE_WBT	1
 
 /*
  * block elevator interface
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index e39c45bc0a97..10410d9b03ad 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -644,6 +644,7 @@ enum {
 	QUEUE_FLAG_RQ_ALLOC_TIME,	/* record rq->alloc_time_ns */
 	QUEUE_FLAG_HCTX_ACTIVE,		/* at least one blk-mq hctx is active */
 	QUEUE_FLAG_SQ_SCHED,		/* single queue style io dispatch */
+	QUEUE_FLAG_DISABLE_WBT,		/* for sched to disable/enable wbt */
 	QUEUE_FLAG_MAX
 };
 
@@ -679,6 +680,8 @@ void blk_queue_flag_clear(unsigned int flag, struct request_queue *q);
 #define blk_queue_sq_sched(q)	test_bit(QUEUE_FLAG_SQ_SCHED, &(q)->queue_flags)
 #define blk_queue_skip_tagset_quiesce(q) \
 	((q)->limits.features & BLK_FEAT_SKIP_TAGSET_QUIESCE)
+#define blk_queue_disable_wbt(q)	\
+	test_bit(QUEUE_FLAG_DISABLE_WBT, &(q)->queue_flags)
 
 extern void blk_set_pm_only(struct request_queue *q);
 extern void blk_clear_pm_only(struct request_queue *q);
-- 
2.47.0

Re: [PATCH V2 02/20] block: move ELEVATOR_FLAG_DISABLE_WBT as request queue flag

[Yu Kuai]

Hi,

在 2025/04/19 0:36, Ming Lei 写道:
> ELEVATOR_FLAG_DISABLE_WBT is only used by BFQ to disallow wbt when BFQ is
> in use. The flag is set in BFQ's init(), and cleared in BFQ's exit().
> 
> Making it as request queue flag, so that we can avoid to deal with elevator
> switch race. Also it isn't graceful to checking one scheduler flag in
> wbt_enable_default().
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/bfq-iosched.c    | 4 ++--
>   block/blk-mq-debugfs.c | 1 +
>   block/blk-wbt.c        | 3 +--
>   block/elevator.h       | 1 -
>   include/linux/blkdev.h | 3 +++
>   5 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index abd80dc13562..40e4106a71e7 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -7210,7 +7210,7 @@ static void bfq_exit_queue(struct elevator_queue *e)
>   #endif
>   
>   	blk_stat_disable_accounting(bfqd->queue);
> -	clear_bit(ELEVATOR_FLAG_DISABLE_WBT, &e->flags);
> +	blk_queue_flag_clear(QUEUE_FLAG_DISABLE_WBT, bfqd->queue);
>   	wbt_enable_default(bfqd->queue->disk);
>   
>   	kfree(bfqd);
> @@ -7397,7 +7397,7 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e)
>   	/* We dispatch from request queue wide instead of hw queue */
>   	blk_queue_flag_set(QUEUE_FLAG_SQ_SCHED, q);
>   
> -	set_bit(ELEVATOR_FLAG_DISABLE_WBT, &eq->flags);
> +	blk_queue_flag_set(QUEUE_FLAG_DISABLE_WBT, q);
>   	wbt_disable_default(q->disk);
>   	blk_stat_enable_accounting(q);
>   
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index 3421b5521fe2..31e249a18407 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -93,6 +93,7 @@ static const char *const blk_queue_flag_name[] = {
>   	QUEUE_FLAG_NAME(RQ_ALLOC_TIME),
>   	QUEUE_FLAG_NAME(HCTX_ACTIVE),
>   	QUEUE_FLAG_NAME(SQ_SCHED),
> +	QUEUE_FLAG_NAME(DISABLE_WBT),
>   };
>   #undef QUEUE_FLAG_NAME
>   
> diff --git a/block/blk-wbt.c b/block/blk-wbt.c
> index f1754d07f7e0..29cd2e33666f 100644
> --- a/block/blk-wbt.c
> +++ b/block/blk-wbt.c
> @@ -704,8 +704,7 @@ void wbt_enable_default(struct gendisk *disk)
>   	struct rq_qos *rqos;
>   	bool enable = IS_ENABLED(CONFIG_BLK_WBT_MQ);
>   
> -	if (q->elevator &&
> -	    test_bit(ELEVATOR_FLAG_DISABLE_WBT, &q->elevator->flags))
> +	if (blk_queue_disable_wbt(q))
>   		enable = false;
>   
>   	/* Throttling already enabled? */
> diff --git a/block/elevator.h b/block/elevator.h
> index e4e44dfac503..e27af5492cdb 100644
> --- a/block/elevator.h
> +++ b/block/elevator.h
> @@ -121,7 +121,6 @@ struct elevator_queue
>   };
>   
>   #define ELEVATOR_FLAG_REGISTERED	0
> -#define ELEVATOR_FLAG_DISABLE_WBT	1
>   
>   /*
>    * block elevator interface
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index e39c45bc0a97..10410d9b03ad 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -644,6 +644,7 @@ enum {
>   	QUEUE_FLAG_RQ_ALLOC_TIME,	/* record rq->alloc_time_ns */
>   	QUEUE_FLAG_HCTX_ACTIVE,		/* at least one blk-mq hctx is active */
>   	QUEUE_FLAG_SQ_SCHED,		/* single queue style io dispatch */
> +	QUEUE_FLAG_DISABLE_WBT,		/* for sched to disable/enable wbt */

In fact, user can still enabel wbt by sysfs api. Perhaps change the name
to QUEUE_FLAG_WBT_DISABLE_DEFAULT?

Thanks,
Kuai

>   	QUEUE_FLAG_MAX
>   };
>   
> @@ -679,6 +680,8 @@ void blk_queue_flag_clear(unsigned int flag, struct request_queue *q);
>   #define blk_queue_sq_sched(q)	test_bit(QUEUE_FLAG_SQ_SCHED, &(q)->queue_flags)
>   #define blk_queue_skip_tagset_quiesce(q) \
>   	((q)->limits.features & BLK_FEAT_SKIP_TAGSET_QUIESCE)
> +#define blk_queue_disable_wbt(q)	\
> +	test_bit(QUEUE_FLAG_DISABLE_WBT, &(q)->queue_flags)
>   
>   extern void blk_set_pm_only(struct request_queue *q);
>   extern void blk_clear_pm_only(struct request_queue *q);
> 

Re: [PATCH V2 02/20] block: move ELEVATOR_FLAG_DISABLE_WBT as request queue flag

[Nilay Shroff]

On 4/18/25 10:06 PM, Ming Lei wrote:
> ELEVATOR_FLAG_DISABLE_WBT is only used by BFQ to disallow wbt when BFQ is
> in use. The flag is set in BFQ's init(), and cleared in BFQ's exit().
> 
> Making it as request queue flag, so that we can avoid to deal with elevator
> switch race. Also it isn't graceful to checking one scheduler flag in
> wbt_enable_default().
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Looks good to me:
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

Re: [PATCH V2 02/20] block: move ELEVATOR_FLAG_DISABLE_WBT as request queue flag

[Christoph Hellwig]

s/as/a/ in the Subject, and I also agree about the naming comment.

Otherwise this looks good.

Re: [PATCH V2 02/20] block: move ELEVATOR_FLAG_DISABLE_WBT as request queue flag

[Hannes Reinecke]

On 4/18/25 18:36, Ming Lei wrote:
> ELEVATOR_FLAG_DISABLE_WBT is only used by BFQ to disallow wbt when BFQ is
> in use. The flag is set in BFQ's init(), and cleared in BFQ's exit().
> 
> Making it as request queue flag, so that we can avoid to deal with elevator
> switch race. Also it isn't graceful to checking one scheduler flag in
> wbt_enable_default().
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/bfq-iosched.c    | 4 ++--
>   block/blk-mq-debugfs.c | 1 +
>   block/blk-wbt.c        | 3 +--
>   block/elevator.h       | 1 -
>   include/linux/blkdev.h | 3 +++
>   5 files changed, 7 insertions(+), 5 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH V2 03/20] block: don't call freeze queue in elevator_switch() and elevator_disable()

[Ming Lei]

Both elevator_switch() and elevator_disable() are called from sysfs
store and updating nr_hw_queue code paths only.

And in the two code paths, queue has been frozen already, so don't call
freeze queue in the two functions.

Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/elevator.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index b4d08026b02c..5051a98dc08c 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -615,12 +615,11 @@ void elevator_init_mq(struct request_queue *q)
  */
 int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
 {
-	unsigned int memflags;
 	int ret;
 
+	WARN_ON_ONCE(q->mq_freeze_depth == 0);
 	lockdep_assert_held(&q->elevator_lock);
 
-	memflags = blk_mq_freeze_queue(q);
 	blk_mq_quiesce_queue(q);
 
 	if (q->elevator) {
@@ -641,7 +640,6 @@ int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
 
 out_unfreeze:
 	blk_mq_unquiesce_queue(q);
-	blk_mq_unfreeze_queue(q, memflags);
 
 	if (ret) {
 		pr_warn("elv: switch to \"%s\" failed, falling back to \"none\"\n",
@@ -653,11 +651,9 @@ int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
 
 void elevator_disable(struct request_queue *q)
 {
-	unsigned int memflags;
-
+	WARN_ON_ONCE(q->mq_freeze_depth == 0);
 	lockdep_assert_held(&q->elevator_lock);
 
-	memflags = blk_mq_freeze_queue(q);
 	blk_mq_quiesce_queue(q);
 
 	elv_unregister_queue(q);
@@ -668,7 +664,6 @@ void elevator_disable(struct request_queue *q)
 	blk_add_trace_msg(q, "elv switch: none");
 
 	blk_mq_unquiesce_queue(q);
-	blk_mq_unfreeze_queue(q, memflags);
 }
 
 /*
-- 
2.47.0

Re: [PATCH V2 03/20] block: don't call freeze queue in elevator_switch() and elevator_disable()

[Yu Kuai]

在 2025/04/19 0:36, Ming Lei 写道:
> Both elevator_switch() and elevator_disable() are called from sysfs
> store and updating nr_hw_queue code paths only.
> 
> And in the two code paths, queue has been frozen already, so don't call
> freeze queue in the two functions.
> 
> Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/elevator.c | 9 ++-------
>   1 file changed, 2 insertions(+), 7 deletions(-)
> 
LGTM
Reviewed-by: Yu Kuai <yukuai3@huawei.com>

> diff --git a/block/elevator.c b/block/elevator.c
> index b4d08026b02c..5051a98dc08c 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -615,12 +615,11 @@ void elevator_init_mq(struct request_queue *q)
>    */
>   int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
>   {
> -	unsigned int memflags;
>   	int ret;
>   
> +	WARN_ON_ONCE(q->mq_freeze_depth == 0);
>   	lockdep_assert_held(&q->elevator_lock);
>   
> -	memflags = blk_mq_freeze_queue(q);
>   	blk_mq_quiesce_queue(q);
>   
>   	if (q->elevator) {
> @@ -641,7 +640,6 @@ int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
>   
>   out_unfreeze:
>   	blk_mq_unquiesce_queue(q);
> -	blk_mq_unfreeze_queue(q, memflags);
>   
>   	if (ret) {
>   		pr_warn("elv: switch to \"%s\" failed, falling back to \"none\"\n",
> @@ -653,11 +651,9 @@ int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
>   
>   void elevator_disable(struct request_queue *q)
>   {
> -	unsigned int memflags;
> -
> +	WARN_ON_ONCE(q->mq_freeze_depth == 0);
>   	lockdep_assert_held(&q->elevator_lock);
>   
> -	memflags = blk_mq_freeze_queue(q);
>   	blk_mq_quiesce_queue(q);
>   
>   	elv_unregister_queue(q);
> @@ -668,7 +664,6 @@ void elevator_disable(struct request_queue *q)
>   	blk_add_trace_msg(q, "elv switch: none");
>   
>   	blk_mq_unquiesce_queue(q);
> -	blk_mq_unfreeze_queue(q, memflags);
>   }
>   
>   /*
> 

Re: [PATCH V2 03/20] block: don't call freeze queue in elevator_switch() and elevator_disable()

[Christoph Hellwig]

On Sat, Apr 19, 2025 at 12:36:44AM +0800, Ming Lei wrote:
> Both elevator_switch() and elevator_disable() are called from sysfs
> store and updating nr_hw_queue code paths only.

I don't understand this sentence.  What does
"and updating nr_hw_queue code paths only" mean?

Re: [PATCH V2 03/20] block: don't call freeze queue in elevator_switch() and elevator_disable()

[Ming Lei]

On Mon, Apr 21, 2025 at 02:34:56PM +0200, Christoph Hellwig wrote:
> On Sat, Apr 19, 2025 at 12:36:44AM +0800, Ming Lei wrote:
> > Both elevator_switch() and elevator_disable() are called from sysfs
> > store and updating nr_hw_queue code paths only.
> 
> I don't understand this sentence.  What does
> "and updating nr_hw_queue code paths only" mean?

Both two are only called from the two code paths, in which queue is
guaranteed to be frozen.


Thanks,
Ming

Re: [PATCH V2 03/20] block: don't call freeze queue in elevator_switch() and elevator_disable()

[Christoph Hellwig]

On Tue, Apr 22, 2025 at 12:00:44PM +0800, Ming Lei wrote:
> On Mon, Apr 21, 2025 at 02:34:56PM +0200, Christoph Hellwig wrote:
> > On Sat, Apr 19, 2025 at 12:36:44AM +0800, Ming Lei wrote:
> > > Both elevator_switch() and elevator_disable() are called from sysfs
> > > store and updating nr_hw_queue code paths only.
> > 
> > I don't understand this sentence.  What does
> > "and updating nr_hw_queue code paths only" mean?
> 
> Both two are only called from the two code paths, in which queue is
> guaranteed to be frozen.

Yes, that's also verified by the asserts.  But the commit message doesn't
sound like that at all.

Re: [PATCH V2 03/20] block: don't call freeze queue in elevator_switch() and elevator_disable()

[Hannes Reinecke]

On 4/18/25 18:36, Ming Lei wrote:
> Both elevator_switch() and elevator_disable() are called from sysfs
> store and updating nr_hw_queue code paths only.
> 
> And in the two code paths, queue has been frozen already, so don't call
> freeze queue in the two functions.
> 
> Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/elevator.c | 9 ++-------
>   1 file changed, 2 insertions(+), 7 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH V2 04/20] block: add two helpers for registering/un-registering sched debugfs

[Ming Lei]

Add blk_mq_sched_reg_debugfs()/blk_mq_sched_unreg_debugfs() to clean up
sched init/exit code a bit.

Register & unregister debugfs for sched & sched_hctx order is changed a
bit, but it is safe because sched & sched_hctx is guaranteed to be ready
when exporting via debugfs.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq-sched.c | 45 +++++++++++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 9b81771774ef..2abc5e0704e8 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -434,6 +434,30 @@ static int blk_mq_init_sched_shared_tags(struct request_queue *queue)
 	return 0;
 }
 
+static void blk_mq_sched_reg_debugfs(struct request_queue *q)
+{
+	struct blk_mq_hw_ctx *hctx;
+	unsigned long i;
+
+	mutex_lock(&q->debugfs_mutex);
+	blk_mq_debugfs_register_sched(q);
+	queue_for_each_hw_ctx(q, hctx, i)
+		blk_mq_debugfs_register_sched_hctx(q, hctx);
+	mutex_unlock(&q->debugfs_mutex);
+}
+
+static void blk_mq_sched_unreg_debugfs(struct request_queue *q)
+{
+	struct blk_mq_hw_ctx *hctx;
+	unsigned long i;
+
+	mutex_lock(&q->debugfs_mutex);
+	queue_for_each_hw_ctx(q, hctx, i)
+		blk_mq_debugfs_unregister_sched_hctx(hctx);
+	blk_mq_debugfs_unregister_sched(q);
+	mutex_unlock(&q->debugfs_mutex);
+}
+
 /* 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)
 {
@@ -467,10 +491,6 @@ int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)
 	if (ret)
 		goto err_free_map_and_rqs;
 
-	mutex_lock(&q->debugfs_mutex);
-	blk_mq_debugfs_register_sched(q);
-	mutex_unlock(&q->debugfs_mutex);
-
 	queue_for_each_hw_ctx(q, hctx, i) {
 		if (e->ops.init_hctx) {
 			ret = e->ops.init_hctx(hctx, i);
@@ -482,11 +502,11 @@ int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)
 				return ret;
 			}
 		}
-		mutex_lock(&q->debugfs_mutex);
-		blk_mq_debugfs_register_sched_hctx(q, hctx);
-		mutex_unlock(&q->debugfs_mutex);
 	}
 
+	/* sched is initialized, it is ready to export it via debugfs */
+	blk_mq_sched_reg_debugfs(q);
+
 	return 0;
 
 err_free_map_and_rqs:
@@ -524,11 +544,10 @@ void blk_mq_exit_sched(struct request_queue *q, struct elevator_queue *e)
 	unsigned long i;
 	unsigned int flags = 0;
 
-	queue_for_each_hw_ctx(q, hctx, i) {
-		mutex_lock(&q->debugfs_mutex);
-		blk_mq_debugfs_unregister_sched_hctx(hctx);
-		mutex_unlock(&q->debugfs_mutex);
+	/* unexport via debugfs before exiting sched */
+	blk_mq_sched_unreg_debugfs(q);
 
+	queue_for_each_hw_ctx(q, hctx, i) {
 		if (e->type->ops.exit_hctx && hctx->sched_data) {
 			e->type->ops.exit_hctx(hctx, i);
 			hctx->sched_data = NULL;
@@ -536,10 +555,6 @@ void blk_mq_exit_sched(struct request_queue *q, struct elevator_queue *e)
 		flags = hctx->flags;
 	}
 
-	mutex_lock(&q->debugfs_mutex);
-	blk_mq_debugfs_unregister_sched(q);
-	mutex_unlock(&q->debugfs_mutex);
-
 	if (e->type->ops.exit_sched)
 		e->type->ops.exit_sched(e);
 	blk_mq_sched_tags_teardown(q, flags);
-- 
2.47.0

Re: [PATCH V2 04/20] block: add two helpers for registering/un-registering sched debugfs

[Yu Kuai]

在 2025/04/19 0:36, Ming Lei 写道:
> Add blk_mq_sched_reg_debugfs()/blk_mq_sched_unreg_debugfs() to clean up
> sched init/exit code a bit.
> 
> Register & unregister debugfs for sched & sched_hctx order is changed a
> bit, but it is safe because sched & sched_hctx is guaranteed to be ready
> when exporting via debugfs.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/blk-mq-sched.c | 45 +++++++++++++++++++++++++++++---------------
>   1 file changed, 30 insertions(+), 15 deletions(-)
> 
LGTM
Reviewed-by: Yu Kuai <yukuai3@huawei.com>

> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> index 9b81771774ef..2abc5e0704e8 100644
> --- a/block/blk-mq-sched.c
> +++ b/block/blk-mq-sched.c
> @@ -434,6 +434,30 @@ static int blk_mq_init_sched_shared_tags(struct request_queue *queue)
>   	return 0;
>   }
>   
> +static void blk_mq_sched_reg_debugfs(struct request_queue *q)
> +{
> +	struct blk_mq_hw_ctx *hctx;
> +	unsigned long i;
> +
> +	mutex_lock(&q->debugfs_mutex);
> +	blk_mq_debugfs_register_sched(q);
> +	queue_for_each_hw_ctx(q, hctx, i)
> +		blk_mq_debugfs_register_sched_hctx(q, hctx);
> +	mutex_unlock(&q->debugfs_mutex);
> +}
> +
> +static void blk_mq_sched_unreg_debugfs(struct request_queue *q)
> +{
> +	struct blk_mq_hw_ctx *hctx;
> +	unsigned long i;
> +
> +	mutex_lock(&q->debugfs_mutex);
> +	queue_for_each_hw_ctx(q, hctx, i)
> +		blk_mq_debugfs_unregister_sched_hctx(hctx);
> +	blk_mq_debugfs_unregister_sched(q);
> +	mutex_unlock(&q->debugfs_mutex);
> +}
> +
>   /* 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)
>   {
> @@ -467,10 +491,6 @@ int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)
>   	if (ret)
>   		goto err_free_map_and_rqs;
>   
> -	mutex_lock(&q->debugfs_mutex);
> -	blk_mq_debugfs_register_sched(q);
> -	mutex_unlock(&q->debugfs_mutex);
> -
>   	queue_for_each_hw_ctx(q, hctx, i) {
>   		if (e->ops.init_hctx) {
>   			ret = e->ops.init_hctx(hctx, i);
> @@ -482,11 +502,11 @@ int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)
>   				return ret;
>   			}
>   		}
> -		mutex_lock(&q->debugfs_mutex);
> -		blk_mq_debugfs_register_sched_hctx(q, hctx);
> -		mutex_unlock(&q->debugfs_mutex);
>   	}
>   
> +	/* sched is initialized, it is ready to export it via debugfs */
> +	blk_mq_sched_reg_debugfs(q);
> +
>   	return 0;
>   
>   err_free_map_and_rqs:
> @@ -524,11 +544,10 @@ void blk_mq_exit_sched(struct request_queue *q, struct elevator_queue *e)
>   	unsigned long i;
>   	unsigned int flags = 0;
>   
> -	queue_for_each_hw_ctx(q, hctx, i) {
> -		mutex_lock(&q->debugfs_mutex);
> -		blk_mq_debugfs_unregister_sched_hctx(hctx);
> -		mutex_unlock(&q->debugfs_mutex);
> +	/* unexport via debugfs before exiting sched */
> +	blk_mq_sched_unreg_debugfs(q);
>   
> +	queue_for_each_hw_ctx(q, hctx, i) {
>   		if (e->type->ops.exit_hctx && hctx->sched_data) {
>   			e->type->ops.exit_hctx(hctx, i);
>   			hctx->sched_data = NULL;
> @@ -536,10 +555,6 @@ void blk_mq_exit_sched(struct request_queue *q, struct elevator_queue *e)
>   		flags = hctx->flags;
>   	}
>   
> -	mutex_lock(&q->debugfs_mutex);
> -	blk_mq_debugfs_unregister_sched(q);
> -	mutex_unlock(&q->debugfs_mutex);
> -
>   	if (e->type->ops.exit_sched)
>   		e->type->ops.exit_sched(e);
>   	blk_mq_sched_tags_teardown(q, flags);
> 

Re: [PATCH V2 04/20] block: add two helpers for registering/un-registering sched debugfs

[Nilay Shroff]

On 4/18/25 10:06 PM, Ming Lei wrote:
> Add blk_mq_sched_reg_debugfs()/blk_mq_sched_unreg_debugfs() to clean up
> sched init/exit code a bit.
> 
> Register & unregister debugfs for sched & sched_hctx order is changed a
> bit, but it is safe because sched & sched_hctx is guaranteed to be ready
> when exporting via debugfs.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Looks good to me:
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

Re: [PATCH V2 04/20] block: add two helpers for registering/un-registering sched debugfs

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH V2 04/20] block: add two helpers for registering/un-registering sched debugfs

[Hannes Reinecke]

On 4/18/25 18:36, Ming Lei wrote:
> Add blk_mq_sched_reg_debugfs()/blk_mq_sched_unreg_debugfs() to clean up
> sched init/exit code a bit.
> 
> Register & unregister debugfs for sched & sched_hctx order is changed a
> bit, but it is safe because sched & sched_hctx is guaranteed to be ready
> when exporting via debugfs.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/blk-mq-sched.c | 45 +++++++++++++++++++++++++++++---------------
>   1 file changed, 30 insertions(+), 15 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH V2 05/20] block: move sched debugfs register into elvevator_register_queue

[Ming Lei]

sched debugfs shares same lifetime with scheduler's kobject, and same
lock(elevator lock), so move sched debugfs register/unregister into
elevator_register_queue() and elevator_unregister_queue().

Then we needn't blk_mq_debugfs_register() for us to register sched
debugfs any more.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq-debugfs.c | 11 -----------
 block/blk-mq-sched.c   | 11 ++---------
 block/elevator.c       |  8 ++++++++
 block/elevator.h       |  3 +++
 4 files changed, 13 insertions(+), 20 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 31e249a18407..0fa0f8836b7d 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -625,20 +625,9 @@ void blk_mq_debugfs_register(struct request_queue *q)
 
 	debugfs_create_files(q->debugfs_dir, q, blk_mq_debugfs_queue_attrs);
 
-	/*
-	 * blk_mq_init_sched() attempted to do this already, but q->debugfs_dir
-	 * didn't exist yet (because we don't know what to name the directory
-	 * until the queue is registered to a gendisk).
-	 */
-	if (q->elevator && !q->sched_debugfs_dir)
-		blk_mq_debugfs_register_sched(q);
-
-	/* Similarly, blk_mq_init_hctx() couldn't do this previously. */
 	queue_for_each_hw_ctx(q, hctx, i) {
 		if (!hctx->debugfs_dir)
 			blk_mq_debugfs_register_hctx(q, hctx);
-		if (q->elevator && !hctx->sched_debugfs_dir)
-			blk_mq_debugfs_register_sched_hctx(q, hctx);
 	}
 
 	if (q->rq_qos) {
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 2abc5e0704e8..336a15ffecfa 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -434,7 +434,7 @@ static int blk_mq_init_sched_shared_tags(struct request_queue *queue)
 	return 0;
 }
 
-static void blk_mq_sched_reg_debugfs(struct request_queue *q)
+void blk_mq_sched_reg_debugfs(struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
 	unsigned long i;
@@ -446,7 +446,7 @@ static void blk_mq_sched_reg_debugfs(struct request_queue *q)
 	mutex_unlock(&q->debugfs_mutex);
 }
 
-static void blk_mq_sched_unreg_debugfs(struct request_queue *q)
+void blk_mq_sched_unreg_debugfs(struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
 	unsigned long i;
@@ -503,10 +503,6 @@ int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)
 			}
 		}
 	}
-
-	/* sched is initialized, it is ready to export it via debugfs */
-	blk_mq_sched_reg_debugfs(q);
-
 	return 0;
 
 err_free_map_and_rqs:
@@ -544,9 +540,6 @@ void blk_mq_exit_sched(struct request_queue *q, struct elevator_queue *e)
 	unsigned long i;
 	unsigned int flags = 0;
 
-	/* unexport via debugfs before exiting sched */
-	blk_mq_sched_unreg_debugfs(q);
-
 	queue_for_each_hw_ctx(q, hctx, i) {
 		if (e->type->ops.exit_hctx && hctx->sched_data) {
 			e->type->ops.exit_hctx(hctx, i);
diff --git a/block/elevator.c b/block/elevator.c
index 5051a98dc08c..d25b9cc6c509 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -472,6 +472,11 @@ int elv_register_queue(struct request_queue *q, bool uevent)
 		if (uevent)
 			kobject_uevent(&e->kobj, KOBJ_ADD);
 
+		/*
+		 * Sched is initialized, it is ready to export it via
+		 * debugfs
+		 */
+		blk_mq_sched_reg_debugfs(q);
 		set_bit(ELEVATOR_FLAG_REGISTERED, &e->flags);
 	}
 	return error;
@@ -486,6 +491,9 @@ void elv_unregister_queue(struct request_queue *q)
 	if (e && test_and_clear_bit(ELEVATOR_FLAG_REGISTERED, &e->flags)) {
 		kobject_uevent(&e->kobj, KOBJ_REMOVE);
 		kobject_del(&e->kobj);
+
+		/* unexport via debugfs before exiting sched */
+		blk_mq_sched_unreg_debugfs(q);
 	}
 }
 
diff --git a/block/elevator.h b/block/elevator.h
index e27af5492cdb..9198676644a9 100644
--- a/block/elevator.h
+++ b/block/elevator.h
@@ -181,4 +181,7 @@ extern struct request *elv_rb_find(struct rb_root *, sector_t);
 #define rq_entry_fifo(ptr)	list_entry((ptr), struct request, queuelist)
 #define rq_fifo_clear(rq)	list_del_init(&(rq)->queuelist)
 
+void blk_mq_sched_reg_debugfs(struct request_queue *q);
+void blk_mq_sched_unreg_debugfs(struct request_queue *q);
+
 #endif /* _ELEVATOR_H */
-- 
2.47.0

Re: [PATCH V2 05/20] block: move sched debugfs register into elvevator_register_queue

[Yu Kuai]

在 2025/04/19 0:36, Ming Lei 写道:
> sched debugfs shares same lifetime with scheduler's kobject, and same
> lock(elevator lock), so move sched debugfs register/unregister into
> elevator_register_queue() and elevator_unregister_queue().
> 
> Then we needn't blk_mq_debugfs_register() for us to register sched
> debugfs any more.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/blk-mq-debugfs.c | 11 -----------
>   block/blk-mq-sched.c   | 11 ++---------
>   block/elevator.c       |  8 ++++++++
>   block/elevator.h       |  3 +++
>   4 files changed, 13 insertions(+), 20 deletions(-)
> 
LGTM
Reviewed-by: Yu Kuai <yukuai3@huawei.com>

> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index 31e249a18407..0fa0f8836b7d 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -625,20 +625,9 @@ void blk_mq_debugfs_register(struct request_queue *q)
>   
>   	debugfs_create_files(q->debugfs_dir, q, blk_mq_debugfs_queue_attrs);
>   
> -	/*
> -	 * blk_mq_init_sched() attempted to do this already, but q->debugfs_dir
> -	 * didn't exist yet (because we don't know what to name the directory
> -	 * until the queue is registered to a gendisk).
> -	 */
> -	if (q->elevator && !q->sched_debugfs_dir)
> -		blk_mq_debugfs_register_sched(q);
> -
> -	/* Similarly, blk_mq_init_hctx() couldn't do this previously. */
>   	queue_for_each_hw_ctx(q, hctx, i) {
>   		if (!hctx->debugfs_dir)
>   			blk_mq_debugfs_register_hctx(q, hctx);
> -		if (q->elevator && !hctx->sched_debugfs_dir)
> -			blk_mq_debugfs_register_sched_hctx(q, hctx);
>   	}
>   
>   	if (q->rq_qos) {
> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> index 2abc5e0704e8..336a15ffecfa 100644
> --- a/block/blk-mq-sched.c
> +++ b/block/blk-mq-sched.c
> @@ -434,7 +434,7 @@ static int blk_mq_init_sched_shared_tags(struct request_queue *queue)
>   	return 0;
>   }
>   
> -static void blk_mq_sched_reg_debugfs(struct request_queue *q)
> +void blk_mq_sched_reg_debugfs(struct request_queue *q)
>   {
>   	struct blk_mq_hw_ctx *hctx;
>   	unsigned long i;
> @@ -446,7 +446,7 @@ static void blk_mq_sched_reg_debugfs(struct request_queue *q)
>   	mutex_unlock(&q->debugfs_mutex);
>   }
>   
> -static void blk_mq_sched_unreg_debugfs(struct request_queue *q)
> +void blk_mq_sched_unreg_debugfs(struct request_queue *q)
>   {
>   	struct blk_mq_hw_ctx *hctx;
>   	unsigned long i;
> @@ -503,10 +503,6 @@ int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)
>   			}
>   		}
>   	}
> -
> -	/* sched is initialized, it is ready to export it via debugfs */
> -	blk_mq_sched_reg_debugfs(q);
> -
>   	return 0;
>   
>   err_free_map_and_rqs:
> @@ -544,9 +540,6 @@ void blk_mq_exit_sched(struct request_queue *q, struct elevator_queue *e)
>   	unsigned long i;
>   	unsigned int flags = 0;
>   
> -	/* unexport via debugfs before exiting sched */
> -	blk_mq_sched_unreg_debugfs(q);
> -
>   	queue_for_each_hw_ctx(q, hctx, i) {
>   		if (e->type->ops.exit_hctx && hctx->sched_data) {
>   			e->type->ops.exit_hctx(hctx, i);
> diff --git a/block/elevator.c b/block/elevator.c
> index 5051a98dc08c..d25b9cc6c509 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -472,6 +472,11 @@ int elv_register_queue(struct request_queue *q, bool uevent)
>   		if (uevent)
>   			kobject_uevent(&e->kobj, KOBJ_ADD);
>   
> +		/*
> +		 * Sched is initialized, it is ready to export it via
> +		 * debugfs
> +		 */
> +		blk_mq_sched_reg_debugfs(q);
>   		set_bit(ELEVATOR_FLAG_REGISTERED, &e->flags);
>   	}
>   	return error;
> @@ -486,6 +491,9 @@ void elv_unregister_queue(struct request_queue *q)
>   	if (e && test_and_clear_bit(ELEVATOR_FLAG_REGISTERED, &e->flags)) {
>   		kobject_uevent(&e->kobj, KOBJ_REMOVE);
>   		kobject_del(&e->kobj);
> +
> +		/* unexport via debugfs before exiting sched */
> +		blk_mq_sched_unreg_debugfs(q);
>   	}
>   }
>   
> diff --git a/block/elevator.h b/block/elevator.h
> index e27af5492cdb..9198676644a9 100644
> --- a/block/elevator.h
> +++ b/block/elevator.h
> @@ -181,4 +181,7 @@ extern struct request *elv_rb_find(struct rb_root *, sector_t);
>   #define rq_entry_fifo(ptr)	list_entry((ptr), struct request, queuelist)
>   #define rq_fifo_clear(rq)	list_del_init(&(rq)->queuelist)
>   
> +void blk_mq_sched_reg_debugfs(struct request_queue *q);
> +void blk_mq_sched_unreg_debugfs(struct request_queue *q);
> +
>   #endif /* _ELEVATOR_H */
> 

Re: [PATCH V2 05/20] block: move sched debugfs register into elvevator_register_queue

[Nilay Shroff]

On 4/18/25 10:06 PM, Ming Lei wrote:
> sched debugfs shares same lifetime with scheduler's kobject, and same
> lock(elevator lock), so move sched debugfs register/unregister into
> elevator_register_queue() and elevator_unregister_queue().
> 
> Then we needn't blk_mq_debugfs_register() for us to register sched
> debugfs any more.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Looks goot to me:
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

Re: [PATCH V2 05/20] block: move sched debugfs register into elvevator_register_queue

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH V2 05/20] block: move sched debugfs register into elvevator_register_queue

[Hannes Reinecke]

On 4/18/25 18:36, Ming Lei wrote:
> sched debugfs shares same lifetime with scheduler's kobject, and same
> lock(elevator lock), so move sched debugfs register/unregister into
> elevator_register_queue() and elevator_unregister_queue().
> 
> Then we needn't blk_mq_debugfs_register() for us to register sched
> debugfs any more.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/blk-mq-debugfs.c | 11 -----------
>   block/blk-mq-sched.c   | 11 ++---------
>   block/elevator.c       |  8 ++++++++
>   block/elevator.h       |  3 +++
>   4 files changed, 13 insertions(+), 20 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH V2 06/20] block: add & pass 'struct gendisk_data' for retrying add/del disk in updating nr_hw_queues

[Ming Lei]

Add/del gendisk code is actually reader of 'nr_hw_queues' in case of blk-mq:

- debugfs / hctx sysfs register

- setup scheduler since ->sched_tags depends on hctx, which relies on
'nr_hw_queues'

Add & pass 'struct gendisk_data' to add/del disk helper and prepare for
retrying add/del disk when updating nr_hw_queues is in-progress.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/genhd.c | 105 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 67 insertions(+), 38 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index c2bd86cd09de..4370c5be1f34 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -33,6 +33,13 @@
 #include "blk-rq-qos.h"
 #include "blk-cgroup.h"
 
+struct gendisk_data {
+	struct gendisk *disk;
+	struct device *parent;
+	const struct attribute_group **groups;
+	struct fwnode_handle *fwnode;
+};
+
 static struct kobject *block_depr;
 
 /*
@@ -389,21 +396,9 @@ int disk_scan_partitions(struct gendisk *disk, blk_mode_t mode)
 	return ret;
 }
 
-/**
- * add_disk_fwnode - add disk information to kernel list with fwnode
- * @parent: parent device for the disk
- * @disk: per-device partitioning information
- * @groups: Additional per-device sysfs groups
- * @fwnode: attached disk fwnode
- *
- * This function registers the partitioning information in @disk
- * with the kernel. Also attach a fwnode to the disk device.
- */
-int __must_check add_disk_fwnode(struct device *parent, struct gendisk *disk,
-				 const struct attribute_group **groups,
-				 struct fwnode_handle *fwnode)
-
+static int __add_disk_fwnode(struct gendisk_data *data)
 {
+	struct gendisk *disk = data->disk;
 	struct device *ddev = disk_to_dev(disk);
 	int ret;
 
@@ -463,11 +458,11 @@ int __must_check add_disk_fwnode(struct device *parent, struct gendisk *disk,
 	/* delay uevents, until we scanned partition table */
 	dev_set_uevent_suppress(ddev, 1);
 
-	ddev->parent = parent;
-	ddev->groups = groups;
+	ddev->parent = data->parent;
+	ddev->groups = data->groups;
 	dev_set_name(ddev, "%s", disk->disk_name);
-	if (fwnode)
-		device_set_node(ddev, fwnode);
+	if (data->fwnode)
+		device_set_node(ddev, data->fwnode);
 	if (!(disk->flags & GENHD_FL_HIDDEN))
 		ddev->devt = MKDEV(disk->major, disk->first_minor);
 	ret = device_add(ddev);
@@ -572,6 +567,30 @@ int __must_check add_disk_fwnode(struct device *parent, struct gendisk *disk,
 	}
 	return ret;
 }
+
+/**
+ * add_disk_fwnode - add disk information to kernel list with fwnode
+ * @parent: parent device for the disk
+ * @disk: per-device partitioning information
+ * @groups: Additional per-device sysfs groups
+ * @fwnode: attached disk fwnode
+ *
+ * This function registers the partitioning information in @disk
+ * with the kernel. Also attach a fwnode to the disk device.
+ */
+int __must_check add_disk_fwnode(struct device *parent, struct gendisk *disk,
+				 const struct attribute_group **groups,
+				 struct fwnode_handle *fwnode)
+{
+	struct gendisk_data data = {
+		.disk	= disk,
+		.parent	= parent,
+		.groups	= groups,
+		.fwnode = fwnode,
+	};
+
+	return __add_disk_fwnode(&data);
+}
 EXPORT_SYMBOL_GPL(add_disk_fwnode);
 
 /**
@@ -652,27 +671,9 @@ void blk_mark_disk_dead(struct gendisk *disk)
 }
 EXPORT_SYMBOL_GPL(blk_mark_disk_dead);
 
-/**
- * del_gendisk - remove the gendisk
- * @disk: the struct gendisk to remove
- *
- * Removes the gendisk and all its associated resources. This deletes the
- * partitions associated with the gendisk, and unregisters the associated
- * request_queue.
- *
- * This is the counter to the respective __device_add_disk() call.
- *
- * The final removal of the struct gendisk happens when its refcount reaches 0
- * with put_disk(), which should be called after del_gendisk(), if
- * __device_add_disk() was used.
- *
- * Drivers exist which depend on the release of the gendisk to be synchronous,
- * it should not be deferred.
- *
- * Context: can sleep
- */
-void del_gendisk(struct gendisk *disk)
+static void __del_gendisk(struct gendisk_data *data)
 {
+	struct gendisk *disk = data->disk;
 	struct request_queue *q = disk->queue;
 	struct block_device *part;
 	unsigned long idx;
@@ -766,6 +767,34 @@ void del_gendisk(struct gendisk *disk)
 }
 EXPORT_SYMBOL(del_gendisk);
 
+/**
+ * del_gendisk - remove the gendisk
+ * @disk: the struct gendisk to remove
+ *
+ * Removes the gendisk and all its associated resources. This deletes the
+ * partitions associated with the gendisk, and unregisters the associated
+ * request_queue.
+ *
+ * This is the counter to the respective __device_add_disk() call.
+ *
+ * The final removal of the struct gendisk happens when its refcount reaches 0
+ * with put_disk(), which should be called after del_gendisk(), if
+ * __device_add_disk() was used.
+ *
+ * Drivers exist which depend on the release of the gendisk to be synchronous,
+ * it should not be deferred.
+ *
+ * Context: can sleep
+ */
+void del_gendisk(struct gendisk *disk)
+{
+	struct gendisk_data data = {
+		.disk	= disk,
+	};
+
+	__del_gendisk(&data);
+}
+
 /**
  * invalidate_disk - invalidate the disk
  * @disk: the struct gendisk to invalidate
-- 
2.47.0

[PATCH V2 07/20] block: prevent adding/deleting disk during updating nr_hw_queues

[Ming Lei]

Both adding/deleting disk code are reader of `nr_hw_queues`, so we can't
allow them in-progress when updating nr_hw_queues, kernel panic and
kasan has been reported in [1].

Prevent adding/deleting disk during updating nr_hw_queues by setting
set->updating_nr_hwq, and use SRCU to fail & retry to add/delete disk.

This way avoids lot of trouble.

Reported-by: Nilay Shroff <nilay@linux.ibm.com>
Closes: https://lore.kernel.org/linux-block/a5896cdb-a59a-4a37-9f99-20522f5d2987@linux.ibm.com/
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c         | 22 +++++++++++++++++++++-
 block/genhd.c          | 36 ++++++++++++++++++++++++++++++++----
 include/linux/blk-mq.h |  5 +++++
 3 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 7cda919fafba..e1662617cc7a 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4782,12 +4782,18 @@ int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set)
 			goto out_free_srcu;
 	}
 
+	mutex_init(&set->update_nr_hwq_lock);
+	init_waitqueue_head(&set->update_nr_hwq_wq);
+	ret = init_srcu_struct(&set->update_nr_hwq_srcu);
+	if (ret)
+		goto out_cleanup_srcu;
+
 	ret = -ENOMEM;
 	set->tags = kcalloc_node(set->nr_hw_queues,
 				 sizeof(struct blk_mq_tags *), GFP_KERNEL,
 				 set->numa_node);
 	if (!set->tags)
-		goto out_cleanup_srcu;
+		goto out_cleanup_hwq_srcu;
 
 	for (i = 0; i < set->nr_maps; i++) {
 		set->map[i].mq_map = kcalloc_node(nr_cpu_ids,
@@ -4816,6 +4822,8 @@ int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set)
 	}
 	kfree(set->tags);
 	set->tags = NULL;
+out_cleanup_hwq_srcu:
+	cleanup_srcu_struct(&set->update_nr_hwq_srcu);
 out_cleanup_srcu:
 	if (set->flags & BLK_MQ_F_BLOCKING)
 		cleanup_srcu_struct(set->srcu);
@@ -5077,9 +5085,21 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 
 void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int nr_hw_queues)
 {
+	mutex_lock(&set->update_nr_hwq_lock);
+	/*
+	 * Mark us in updating nr_hw_queues for preventing reader of
+	 * nr_hw_queues, such as adding/deleting disk.
+	 */
+	set->updating_nr_hwq = true;
+	synchronize_srcu(&set->update_nr_hwq_srcu);
+
 	mutex_lock(&set->tag_list_lock);
 	__blk_mq_update_nr_hw_queues(set, nr_hw_queues);
 	mutex_unlock(&set->tag_list_lock);
+
+	set->updating_nr_hwq = false;
+	wake_up_all(&set->update_nr_hwq_wq);
+	mutex_unlock(&set->update_nr_hwq_lock);
 }
 EXPORT_SYMBOL_GPL(blk_mq_update_nr_hw_queues);
 
diff --git a/block/genhd.c b/block/genhd.c
index 4370c5be1f34..d22fdc0d5383 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -396,6 +396,33 @@ int disk_scan_partitions(struct gendisk *disk, blk_mode_t mode)
 	return ret;
 }
 
+static int retry_on_updating_nr_hwq(struct gendisk_data *data,
+				    int (*cb)(struct gendisk_data *data))
+{
+	struct gendisk *disk = data->disk;
+	struct blk_mq_tag_set *set;
+
+	if (!queue_is_mq(disk->queue))
+		return cb(data);
+
+	set = disk->queue->tag_set;
+	do {
+		int idx, ret;
+
+		idx = srcu_read_lock(&set->update_nr_hwq_srcu);
+		if (set->updating_nr_hwq) {
+			srcu_read_unlock(&set->update_nr_hwq_srcu, idx);
+			goto wait;
+		}
+		ret = cb(data);
+		srcu_read_unlock(&set->update_nr_hwq_srcu, idx);
+		return ret;
+ wait:
+		wait_event_interruptible(set->update_nr_hwq_wq,
+				!set->updating_nr_hwq);
+	} while (true);
+}
+
 static int __add_disk_fwnode(struct gendisk_data *data)
 {
 	struct gendisk *disk = data->disk;
@@ -589,7 +616,7 @@ int __must_check add_disk_fwnode(struct device *parent, struct gendisk *disk,
 		.fwnode = fwnode,
 	};
 
-	return __add_disk_fwnode(&data);
+	return retry_on_updating_nr_hwq(&data, __add_disk_fwnode);
 }
 EXPORT_SYMBOL_GPL(add_disk_fwnode);
 
@@ -671,7 +698,7 @@ void blk_mark_disk_dead(struct gendisk *disk)
 }
 EXPORT_SYMBOL_GPL(blk_mark_disk_dead);
 
-static void __del_gendisk(struct gendisk_data *data)
+static int __del_gendisk(struct gendisk_data *data)
 {
 	struct gendisk *disk = data->disk;
 	struct request_queue *q = disk->queue;
@@ -682,7 +709,7 @@ static void __del_gendisk(struct gendisk_data *data)
 	might_sleep();
 
 	if (WARN_ON_ONCE(!disk_live(disk) && !(disk->flags & GENHD_FL_HIDDEN)))
-		return;
+		return 0;
 
 	disk_del_events(disk);
 
@@ -764,6 +791,7 @@ static void __del_gendisk(struct gendisk_data *data)
 
 	if (start_drain)
 		blk_unfreeze_release_lock(q);
+	return 0;
 }
 EXPORT_SYMBOL(del_gendisk);
 
@@ -792,7 +820,7 @@ void del_gendisk(struct gendisk *disk)
 		.disk	= disk,
 	};
 
-	__del_gendisk(&data);
+	retry_on_updating_nr_hwq(&data, __del_gendisk);
 }
 
 /**
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 8eb9b3310167..afe76dcfaa3c 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -527,6 +527,11 @@ struct blk_mq_tag_set {
 	struct mutex		tag_list_lock;
 	struct list_head	tag_list;
 	struct srcu_struct	*srcu;
+
+	bool			updating_nr_hwq;
+	struct mutex		update_nr_hwq_lock;
+	struct srcu_struct	update_nr_hwq_srcu;
+	wait_queue_head_t	update_nr_hwq_wq;
 };
 
 /**
-- 
2.47.0

Re: [PATCH V2 07/20] block: prevent adding/deleting disk during updating nr_hw_queues

[Nilay Shroff]

On 4/18/25 10:06 PM, Ming Lei wrote:
> Both adding/deleting disk code are reader of `nr_hw_queues`, so we can't
> allow them in-progress when updating nr_hw_queues, kernel panic and
> kasan has been reported in [1].
> 
> Prevent adding/deleting disk during updating nr_hw_queues by setting
> set->updating_nr_hwq, and use SRCU to fail & retry to add/delete disk.
> 
> This way avoids lot of trouble.
> 
> Reported-by: Nilay Shroff <nilay@linux.ibm.com>
> Closes: https://lore.kernel.org/linux-block/a5896cdb-a59a-4a37-9f99-20522f5d2987@linux.ibm.com/
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
[...]
[...]
>  
> +static int retry_on_updating_nr_hwq(struct gendisk_data *data,
> +				    int (*cb)(struct gendisk_data *data))
> +{
> +	struct gendisk *disk = data->disk;
> +	struct blk_mq_tag_set *set;
> +
> +	if (!queue_is_mq(disk->queue))
> +		return cb(data);
> +
> +	set = disk->queue->tag_set;
> +	do {
> +		int idx, ret;
> +
> +		idx = srcu_read_lock(&set->update_nr_hwq_srcu);
> +		if (set->updating_nr_hwq) {
> +			srcu_read_unlock(&set->update_nr_hwq_srcu, idx);
> +			goto wait;
> +		}
> +		ret = cb(data);
> +		srcu_read_unlock(&set->update_nr_hwq_srcu, idx);
> +		return ret;
> + wait:
> +		wait_event_interruptible(set->update_nr_hwq_wq,
> +				!set->updating_nr_hwq);
> +	} while (true);
> +}
> +
Yes using srcu with wait event would fix this. However after looking through 
changes it seems to me that the current changes are now modeled as below:

- Writer(blk_mq_update_nr_hw_queues) must wait until all readers are done 
  (or prior SRCU read-side critical-section complete)
- Readers (add/del disk) must wait until a writer(blk_mq_update_nr_hw_queues)
  finishes

And IMO, this behavior is best modeled with a reader-writer lock, rather than 
(S)RCU. 

In my view, RCU is optimized for read-heavy workloads with:
- Non-blocking readers
- Deferred updates by writers
- Writers don’t block readers and readers don’t block writers.

So I believe the simpler choice shall be using reader-writer lock instead of srcu. 
With reader-writer lock we should be also able to get away with implementing wait 
queue for add/del disk. Moreover, the reader-writer lock shall also work well with 
the another reader elv_iosched_store. 

Thanks,
--Nilay 

Re: [PATCH V2 07/20] block: prevent adding/deleting disk during updating nr_hw_queues

[Ming Lei]

On Sat, Apr 19, 2025 at 04:50:15PM +0530, Nilay Shroff wrote:
> 
> 
> On 4/18/25 10:06 PM, Ming Lei wrote:
> > Both adding/deleting disk code are reader of `nr_hw_queues`, so we can't
> > allow them in-progress when updating nr_hw_queues, kernel panic and
> > kasan has been reported in [1].
> > 
> > Prevent adding/deleting disk during updating nr_hw_queues by setting
> > set->updating_nr_hwq, and use SRCU to fail & retry to add/delete disk.
> > 
> > This way avoids lot of trouble.
> > 
> > Reported-by: Nilay Shroff <nilay@linux.ibm.com>
> > Closes: https://lore.kernel.org/linux-block/a5896cdb-a59a-4a37-9f99-20522f5d2987@linux.ibm.com/
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> [...]
> [...]
> >  
> > +static int retry_on_updating_nr_hwq(struct gendisk_data *data,
> > +				    int (*cb)(struct gendisk_data *data))
> > +{
> > +	struct gendisk *disk = data->disk;
> > +	struct blk_mq_tag_set *set;
> > +
> > +	if (!queue_is_mq(disk->queue))
> > +		return cb(data);
> > +
> > +	set = disk->queue->tag_set;
> > +	do {
> > +		int idx, ret;
> > +
> > +		idx = srcu_read_lock(&set->update_nr_hwq_srcu);
> > +		if (set->updating_nr_hwq) {
> > +			srcu_read_unlock(&set->update_nr_hwq_srcu, idx);
> > +			goto wait;
> > +		}
> > +		ret = cb(data);
> > +		srcu_read_unlock(&set->update_nr_hwq_srcu, idx);
> > +		return ret;
> > + wait:
> > +		wait_event_interruptible(set->update_nr_hwq_wq,
> > +				!set->updating_nr_hwq);
> > +	} while (true);
> > +}
> > +
> Yes using srcu with wait event would fix this. However after looking through 
> changes it seems to me that the current changes are now modeled as below:
> 
> - Writer(blk_mq_update_nr_hw_queues) must wait until all readers are done 
>   (or prior SRCU read-side critical-section complete)
> - Readers (add/del disk) must wait until a writer(blk_mq_update_nr_hw_queues)
>   finishes
> 
> And IMO, this behavior is best modeled with a reader-writer lock, rather than 
> (S)RCU. 
> 
> In my view, RCU is optimized for read-heavy workloads with:
> - Non-blocking readers

srcu allows blocking reader

> - Deferred updates by writers
> - Writers don’t block readers and readers don’t block writers.
> 
> So I believe the simpler choice shall be using reader-writer lock instead of srcu. 
> With reader-writer lock we should be also able to get away with implementing wait 
> queue for add/del disk. Moreover, the reader-writer lock shall also work well with 
> the another reader elv_iosched_store. 

Basically I agree with you that rwsem(instead of rwlock) should match with
this case in theory, but I feel that rwsem is stronger than srcu from lock
viewpoint, and we will add new dependency if rwsem is held inside
->store(), such as the following splat.


[   93.214889] ======================================================
[   93.219162] scsi 10:0:0:0: Direct-Access     Linux    scsi_debug       0191 PQ: 0 ANSI: 7
[   93.219549] WARNING: possible circular locking dependency detected
[   93.219557] 6.15.0-rc2_ublk+ #536 Not tainted
[   93.219565] ------------------------------------------------------
[   93.219568] check/1690 is trying to acquire lock:
[   93.222730] sd 9:0:0:0: [sdc] Synchronizing SCSI cache
[   93.223139] ffff8882ad849598
[   93.224074] scsi 10:0:0:0: Power-on or device reset occurred
[   93.227331] scsi 9:0:0:0: Direct-Access     Linux    scsi_debug       0191 PQ: 0 ANSI: 7
[   93.227581]  (kn->active#103){++++}-{0:0}, at: __kernfs_remove+0x213/0x680
[   93.231428] scsi 9:0:0:0: Power-on or device reset occurred
[   93.231890]
               but task is already holding lock:
[   93.231897] ffff88828ef358a0 (&q->q_usage_counter(queue)#14
[   93.232621] sd 10:0:0:0: Attached scsi generic sg3 type 0
[   93.234640] sd 10:0:0:0: [sdc] 16384 512-byte logical blocks: (8.39 MB/8.00 MiB)
[   93.235975] ){++++}-{0:0}, at: del_gendisk+0x186/0x250
[   93.238123] sd 10:0:0:0: [sdc] Write Protect is off
[   93.239546]
               which lock already depends on the new lock.

[   93.239552]
               the existing dependency chain (in reverse order) is:
[   93.239556]
               -> #4 (&q->q_usage_counter(queue)
[   93.240114] sd 9:0:0:0: Attached scsi generic sg3 type 0
[   93.240963] sd 10:0:0:0: [sdc] Mode Sense: 00 00 00 00
[   93.241757] sd 9:0:0:0: [sde] 16384 512-byte logical blocks: (8.39 MB/8.00 MiB)
[   93.242387] #14){++++}-{0:0}:
[   93.242404]        blk_queue_enter+0x4c9/0x640
[   93.242916] sd 9:0:0:0: [sde] Write Protect is off
[   93.242948] sd 9:0:0:0: [sde] Mode Sense: 73 00 10 08
[   93.243028] sd 9:0:0:0: [sde] Asking for cache data failed
[   93.243057] sd 9:0:0:0: [sde] Assuming drive cache: write through
[   93.243120] sd 9:0:0:0: [sde] Unexpected: RSCS has been set and the permanent stream count is 0
[   93.243255] sd 9:0:0:0: [sde] Preferred minimum I/O size 512 bytes
[   93.243284] sd 9:0:0:0: [sde] Optimal transfer size 524288 bytes
[   93.243568] sd 10:0:0:0: [sdc] Asking for cache data failed
[   93.244771]        blk_mq_alloc_request+0x4ae/0x8d0
[   93.244786]        scsi_execute_cmd+0x196/0xcf0
[   93.246604] sd 10:0:0:0: [sdc] Assuming drive cache: write through
[   93.247875]        read_capacity_16+0x1cf/0xbf0
[   93.247891]        sd_revalidate_disk.isra.0+0x15c0/0x8f50
[   93.249239] sd 10:0:0:0: [sdc] Unexpected: RSCS has been set and the permanent stream count is 0
[   93.251084]        sd_probe+0x815/0xf20
[   93.251098]        really_probe+0x1e0/0x930
[   93.252235] sd 10:0:0:0: [sdc] Preferred minimum I/O size 512 bytes
[   93.253608]        __driver_probe_device+0x18c/0x3d0
[   93.253623]        driver_probe_device+0x4a/0x120
[   93.253633]        __device_attach_driver+0x162/0x270
[   93.253643]        bus_for_each_drv+0x114/0x1a0
[   93.255593] sd 10:0:0:0: [sdc] Optimal transfer size 524288 bytes
[   93.256874]        __device_attach_async_helper+0x19e/0x240
[   93.362362]        async_run_entry_fn+0x97/0x4f0
[   93.364343]        process_one_work+0x841/0x17a0
[   93.366439]        worker_thread+0x6e8/0x1270
[   93.369068]        kthread+0x388/0x750
[   93.371762]        ret_from_fork+0x31/0x70
[   93.374588]        ret_from_fork_asm+0x1a/0x30
[   93.377490]
               -> #3 (&q->limits_lock){+.+.}-{4:4}:
[   93.381214]        __mutex_lock+0x192/0x1960
[   93.383220]        get_sectorsize+0x234/0x5f0
[   93.384945]        sr_block_open+0x1a9/0x220
[   93.386895]        blkdev_get_whole+0x8f/0x200
[   93.388922]        bdev_open+0x223/0xc10
[   93.390776]        blkdev_open+0x1f3/0x350
[   93.392704]        do_dentry_open+0x491/0x1820
[   93.394676]        vfs_open+0x82/0x350
[   93.396370]        path_openat+0x1b0c/0x2b10
[   93.398150]        do_filp_open+0x1e7/0x430
[   93.400494]        do_sys_openat2+0xed/0x180
[   93.403129]        __x64_sys_openat+0x122/0x1e0
[   93.405846]        do_syscall_64+0x95/0x180
[   93.408486]        entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   93.411165]
               -> #2 (&disk->open_mutex){+.+.}-{4:4}:
[   93.414296]        __mutex_lock+0x192/0x1960
[   93.415744]        bdev_open+0x339/0xc10
[   93.417148]        bdev_file_open_by_dev+0xc6/0x140
[   93.418669]        disk_scan_partitions+0x190/0x280
[   93.420188]        __add_disk_fwnode+0xdfe/0x1140
[   93.421708]        device_add_disk+0x184/0x270
[   93.423162]        nvme_alloc_ns+0x16f5/0x3580
[   93.424625]        nvme_scan_ns+0x628/0x7f0
[   93.425974]        async_run_entry_fn+0x97/0x4f0
[   93.427424]        process_one_work+0x841/0x17a0
[   93.428876]        worker_thread+0x6e8/0x1270
[   93.430336]        kthread+0x388/0x750
[   93.431760]        ret_from_fork+0x31/0x70
[   93.433186]        ret_from_fork_asm+0x1a/0x30
[   93.434667]
               -> #1 (&set->update_nr_hwq_lock){.+.+}-{4:4}:
[   93.437156]        down_read_interruptible+0x92/0x490
[   93.438774]        elevator_change+0x1a8/0x210
[   93.440281]        elv_iosched_store+0x14d/0x1f0
[   93.441928]        queue_attr_store+0x235/0x2f0
[   93.443469]        kernfs_fop_write_iter+0x359/0x530
[   93.445074]        vfs_write+0x5fb/0x1130
[   93.446440]        ksys_write+0xf9/0x1c0
[   93.447714]        do_syscall_64+0x95/0x180
[   93.449042]        entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   93.450585]
               -> #0 (kn->active#103){++++}-{0:0}:
[   93.452761]        __lock_acquire+0x1459/0x2200
[   93.454081]        lock_acquire+0x165/0x310
[   93.455378]        kernfs_drain+0x3a8/0x460
[   93.456736]        __kernfs_remove+0x213/0x680
[   93.458162]        kernfs_remove_by_name_ns+0x9c/0x110
[   93.459707]        remove_files+0x8c/0x1a0
[   93.461066]        sysfs_remove_group+0x7b/0x160
[   93.462494]        sysfs_remove_groups+0x53/0xa0
[   93.463916]        __kobject_del+0x7d/0x1d0
[   93.465242]        kobject_del+0x35/0x50
[   93.466568]        blk_unregister_queue+0x154/0x2c0
[   93.468022]        __del_gendisk+0x4d1/0xa10
[   93.469391]        del_gendisk+0x186/0x250
[   93.470757]        sd_remove+0x89/0x130
[   93.472093]        device_release_driver_internal+0x379/0x540
[   93.473726]        bus_remove_device+0x1f5/0x3f0
[   93.475198]        device_del+0x33c/0x8d0
[   93.476458]        __scsi_remove_device+0x276/0x340
[   93.477877]        sdev_store_delete+0x87/0x120
[   93.479288]        kernfs_fop_write_iter+0x359/0x530
[   93.480710]        vfs_write+0x5fb/0x1130
[   93.481986]        ksys_write+0xf9/0x1c0
[   93.483267]        do_syscall_64+0x95/0x180
[   93.484595]        entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   93.486120]
               other info that might help us debug this:

[   93.489456] Chain exists of:
                 kn->active#103 --> &q->limits_lock --> &q->q_usage_counter(queue)#14

[   93.493238]  Possible unsafe locking scenario:

[   93.495533]        CPU0                    CPU1
[   93.496993]        ----                    ----
[   93.498399]   lock(&q->q_usage_counter(queue)#14);
[   93.499859]                                lock(&q->limits_lock);
[   93.501504]                                lock(&q->q_usage_counter(queue)#14);
[   93.503463]   lock(kn->active#103);
[   93.504830]
                *** DEADLOCK ***

[   93.507934] 6 locks held by check/1690:
[   93.509293]  #0: ffff88811032c420 (sb_writers#4){.+.+}-{0:0}, at: ksys_write+0xf9/0x1c0
[   93.511216]  #1: ffff888115cbb088 (&of->mutex#2){+.+.}-{4:4}, at: kernfs_fop_write_iter+0x21d/0x530
[   93.513380]  #2: ffff888126a140e0 (&shost->scan_mutex){+.+.}-{4:4}, at: sdev_store_delete+0x7f/0x120
[   93.515654]  #3: ffff8882a1b4e380 (&dev->mutex){....}-{4:4}, at: device_release_driver_internal+0x91/0x540
[   93.517859]  #4: ffff888126a14388 (&set->update_nr_hwq_lock){.+.+}-{4:4}, at: del_gendisk+0x17c/0x250
[   93.520267]  #5: ffff88828ef358a0 (&q->q_usage_counter(queue)#14){++++}-{0:0}, at: del_gendisk+0x186/0x250
[   93.522559]
               stack backtrace:
[   93.524777] CPU: 2 UID: 0 PID: 1690 Comm: check Not tainted 6.15.0-rc2_ublk+ #536 PREEMPT(voluntary)
[   93.524785] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-1.fc39 04/01/2014
[   93.524789] Call Trace:
[   93.524794]  <TASK>
[   93.524799]  dump_stack_lvl+0x93/0xf0
[   93.524810]  print_circular_bug+0x275/0x340
[   93.524820]  check_noncircular+0x146/0x160
[   93.524831]  __lock_acquire+0x1459/0x2200
[   93.524842]  lock_acquire+0x165/0x310
[   93.524849]  ? __kernfs_remove+0x213/0x680
[   93.524857]  ? up_write+0x19e/0x500
[   93.524864]  kernfs_drain+0x3a8/0x460
[   93.524870]  ? __kernfs_remove+0x213/0x680
[   93.524878]  ? __pfx_kernfs_drain+0x10/0x10
[   93.524884]  ? find_held_lock+0x2b/0x80
[   93.524889]  ? kernfs_root+0xb0/0x1c0
[   93.524900]  __kernfs_remove+0x213/0x680
[   93.524906]  ? kernfs_find_ns+0x192/0x390
[   93.524914]  kernfs_remove_by_name_ns+0x9c/0x110
[   93.524923]  remove_files+0x8c/0x1a0
[   93.524930]  sysfs_remove_group+0x7b/0x160
[   93.524937]  sysfs_remove_groups+0x53/0xa0
[   93.524945]  __kobject_del+0x7d/0x1d0
[   93.524953]  kobject_del+0x35/0x50
[   93.524960]  blk_unregister_queue+0x154/0x2c0
[   93.524968]  __del_gendisk+0x4d1/0xa10
[   93.524978]  ? __pfx___del_gendisk+0x10/0x10
[   93.524986]  ? kobject_put+0x61/0x4b0
[   93.524995]  del_gendisk+0x186/0x250
[   93.525019]  ? __pfx_del_gendisk+0x10/0x10
[   93.525029]  ? __pm_runtime_resume+0x79/0x100
[   93.525048]  sd_remove+0x89/0x130
[   93.525059]  device_release_driver_internal+0x379/0x540
[   93.525068]  ? kobject_put+0x61/0x4b0
[   93.525076]  bus_remove_device+0x1f5/0x3f0
[   93.525085]  device_del+0x33c/0x8d0
[   93.525093]  ? attribute_container_device_trigger+0x183/0x1f0
[   93.525099]  ? __pfx_device_del+0x10/0x10
[   93.525106]  ? __pfx_attribute_container_device_trigger+0x10/0x10
[   93.525115]  __scsi_remove_device+0x276/0x340
[   93.525122]  sdev_store_delete+0x87/0x120
[   93.525128]  kernfs_fop_write_iter+0x359/0x530
[   93.525138]  vfs_write+0x5fb/0x1130
[   93.525147]  ? __pfx_vfs_write+0x10/0x10
[   93.525158]  ? lock_acquire+0x165/0x310
[   93.525165]  ? nohz_balance_exit_idle.part.0+0xe2/0x330
[   93.525175]  ksys_write+0xf9/0x1c0
[   93.525182]  ? __pfx_ksys_write+0x10/0x10
[   93.525188]  ? __pfx_run_posix_cpu_timers+0x10/0x10
[   93.525198]  do_syscall_64+0x95/0x180
[   93.525206]  ? __lock_acquire+0x432/0x2200
[   93.525219]  ? __lock_acquire+0x432/0x2200
[   93.525229]  ? lock_acquire+0x165/0x310
[   93.525236]  ? ktime_get+0x32/0x150
[   93.525243]  ? find_held_lock+0x2b/0x80
[   93.525248]  ? ktime_get+0x32/0x150
[   93.525257]  ? seqcount_lockdep_reader_access.constprop.0+0x4b/0xb0
[   93.525264]  ? kvm_sched_clock_read+0x11/0x20
[   93.525270]  ? sched_clock+0x10/0x30
[   93.525277]  ? sched_clock_cpu+0x6c/0x4c0
[   93.525286]  ? __pfx_sched_clock_cpu+0x10/0x10
[   93.525293]  ? hrtimer_interrupt+0x363/0x870
[   93.525301]  ? irqtime_account_irq+0x51/0x200
[   93.525310]  ? irqentry_exit_to_user_mode+0x90/0x280
[   93.525318]  ? clear_bhb_loop+0x35/0x90
[   93.525324]  ? clear_bhb_loop+0x35/0x90
[   93.525329]  ? clear_bhb_loop+0x35/0x90
[   93.525335]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   93.525341] RIP: 0033:0x7f6a158fb174
[   93.525364] Code: 89 02 48 c7 c0 ff ff ff ff eb bd 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 80 3d 6d b4 0d 00 00 74 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 55 48 89 e5 48 83 ec 20 48 89
[   93.525369] RSP: 002b:00007fff65c97158 EFLAGS: 00000202 ORIG_RAX: 0000000000000001
[   93.525376] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f6a158fb174
[   93.525381] RDX: 0000000000000002 RSI: 0000563b4b680130 RDI: 0000000000000001
[   93.525384] RBP: 00007fff65c97180 R08: 0000000000000073 R09: 0000000000000001
[   93.525387] R10: 0000000000000000 R11: 0000000000000202 R12: 0000000000000002
[   93.525395] R13: 0000563b4b680130 R14: 00007f6a159cf780 R15: 0000000000000002
[   93.525407]  </TASK>
[   93.657609] sd 11:0:0:0: [sdd] Synchronizing SCSI cache



Thanks,
Ming

Re: [PATCH V2 07/20] block: prevent adding/deleting disk during updating nr_hw_queues

[Christoph Hellwig]

On Mon, Apr 21, 2025 at 10:39:24AM +0800, Ming Lei wrote:
> > In my view, RCU is optimized for read-heavy workloads with:
> > - Non-blocking readers
> 
> srcu allows blocking reader

It does.  But it's certainly not optimized for long blocking readers.

> Basically I agree with you that rwsem(instead of rwlock) should match with
> this case in theory, but I feel that rwsem is stronger than srcu from lock
> viewpoint, and we will add new dependency if rwsem is held inside
> ->store(), such as the following splat.

How does manually implementing a reader/write lock using SRCU avoid
that dependency vs just hiding it?

I'd rather sort this out as a rwsem is very natural her as Nilay pointed
out, and also avoids the whole giv up and retry pattern.

Re: [PATCH V2 07/20] block: prevent adding/deleting disk during updating nr_hw_queues

[Ming Lei]

On Tue, Apr 22, 2025 at 09:04:32AM +0200, Christoph Hellwig wrote:
> On Mon, Apr 21, 2025 at 10:39:24AM +0800, Ming Lei wrote:
> > > In my view, RCU is optimized for read-heavy workloads with:
> > > - Non-blocking readers
> > 
> > srcu allows blocking reader
> 
> It does.  But it's certainly not optimized for long blocking readers.
> 
> > Basically I agree with you that rwsem(instead of rwlock) should match with
> > this case in theory, but I feel that rwsem is stronger than srcu from lock
> > viewpoint, and we will add new dependency if rwsem is held inside
> > ->store(), such as the following splat.
> 
> How does manually implementing a reader/write lock using SRCU avoid
> that dependency vs just hiding it?
> 
> I'd rather sort this out as a rwsem is very natural her as Nilay pointed
> out, and also avoids the whole giv up and retry pattern.
 
Thinking of further, the warning triggered from rwsem is false positive,
because elevator switch can't happen until disk is added, and the splat
can be avoided by switching to down_read_nested() in elevator_change(),
which need to be nested too.

So looks fine to use rwsem.


Thanks,
Ming

Re: [PATCH V2 07/20] block: prevent adding/deleting disk during updating nr_hw_queues

[Nilay Shroff]

[-- Attachment #1: Type: text/plain, Size: 1857 bytes --]



On 4/22/25 2:59 PM, Ming Lei wrote:
> On Tue, Apr 22, 2025 at 09:04:32AM +0200, Christoph Hellwig wrote:
>> On Mon, Apr 21, 2025 at 10:39:24AM +0800, Ming Lei wrote:
>>>> In my view, RCU is optimized for read-heavy workloads with:
>>>> - Non-blocking readers
>>>
>>> srcu allows blocking reader
>>
>> It does.  But it's certainly not optimized for long blocking readers.
>>
>>> Basically I agree with you that rwsem(instead of rwlock) should match with
>>> this case in theory, but I feel that rwsem is stronger than srcu from lock
>>> viewpoint, and we will add new dependency if rwsem is held inside
>>> ->store(), such as the following splat.
>>
>> How does manually implementing a reader/write lock using SRCU avoid
>> that dependency vs just hiding it?
>>
>> I'd rather sort this out as a rwsem is very natural her as Nilay pointed
>> out, and also avoids the whole giv up and retry pattern.
>  
> Thinking of further, the warning triggered from rwsem is false positive,
> because elevator switch can't happen until disk is added, and the splat
> can be avoided by switching to down_read_nested() in elevator_change(),
> which need to be nested too.
> 
> So looks fine to use rwsem.
> 
Instead of using down_read_nested() why can't we move the reader-lock 
inside elv_iosched_store() ?

With your patchset, now elevator_change() is called from three places:
- elevator_set_default
- elevator_set_none
- elv_iosched_store

As both elevator_set_default and elevator_set_none should run holding
reader-lock, I think we should remove reader-lock from  elevator_change and 
move it inside elv_iosched_store.

In fact, I just quickly prototyped rwsem replacing srcu on top of your 
changes and ran blktests. The rwsem changes sustain blktests and I didn't
encounter any lockdep splat. For reference, attached my (quick) prototype
changes.

Thanks,
--Nilay

[-- Attachment #2: tag-set-rwsem.diff --]
[-- Type: text/x-patch, Size: 4503 bytes --]

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 5c9f7391af92..bbe3dd40f1e8 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4773,18 +4773,14 @@ int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set)
 			goto out_free_srcu;
 	}
 
-	mutex_init(&set->update_nr_hwq_lock);
-	init_waitqueue_head(&set->update_nr_hwq_wq);
-	ret = init_srcu_struct(&set->update_nr_hwq_srcu);
-	if (ret)
-		goto out_cleanup_srcu;
+	init_rwsem(&set->update_nr_hwq_lock);
 
 	ret = -ENOMEM;
 	set->tags = kcalloc_node(set->nr_hw_queues,
 				 sizeof(struct blk_mq_tags *), GFP_KERNEL,
 				 set->numa_node);
 	if (!set->tags)
-		goto out_cleanup_hwq_srcu;
+		goto out_cleanup_srcu;
 
 	for (i = 0; i < set->nr_maps; i++) {
 		set->map[i].mq_map = kcalloc_node(nr_cpu_ids,
@@ -4813,8 +4809,6 @@ int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set)
 	}
 	kfree(set->tags);
 	set->tags = NULL;
-out_cleanup_hwq_srcu:
-	cleanup_srcu_struct(&set->update_nr_hwq_srcu);
 out_cleanup_srcu:
 	if (set->flags & BLK_MQ_F_BLOCKING)
 		cleanup_srcu_struct(set->srcu);
@@ -5007,21 +5001,16 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 
 void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int nr_hw_queues)
 {
-	mutex_lock(&set->update_nr_hwq_lock);
+	down_write(&set->update_nr_hwq_lock);
 	/*
 	 * Mark us in updating nr_hw_queues for preventing reader of
 	 * nr_hw_queues, such as adding/deleting disk.
 	 */
-	set->updating_nr_hwq = true;
-	synchronize_srcu(&set->update_nr_hwq_srcu);
-
 	mutex_lock(&set->tag_list_lock);
 	__blk_mq_update_nr_hw_queues(set, nr_hw_queues);
 	mutex_unlock(&set->tag_list_lock);
 
-	set->updating_nr_hwq = false;
-	wake_up_all(&set->update_nr_hwq_wq);
-	mutex_unlock(&set->update_nr_hwq_lock);
+	up_write(&set->update_nr_hwq_lock);
 }
 EXPORT_SYMBOL_GPL(blk_mq_update_nr_hw_queues);
 
diff --git a/block/elevator.c b/block/elevator.c
index 378553fce5d8..69193d04fffa 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -743,15 +743,8 @@ int __elevator_change(struct request_queue *q, struct elv_change_ctx *ctx)
 static int elevator_change(struct request_queue *q,
 			   struct elv_change_ctx *ctx)
 {
-	struct blk_mq_tag_set *set = q->tag_set;
 	unsigned int memflags;
-	int ret, idx;
-
-	idx = srcu_read_lock(&set->update_nr_hwq_srcu);
-	if (set->updating_nr_hwq) {
-		ret = -EBUSY;
-		goto exit;
-	}
+	int ret;
 
 	memflags = blk_mq_freeze_queue(q);
 	/*
@@ -771,8 +764,6 @@ static int elevator_change(struct request_queue *q,
 	if (!ret)
 		ret = elevator_change_done(q, ctx);
 
-exit:
-	srcu_read_unlock(&set->update_nr_hwq_srcu, idx);
 	return ret;
 }
 
@@ -792,6 +783,7 @@ ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
 			  size_t count)
 {
 	struct request_queue *q = disk->queue;
+	struct blk_mq_tag_set *set = q->tag_set;
 	char elevator_name[ELV_NAME_MAX];
 	struct elv_change_ctx ctx = {
 		.uevent = true,
@@ -808,9 +800,12 @@ ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
 
 	elv_iosched_load_module(ctx.name);
 
+	down_read(&set->update_nr_hwq_lock);
 	ret = elevator_change(q, &ctx);
 	if (!ret)
 		ret = count;
+	up_read(&set->update_nr_hwq_lock);
+
 	return ret;
 }
 
diff --git a/block/genhd.c b/block/genhd.c
index de227aa923ed..359a55bf47ce 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -407,19 +407,12 @@ static int retry_on_updating_nr_hwq(struct gendisk_data *data,
 
 	set = disk->queue->tag_set;
 	do {
-		int idx, ret;
+		int ret;
 
-		idx = srcu_read_lock(&set->update_nr_hwq_srcu);
-		if (set->updating_nr_hwq) {
-			srcu_read_unlock(&set->update_nr_hwq_srcu, idx);
-			goto wait;
-		}
+		down_read(&set->update_nr_hwq_lock);
 		ret = cb(data);
-		srcu_read_unlock(&set->update_nr_hwq_srcu, idx);
+		up_read(&set->update_nr_hwq_lock);
 		return ret;
- wait:
-		wait_event_interruptible(set->update_nr_hwq_wq,
-				!set->updating_nr_hwq);
 	} while (true);
 }
 
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index afe76dcfaa3c..ef84d53095a6 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -9,6 +9,7 @@
 #include <linux/prefetch.h>
 #include <linux/srcu.h>
 #include <linux/rw_hint.h>
+#include <linux/rwsem.h>
 
 struct blk_mq_tags;
 struct blk_flush_queue;
@@ -528,10 +529,7 @@ struct blk_mq_tag_set {
 	struct list_head	tag_list;
 	struct srcu_struct	*srcu;
 
-	bool			updating_nr_hwq;
-	struct mutex		update_nr_hwq_lock;
-	struct srcu_struct	update_nr_hwq_srcu;
-	wait_queue_head_t	update_nr_hwq_wq;
+	struct rw_semaphore	update_nr_hwq_lock;
 };
 
 /**

[PATCH V2 08/20] block: don't allow to switch elevator if updating nr_hw_queues is in-progress

[Ming Lei]

Elevator switch code is another `nr_hw_queue` reader in non-fast-IO code
path, so it can't be done if updating `nr_hw_queues` is in-progress.

Take same approach with not allowing add/del disk when updating
nr_hw_queues is in-progress, by holding srcu lock & check
set->updating_nr_hwq.

Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Closes: https://lore.kernel.org/linux-block/mz4t4tlwiqjijw3zvqnjb7ovvvaegkqganegmmlc567tt5xj67@xal5ro544cnc/
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/elevator.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/block/elevator.c b/block/elevator.c
index d25b9cc6c509..c23912652f96 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -720,9 +720,10 @@ ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
 {
 	char elevator_name[ELV_NAME_MAX];
 	char *name;
-	int ret;
+	int ret, idx;
 	unsigned int memflags;
 	struct request_queue *q = disk->queue;
+	struct blk_mq_tag_set *set = q->tag_set;
 
 	/*
 	 * If the attribute needs to load a module, do it before freezing the
@@ -734,6 +735,12 @@ ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
 
 	elv_iosched_load_module(name);
 
+	idx = srcu_read_lock(&set->update_nr_hwq_srcu);
+	if (set->updating_nr_hwq) {
+		ret = -EBUSY;
+		goto exit;
+	}
+
 	memflags = blk_mq_freeze_queue(q);
 	mutex_lock(&q->elevator_lock);
 	ret = elevator_change(q, name);
@@ -741,6 +748,8 @@ ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
 		ret = count;
 	mutex_unlock(&q->elevator_lock);
 	blk_mq_unfreeze_queue(q, memflags);
+exit:
+	srcu_read_unlock(&set->update_nr_hwq_srcu, idx);
 	return ret;
 }
 
-- 
2.47.0

Re: [PATCH V2 08/20] block: don't allow to switch elevator if updating nr_hw_queues is in-progress

[Nilay Shroff]

On 4/18/25 10:06 PM, Ming Lei wrote:
> Elevator switch code is another `nr_hw_queue` reader in non-fast-IO code
> path, so it can't be done if updating `nr_hw_queues` is in-progress.
> 
> Take same approach with not allowing add/del disk when updating
> nr_hw_queues is in-progress, by holding srcu lock & check
> set->updating_nr_hwq.
> 
> Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Closes: https://lore.kernel.org/linux-block/mz4t4tlwiqjijw3zvqnjb7ovvvaegkqganegmmlc567tt5xj67@xal5ro544cnc/
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/elevator.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/block/elevator.c b/block/elevator.c
> index d25b9cc6c509..c23912652f96 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -720,9 +720,10 @@ ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
>  {
>  	char elevator_name[ELV_NAME_MAX];
>  	char *name;
> -	int ret;
> +	int ret, idx;
>  	unsigned int memflags;
>  	struct request_queue *q = disk->queue;
> +	struct blk_mq_tag_set *set = q->tag_set;
>  
>  	/*
>  	 * If the attribute needs to load a module, do it before freezing the
> @@ -734,6 +735,12 @@ ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
>  
>  	elv_iosched_load_module(name);
>  
> +	idx = srcu_read_lock(&set->update_nr_hwq_srcu);
> +	if (set->updating_nr_hwq) {
> +		ret = -EBUSY;
> +		goto exit;
> +	}
> +
>  	memflags = blk_mq_freeze_queue(q);
>  	mutex_lock(&q->elevator_lock);
>  	ret = elevator_change(q, name);
> @@ -741,6 +748,8 @@ ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
>  		ret = count;
>  	mutex_unlock(&q->elevator_lock);
>  	blk_mq_unfreeze_queue(q, memflags);
> +exit:
> +	srcu_read_unlock(&set->update_nr_hwq_srcu, idx);
>  	return ret;
>  }
>  

I have same comment here as well as I mentioned in the previous patch.
(i.e. about using reader-writer lock instead of srcu).

Thanks,
--Nilay

[PATCH V2 09/20] block: simplify elevator rebuild for updating nr_hw_queues

[Ming Lei]

In blk_mq_update_nr_hw_queues(), nr_hw_queues changes and elevator data
depends on it, so elevator has to be rebuilt.

Now elevator switch isn't allowed during blk_mq_update_nr_hw_queues(), so
we can simply call elevator_change() to rebuild elevator sched tags after
nr_hw_queues is updated.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c   | 103 ++++++-----------------------------------------
 block/blk.h      |   4 +-
 block/elevator.c |  12 +++---
 3 files changed, 22 insertions(+), 97 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index e1662617cc7a..0f4a5e674874 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4929,88 +4929,10 @@ int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr)
 	return ret;
 }
 
-/*
- * request_queue and elevator_type pair.
- * It is just used by __blk_mq_update_nr_hw_queues to cache
- * the elevator_type associated with a request_queue.
- */
-struct blk_mq_qe_pair {
-	struct list_head node;
-	struct request_queue *q;
-	struct elevator_type *type;
-};
-
-/*
- * Cache the elevator_type in qe pair list and switch the
- * io scheduler to 'none'
- */
-static bool blk_mq_elv_switch_none(struct list_head *head,
-		struct request_queue *q)
-{
-	struct blk_mq_qe_pair *qe;
-
-	qe = kmalloc(sizeof(*qe), GFP_NOIO | __GFP_NOWARN | __GFP_NORETRY);
-	if (!qe)
-		return false;
-
-	/* Accessing q->elevator needs protection from ->elevator_lock. */
-	mutex_lock(&q->elevator_lock);
-
-	if (!q->elevator) {
-		kfree(qe);
-		goto unlock;
-	}
-
-	INIT_LIST_HEAD(&qe->node);
-	qe->q = q;
-	qe->type = q->elevator->type;
-	/* keep a reference to the elevator module as we'll switch back */
-	__elevator_get(qe->type);
-	list_add(&qe->node, head);
-	elevator_disable(q);
-unlock:
-	mutex_unlock(&q->elevator_lock);
-
-	return true;
-}
-
-static struct blk_mq_qe_pair *blk_lookup_qe_pair(struct list_head *head,
-						struct request_queue *q)
-{
-	struct blk_mq_qe_pair *qe;
-
-	list_for_each_entry(qe, head, node)
-		if (qe->q == q)
-			return qe;
-
-	return NULL;
-}
-
-static void blk_mq_elv_switch_back(struct list_head *head,
-				  struct request_queue *q)
-{
-	struct blk_mq_qe_pair *qe;
-	struct elevator_type *t;
-
-	qe = blk_lookup_qe_pair(head, q);
-	if (!qe)
-		return;
-	t = qe->type;
-	list_del(&qe->node);
-	kfree(qe);
-
-	mutex_lock(&q->elevator_lock);
-	elevator_switch(q, t);
-	/* drop the reference acquired in blk_mq_elv_switch_none */
-	elevator_put(t);
-	mutex_unlock(&q->elevator_lock);
-}
-
 static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 							int nr_hw_queues)
 {
 	struct request_queue *q;
-	LIST_HEAD(head);
 	int prev_nr_hw_queues = set->nr_hw_queues;
 	unsigned int memflags;
 	int i;
@@ -5028,15 +4950,6 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 	list_for_each_entry(q, &set->tag_list, tag_set_list)
 		blk_mq_freeze_queue_nomemsave(q);
 
-	/*
-	 * Switch IO scheduler to 'none', cleaning up the data associated
-	 * with the previous scheduler. We will switch back once we are done
-	 * updating the new sw to hw queue mappings.
-	 */
-	list_for_each_entry(q, &set->tag_list, tag_set_list)
-		if (!blk_mq_elv_switch_none(&head, q))
-			goto switch_back;
-
 	list_for_each_entry(q, &set->tag_list, tag_set_list) {
 		blk_mq_debugfs_unregister_hctxs(q);
 		blk_mq_sysfs_unregister_hctxs(q);
@@ -5070,9 +4983,19 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 		blk_mq_debugfs_register_hctxs(q);
 	}
 
-switch_back:
-	list_for_each_entry(q, &set->tag_list, tag_set_list)
-		blk_mq_elv_switch_back(&head, q);
+	list_for_each_entry(q, &set->tag_list, tag_set_list) {
+		const char *name = "none";
+
+		mutex_lock(&q->elevator_lock);
+		if (q->elevator && !blk_queue_dying(q))
+			name = q->elevator->type->elevator_name;
+		/*
+		 * nr_hw_queues is changed and elevator data depends on
+		 * it, so we have to force to rebuild elevator
+		 */
+		__elevator_change(q, name, true);
+		mutex_unlock(&q->elevator_lock);
+	}
 
 	list_for_each_entry(q, &set->tag_list, tag_set_list)
 		blk_mq_unfreeze_queue_nomemrestore(q);
diff --git a/block/blk.h b/block/blk.h
index 006e3be433d2..0c3cc1af2525 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -319,8 +319,8 @@ bool blk_bio_list_merge(struct request_queue *q, struct list_head *list,
 
 bool blk_insert_flush(struct request *rq);
 
-int elevator_switch(struct request_queue *q, struct elevator_type *new_e);
-void elevator_disable(struct request_queue *q);
+int __elevator_change(struct request_queue *q, const char *elevator_name,
+		      bool force);
 void elevator_exit(struct request_queue *q);
 int elv_register_queue(struct request_queue *q, bool uevent);
 void elv_unregister_queue(struct request_queue *q);
diff --git a/block/elevator.c b/block/elevator.c
index c23912652f96..f4c02a6c045d 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -621,7 +621,7 @@ void elevator_init_mq(struct request_queue *q)
  * If switching fails, we are most likely running out of memory and not able
  * to restore the old io scheduler, so leaving the io scheduler being none.
  */
-int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
+static int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
 {
 	int ret;
 
@@ -657,7 +657,7 @@ int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
 	return ret;
 }
 
-void elevator_disable(struct request_queue *q)
+static void elevator_disable(struct request_queue *q)
 {
 	WARN_ON_ONCE(q->mq_freeze_depth == 0);
 	lockdep_assert_held(&q->elevator_lock);
@@ -677,7 +677,8 @@ void elevator_disable(struct request_queue *q)
 /*
  * Switch this queue to the given IO scheduler.
  */
-static int elevator_change(struct request_queue *q, const char *elevator_name)
+int __elevator_change(struct request_queue *q, const char *elevator_name,
+		      bool force)
 {
 	struct elevator_type *e;
 	int ret;
@@ -692,7 +693,8 @@ static int elevator_change(struct request_queue *q, const char *elevator_name)
 		return 0;
 	}
 
-	if (q->elevator && elevator_match(q->elevator->type, elevator_name))
+	if (!force && q->elevator &&
+	    elevator_match(q->elevator->type, elevator_name))
 		return 0;
 
 	e = elevator_find_get(elevator_name);
@@ -743,7 +745,7 @@ ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
 
 	memflags = blk_mq_freeze_queue(q);
 	mutex_lock(&q->elevator_lock);
-	ret = elevator_change(q, name);
+	ret = __elevator_change(q, name, false);
 	if (!ret)
 		ret = count;
 	mutex_unlock(&q->elevator_lock);
-- 
2.47.0

Re: [PATCH V2 09/20] block: simplify elevator rebuild for updating nr_hw_queues

[Nilay Shroff]

On 4/18/25 10:06 PM, Ming Lei wrote:
> In blk_mq_update_nr_hw_queues(), nr_hw_queues changes and elevator data
> depends on it, so elevator has to be rebuilt.
> 
> Now elevator switch isn't allowed during blk_mq_update_nr_hw_queues(), so
> we can simply call elevator_change() to rebuild elevator sched tags after
> nr_hw_queues is updated.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Looks good to me:
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

[PATCH V2 10/20] block: add helper of elevator_change()

[Ming Lei]

Add elevator_change() to simplify elv_iosched_store() a bit, and the new
helper will be used for unifying all scheduler change.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/elevator.c | 44 +++++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index f4c02a6c045d..6bf3871c7164 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -53,6 +53,8 @@ static LIST_HEAD(elv_list);
  */
 #define rq_hash_key(rq)		(blk_rq_pos(rq) + blk_rq_sectors(rq))
 
+static int elevator_change(struct request_queue *q, const char *name);
+
 /*
  * Query io scheduler to see if the current process issuing bio may be
  * merged with rq.
@@ -705,6 +707,28 @@ int __elevator_change(struct request_queue *q, const char *elevator_name,
 	return ret;
 }
 
+static int elevator_change(struct request_queue *q, const char *name)
+{
+	struct blk_mq_tag_set *set = q->tag_set;
+	unsigned int memflags;
+	int ret, idx;
+
+	idx = srcu_read_lock(&set->update_nr_hwq_srcu);
+	if (set->updating_nr_hwq) {
+		ret = -EBUSY;
+		goto exit;
+	}
+
+	memflags = blk_mq_freeze_queue(q);
+	mutex_lock(&q->elevator_lock);
+	ret = __elevator_change(q, name, false);
+	mutex_unlock(&q->elevator_lock);
+	blk_mq_unfreeze_queue(q, memflags);
+exit:
+	srcu_read_unlock(&set->update_nr_hwq_srcu, idx);
+	return ret;
+}
+
 static void elv_iosched_load_module(char *elevator_name)
 {
 	struct elevator_type *found;
@@ -720,12 +744,10 @@ static void elv_iosched_load_module(char *elevator_name)
 ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
 			  size_t count)
 {
+	struct request_queue *q = disk->queue;
 	char elevator_name[ELV_NAME_MAX];
 	char *name;
-	int ret, idx;
-	unsigned int memflags;
-	struct request_queue *q = disk->queue;
-	struct blk_mq_tag_set *set = q->tag_set;
+	int ret;
 
 	/*
 	 * If the attribute needs to load a module, do it before freezing the
@@ -737,21 +759,9 @@ ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
 
 	elv_iosched_load_module(name);
 
-	idx = srcu_read_lock(&set->update_nr_hwq_srcu);
-	if (set->updating_nr_hwq) {
-		ret = -EBUSY;
-		goto exit;
-	}
-
-	memflags = blk_mq_freeze_queue(q);
-	mutex_lock(&q->elevator_lock);
-	ret = __elevator_change(q, name, false);
+	ret = elevator_change(q, name);
 	if (!ret)
 		ret = count;
-	mutex_unlock(&q->elevator_lock);
-	blk_mq_unfreeze_queue(q, memflags);
-exit:
-	srcu_read_unlock(&set->update_nr_hwq_srcu, idx);
 	return ret;
 }
 
-- 
2.47.0

[PATCH V2 11/20] block: move blk_unregister_queue() & device_del() after freeze wait

[Ming Lei]

Move blk_unregister_queue() & device_del() after freeze wait, and prepare
for unifying elevator switch.

This way is just fine, since bdev has been unhashed at the beginning of
del_gendisk(), both blk_unregister_queue() & device_del() are dealing
with kobject & debugfs thing only.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/genhd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index d22fdc0d5383..86c3db5b9305 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -749,8 +749,6 @@ static int __del_gendisk(struct gendisk_data *data)
 		bdi_unregister(disk->bdi);
 	}
 
-	blk_unregister_queue(disk);
-
 	kobject_put(disk->part0->bd_holder_dir);
 	kobject_put(disk->slave_dir);
 	disk->slave_dir = NULL;
@@ -759,10 +757,12 @@ static int __del_gendisk(struct gendisk_data *data)
 	disk->part0->bd_stamp = 0;
 	sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
 	pm_runtime_set_memalloc_noio(disk_to_dev(disk), false);
-	device_del(disk_to_dev(disk));
 
 	blk_mq_freeze_queue_wait(q);
 
+	blk_unregister_queue(disk);
+	device_del(disk_to_dev(disk));
+
 	blk_throtl_cancel_bios(disk);
 
 	blk_sync_queue(q);
-- 
2.47.0

Re: [PATCH V2 11/20] block: move blk_unregister_queue() & device_del() after freeze wait

[Nilay Shroff]

On 4/18/25 10:06 PM, Ming Lei wrote:
> Move blk_unregister_queue() & device_del() after freeze wait, and prepare
> for unifying elevator switch.
> 
> This way is just fine, since bdev has been unhashed at the beginning of
> del_gendisk(), both blk_unregister_queue() & device_del() are dealing
> with kobject & debugfs thing only.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/genhd.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index d22fdc0d5383..86c3db5b9305 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -749,8 +749,6 @@ static int __del_gendisk(struct gendisk_data *data)
>  		bdi_unregister(disk->bdi);
>  	}
>  
> -	blk_unregister_queue(disk);
> -
>  	kobject_put(disk->part0->bd_holder_dir);
>  	kobject_put(disk->slave_dir);
>  	disk->slave_dir = NULL;
> @@ -759,10 +757,12 @@ static int __del_gendisk(struct gendisk_data *data)
>  	disk->part0->bd_stamp = 0;
>  	sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
>  	pm_runtime_set_memalloc_noio(disk_to_dev(disk), false);
> -	device_del(disk_to_dev(disk));
>  
>  	blk_mq_freeze_queue_wait(q);
>  
> +	blk_unregister_queue(disk);
> +	device_del(disk_to_dev(disk));
> +
>  	blk_throtl_cancel_bios(disk);
>  
>  	blk_sync_queue(q);
As we first freeze the queue and then enter blk_unregister_queue which 
deals with kobject/debufs, do we need to create memalloc GFP_NOIO scope
while running blk_unregister_queue? I see that device_del already defines
the GFP_NOIO scope. 

Thanks,
--Nilay

Re: [PATCH V2 11/20] block: move blk_unregister_queue() & device_del() after freeze wait

[Ming Lei]

On Sat, Apr 19, 2025 at 06:08:30PM +0530, Nilay Shroff wrote:
> 
> 
> On 4/18/25 10:06 PM, Ming Lei wrote:
> > Move blk_unregister_queue() & device_del() after freeze wait, and prepare
> > for unifying elevator switch.
> > 
> > This way is just fine, since bdev has been unhashed at the beginning of
> > del_gendisk(), both blk_unregister_queue() & device_del() are dealing
> > with kobject & debugfs thing only.
> > 
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  block/genhd.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/block/genhd.c b/block/genhd.c
> > index d22fdc0d5383..86c3db5b9305 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -749,8 +749,6 @@ static int __del_gendisk(struct gendisk_data *data)
> >  		bdi_unregister(disk->bdi);
> >  	}
> >  
> > -	blk_unregister_queue(disk);
> > -
> >  	kobject_put(disk->part0->bd_holder_dir);
> >  	kobject_put(disk->slave_dir);
> >  	disk->slave_dir = NULL;
> > @@ -759,10 +757,12 @@ static int __del_gendisk(struct gendisk_data *data)
> >  	disk->part0->bd_stamp = 0;
> >  	sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
> >  	pm_runtime_set_memalloc_noio(disk_to_dev(disk), false);
> > -	device_del(disk_to_dev(disk));
> >  
> >  	blk_mq_freeze_queue_wait(q);
> >  
> > +	blk_unregister_queue(disk);
> > +	device_del(disk_to_dev(disk));
> > +
> >  	blk_throtl_cancel_bios(disk);
> >  
> >  	blk_sync_queue(q);
> As we first freeze the queue and then enter blk_unregister_queue which 
> deals with kobject/debufs, do we need to create memalloc GFP_NOIO scope
> while running blk_unregister_queue? I see that device_del already defines
> the GFP_NOIO scope. 

We actually need to do that with the read lock together.


Thanks,
Ming

[PATCH V2 12/20] block: add `struct elv_change_ctx` for unifying elevator_change

[Ming Lei]

Add `struct elv_change_ctx` and prepare for unifying elevator_change(),
with this way, any input & output parameter can be provided & observed
in top caller.

This way also helps to move kobject & debugfs things out of
->elevator_lock & freezing queue.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c   | 16 ++++++++++------
 block/blk.h      |  4 ++--
 block/elevator.c | 33 +++++++++++++++++++--------------
 block/elevator.h |  7 +++++++
 4 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 0f4a5e674874..1a287c2e791c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4984,16 +4984,20 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 	}
 
 	list_for_each_entry(q, &set->tag_list, tag_set_list) {
-		const char *name = "none";
-
-		mutex_lock(&q->elevator_lock);
-		if (q->elevator && !blk_queue_dying(q))
-			name = q->elevator->type->elevator_name;
 		/*
 		 * nr_hw_queues is changed and elevator data depends on
 		 * it, so we have to force to rebuild elevator
 		 */
-		__elevator_change(q, name, true);
+		struct elv_change_ctx ctx = {
+			.name	= "none",
+			.force	= true,
+			.uevent	= true,
+		};
+
+		mutex_lock(&q->elevator_lock);
+		if (q->elevator && !blk_queue_dying(q))
+			ctx.name = q->elevator->type->elevator_name;
+		__elevator_change(q, &ctx);
 		mutex_unlock(&q->elevator_lock);
 	}
 
diff --git a/block/blk.h b/block/blk.h
index 0c3cc1af2525..be01cb9f3910 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -12,6 +12,7 @@
 #include "blk-crypto-internal.h"
 
 struct elevator_type;
+struct elv_change_ctx;
 
 #define	BLK_DEV_MAX_SECTORS	(LLONG_MAX >> 9)
 #define	BLK_MIN_SEGMENT_SIZE	4096
@@ -319,8 +320,7 @@ bool blk_bio_list_merge(struct request_queue *q, struct list_head *list,
 
 bool blk_insert_flush(struct request *rq);
 
-int __elevator_change(struct request_queue *q, const char *elevator_name,
-		      bool force);
+int __elevator_change(struct request_queue *q, struct elv_change_ctx *ctx);
 void elevator_exit(struct request_queue *q);
 int elv_register_queue(struct request_queue *q, bool uevent);
 void elv_unregister_queue(struct request_queue *q);
diff --git a/block/elevator.c b/block/elevator.c
index 6bf3871c7164..836138fc148a 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -53,7 +53,8 @@ static LIST_HEAD(elv_list);
  */
 #define rq_hash_key(rq)		(blk_rq_pos(rq) + blk_rq_sectors(rq))
 
-static int elevator_change(struct request_queue *q, const char *name);
+static int elevator_change(struct request_queue *q,
+			   struct elv_change_ctx *ctx);
 
 /*
  * Query io scheduler to see if the current process issuing bio may be
@@ -623,7 +624,8 @@ void elevator_init_mq(struct request_queue *q)
  * If switching fails, we are most likely running out of memory and not able
  * to restore the old io scheduler, so leaving the io scheduler being none.
  */
-static int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
+static int elevator_switch(struct request_queue *q, struct elevator_type *new_e,
+			   struct elv_change_ctx *ctx)
 {
 	int ret;
 
@@ -641,7 +643,7 @@ static int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
 	if (ret)
 		goto out_unfreeze;
 
-	ret = elv_register_queue(q, true);
+	ret = elv_register_queue(q, ctx->uevent);
 	if (ret) {
 		elevator_exit(q);
 		goto out_unfreeze;
@@ -679,9 +681,9 @@ static void elevator_disable(struct request_queue *q)
 /*
  * Switch this queue to the given IO scheduler.
  */
-int __elevator_change(struct request_queue *q, const char *elevator_name,
-		      bool force)
+int __elevator_change(struct request_queue *q, struct elv_change_ctx *ctx)
 {
+	const char *elevator_name = ctx->name;
 	struct elevator_type *e;
 	int ret;
 
@@ -695,19 +697,20 @@ int __elevator_change(struct request_queue *q, const char *elevator_name,
 		return 0;
 	}
 
-	if (!force && q->elevator &&
+	if (!ctx->force && q->elevator &&
 	    elevator_match(q->elevator->type, elevator_name))
 		return 0;
 
 	e = elevator_find_get(elevator_name);
 	if (!e)
 		return -EINVAL;
-	ret = elevator_switch(q, e);
+	ret = elevator_switch(q, e, ctx);
 	elevator_put(e);
 	return ret;
 }
 
-static int elevator_change(struct request_queue *q, const char *name)
+static int elevator_change(struct request_queue *q,
+			   struct elv_change_ctx *ctx)
 {
 	struct blk_mq_tag_set *set = q->tag_set;
 	unsigned int memflags;
@@ -721,7 +724,7 @@ static int elevator_change(struct request_queue *q, const char *name)
 
 	memflags = blk_mq_freeze_queue(q);
 	mutex_lock(&q->elevator_lock);
-	ret = __elevator_change(q, name, false);
+	ret = __elevator_change(q, ctx);
 	mutex_unlock(&q->elevator_lock);
 	blk_mq_unfreeze_queue(q, memflags);
 exit:
@@ -729,7 +732,7 @@ static int elevator_change(struct request_queue *q, const char *name)
 	return ret;
 }
 
-static void elv_iosched_load_module(char *elevator_name)
+static void elv_iosched_load_module(const char *elevator_name)
 {
 	struct elevator_type *found;
 
@@ -746,7 +749,9 @@ ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
 {
 	struct request_queue *q = disk->queue;
 	char elevator_name[ELV_NAME_MAX];
-	char *name;
+	struct elv_change_ctx ctx = {
+		.uevent = true,
+	};
 	int ret;
 
 	/*
@@ -755,11 +760,11 @@ ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
 	 * queue is the one for the device storing the module file.
 	 */
 	strscpy(elevator_name, buf, sizeof(elevator_name));
-	name = strstrip(elevator_name);
+	ctx.name = strstrip(elevator_name);
 
-	elv_iosched_load_module(name);
+	elv_iosched_load_module(ctx.name);
 
-	ret = elevator_change(q, name);
+	ret = elevator_change(q, &ctx);
 	if (!ret)
 		ret = count;
 	return ret;
diff --git a/block/elevator.h b/block/elevator.h
index 9198676644a9..63fc4cad16cc 100644
--- a/block/elevator.h
+++ b/block/elevator.h
@@ -122,6 +122,13 @@ struct elevator_queue
 
 #define ELEVATOR_FLAG_REGISTERED	0
 
+/* Holding context data for changing elevator */
+struct elv_change_ctx {
+	const char *name;
+	bool force;
+	bool uevent;
+};
+
 /*
  * block elevator interface
  */
-- 
2.47.0

Re: [PATCH V2 12/20] block: add `struct elv_change_ctx` for unifying elevator_change

[Nilay Shroff]

On 4/18/25 10:06 PM, Ming Lei wrote:
> Add `struct elv_change_ctx` and prepare for unifying elevator_change(),
> with this way, any input & output parameter can be provided & observed
> in top caller.
> 
> This way also helps to move kobject & debugfs things out of
> ->elevator_lock & freezing queue.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Looks good to me:
Reviewed-by : Nilay Shroff <nilay@linux.ibm.com>

Re: [PATCH V2 12/20] block: add `struct elv_change_ctx` for unifying elevator_change

[Christoph Hellwig]

On Sat, Apr 19, 2025 at 12:36:53AM +0800, Ming Lei wrote:
> Add `struct elv_change_ctx` and prepare for unifying elevator_change(),
> with this way, any input & output parameter can be provided & observed
> in top caller.
> 
> This way also helps to move kobject & debugfs things out of
> ->elevator_lock & freezing queue.

As pointed out last time, please move the entire loop body that
calls __elevator_change from __blk_mq_update_nr_hw_queues into a
helper inside of elevator.c, which means that this structure can
be kept private there.

Also please use a flags value with named flags instead of the various
booleans.

Re: [PATCH V2 12/20] block: add `struct elv_change_ctx` for unifying elevator_change

[Ming Lei]

On Tue, Apr 22, 2025 at 09:07:39AM +0200, Christoph Hellwig wrote:
> On Sat, Apr 19, 2025 at 12:36:53AM +0800, Ming Lei wrote:
> > Add `struct elv_change_ctx` and prepare for unifying elevator_change(),
> > with this way, any input & output parameter can be provided & observed
> > in top caller.
> > 
> > This way also helps to move kobject & debugfs things out of
> > ->elevator_lock & freezing queue.
> 
> As pointed out last time, please move the entire loop body that
> calls __elevator_change from __blk_mq_update_nr_hw_queues into a
> helper inside of elevator.c, which means that this structure can
> be kept private there.

Please see the following patch of 'block: move elv_register[unregister]_queue out of elevator_lock'
in which elevator_change_done() has to be added, then the context structure
can't be kept as private any more.

> 
> Also please use a flags value with named flags instead of the various
> booleans.

'struct elv_change_ctx' has to be parameter, so it doesn't matter to
use flags value any more, and 'bool' should be easier.


Thanks,
Ming

Re: [PATCH V2 12/20] block: add `struct elv_change_ctx` for unifying elevator_change

[Christoph Hellwig]

[-- Attachment #1: Type: text/plain, Size: 808 bytes --]

On Tue, Apr 22, 2025 at 04:36:08PM +0800, Ming Lei wrote:
> Please see the following patch of 'block: move elv_register[unregister]_queue out of elevator_lock'
> in which elevator_change_done() has to be added, then the context structure
> can't be kept as private any more.

It can.  See the attached first patch.  The second patch then cleans
things up further so that we don't need the force flag or
__elevator_change.

> > 
> > Also please use a flags value with named flags instead of the various
> > booleans.
> 
> 'struct elv_change_ctx' has to be parameter, so it doesn't matter to
> use flags value any more, and 'bool' should be easier.

It's still much more readable to have flags.  Especially to discover
and document how init and uevent are related, which seems rather
confusing at the moment.


[-- Attachment #2: 0001-block-make-struct-elv_change_ctx-private-in-elevator.patch --]
[-- Type: text/x-patch, Size: 5146 bytes --]

From a41e07a5c401f8a73d622e670881f79b7636baf6 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Wed, 23 Apr 2025 08:38:23 +0200
Subject: block: make struct elv_change_ctx private in elevator.c

And add a helper for the nr_hw_queues update instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-mq.c   | 23 ++---------------------
 block/blk.h      |  4 +---
 block/elevator.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
 block/elevator.h | 13 -------------
 4 files changed, 45 insertions(+), 39 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 5c9f7391af92..61d3349b3bb3 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4968,27 +4968,8 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 		blk_mq_map_swqueue(q);
 	}
 
-	list_for_each_entry(q, &set->tag_list, tag_set_list) {
-		/*
-		 * nr_hw_queues is changed and elevator data depends on
-		 * it, so we have to force to rebuild elevator
-		 */
-		struct elv_change_ctx ctx = {
-			.name	= "none",
-			.force	= true,
-			.uevent	= true,
-		};
-		int ret = -ENODEV;
-
-		mutex_lock(&q->elevator_lock);
-		if (q->elevator && !blk_queue_dying(q))
-			ctx.name = q->elevator->type->elevator_name;
-		ret = __elevator_change(q, &ctx);
-		mutex_unlock(&q->elevator_lock);
-		blk_mq_unfreeze_queue_nomemrestore(q);
-		if (!ret)
-			WARN_ON_ONCE(elevator_change_done(q, &ctx));
-	}
+	list_for_each_entry(q, &set->tag_list, tag_set_list)
+		elv_update_nr_hw_queues(q);
 	memalloc_noio_restore(memflags);
 
 reregister:
diff --git a/block/blk.h b/block/blk.h
index 48cf6b1c36fe..6ba674d1ceba 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -12,7 +12,6 @@
 #include "blk-crypto-internal.h"
 
 struct elevator_type;
-struct elv_change_ctx;
 
 #define	BLK_DEV_MAX_SECTORS	(LLONG_MAX >> 9)
 #define	BLK_MIN_SEGMENT_SIZE	4096
@@ -320,10 +319,9 @@ bool blk_bio_list_merge(struct request_queue *q, struct list_head *list,
 
 bool blk_insert_flush(struct request *rq);
 
-int __elevator_change(struct request_queue *q, struct elv_change_ctx *ctx);
+void elv_update_nr_hw_queues(struct request_queue *q);
 void elevator_set_default(struct request_queue *q);
 void elevator_set_none(struct request_queue *q);
-int elevator_change_done(struct request_queue *q, struct elv_change_ctx *ctx);
 
 ssize_t part_size_show(struct device *dev, struct device_attribute *attr,
 		char *buf);
diff --git a/block/elevator.c b/block/elevator.c
index 378553fce5d8..71329a73bb44 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -53,6 +53,19 @@ static LIST_HEAD(elv_list);
  */
 #define rq_hash_key(rq)		(blk_rq_pos(rq) + blk_rq_sectors(rq))
 
+/* Holding context data for changing elevator */
+struct elv_change_ctx {
+	const char *name;
+	bool force;
+	bool uevent;
+	bool init;
+
+	/* for unregistering old elevator */
+	struct elevator_queue *old;
+	/* for registering new elevator */
+	struct elevator_queue *new;
+};
+
 static int elevator_change(struct request_queue *q,
 			   struct elv_change_ctx *ctx);
 
@@ -682,7 +695,8 @@ static void elevator_disable(struct request_queue *q,
 	blk_mq_unquiesce_queue(q);
 }
 
-int elevator_change_done(struct request_queue *q, struct elv_change_ctx *ctx)
+static int elevator_change_done(struct request_queue *q,
+		struct elv_change_ctx *ctx)
 {
 	int ret = 0;
 
@@ -712,7 +726,8 @@ int elevator_change_done(struct request_queue *q, struct elv_change_ctx *ctx)
 /*
  * Switch this queue to the given IO scheduler.
  */
-int __elevator_change(struct request_queue *q, struct elv_change_ctx *ctx)
+static int __elevator_change(struct request_queue *q,
+		struct elv_change_ctx *ctx)
 {
 	const char *elevator_name = ctx->name;
 	struct elevator_type *e;
@@ -740,6 +755,31 @@ int __elevator_change(struct request_queue *q, struct elv_change_ctx *ctx)
 	return ret;
 }
 
+/*
+ * The I/O scheduler depends on the number of hardware queues, this forces a
+ * reattachment when nr_hw_queues changes.
+ *
+ * Note that this unfreezes the passed in queue.
+ */
+void elv_update_nr_hw_queues(struct request_queue *q)
+{
+	struct elv_change_ctx ctx = {
+		.name	= "none",
+		.force	= true,
+		.uevent	= true,
+	};
+	int ret;
+
+	mutex_lock(&q->elevator_lock);
+	if (q->elevator && !blk_queue_dying(q))
+		ctx.name = q->elevator->type->elevator_name;
+	ret = __elevator_change(q, &ctx);
+	mutex_unlock(&q->elevator_lock);
+	blk_mq_unfreeze_queue_nomemrestore(q);
+	if (!ret)
+		WARN_ON_ONCE(elevator_change_done(q, &ctx));
+}
+
 static int elevator_change(struct request_queue *q,
 			   struct elv_change_ctx *ctx)
 {
diff --git a/block/elevator.h b/block/elevator.h
index b14c611c74b6..a07ce773a38f 100644
--- a/block/elevator.h
+++ b/block/elevator.h
@@ -124,19 +124,6 @@ struct elevator_queue
 #define ELEVATOR_FLAG_DYING		1
 #define ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT	2
 
-/* Holding context data for changing elevator */
-struct elv_change_ctx {
-	const char *name;
-	bool force;
-	bool uevent;
-	bool init;
-
-	/* for unregistering old elevator */
-	struct elevator_queue *old;
-	/* for registering new elevator */
-	struct elevator_queue *new;
-};
-
 /*
  * block elevator interface
  */
-- 
2.47.2


[-- Attachment #3: 0002-block-remove-__elevator_change.patch --]
[-- Type: text/x-patch, Size: 4234 bytes --]

From c0f7a6a6ed84a83ec4f1e7459b1715bcd612470f Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Wed, 23 Apr 2025 08:53:08 +0200
Subject: block: remove __elevator_change

elv_update_nr_hw_queues is very different from the other elevator
change cases, so open code the logic there.  Move the lookup of the
elevator type into elevator_switch to simplify this a bit.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/elevator.c | 73 ++++++++++++++++++++----------------------------
 1 file changed, 30 insertions(+), 43 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index 71329a73bb44..e51f7b8bad16 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -56,7 +56,6 @@ static LIST_HEAD(elv_list);
 /* Holding context data for changing elevator */
 struct elv_change_ctx {
 	const char *name;
-	bool force;
 	bool uevent;
 	bool init;
 
@@ -644,11 +643,15 @@ void elevator_set_none(struct request_queue *q)
  * If switching fails, we are most likely running out of memory and not able
  * to restore the old io scheduler, so leaving the io scheduler being none.
  */
-static int elevator_switch(struct request_queue *q, struct elevator_type *new_e,
-			   struct elv_change_ctx *ctx)
+static int elevator_switch(struct request_queue *q, struct elv_change_ctx *ctx)
 {
+	struct elevator_type *new_e;
 	int ret;
 
+	new_e = elevator_find_get(ctx->name);
+	if (!new_e)
+		return -EINVAL;
+
 	WARN_ON_ONCE(q->mq_freeze_depth == 0);
 	lockdep_assert_held(&q->elevator_lock);
 
@@ -674,6 +677,7 @@ static int elevator_switch(struct request_queue *q, struct elevator_type *new_e,
 			new_e->elevator_name);
 	}
 
+	elevator_put(new_e);
 	return ret;
 }
 
@@ -723,38 +727,6 @@ static int elevator_change_done(struct request_queue *q,
 	return 0;
 }
 
-/*
- * Switch this queue to the given IO scheduler.
- */
-static int __elevator_change(struct request_queue *q,
-		struct elv_change_ctx *ctx)
-{
-	const char *elevator_name = ctx->name;
-	struct elevator_type *e;
-	int ret;
-
-	/* Make sure queue is not in the middle of being removed */
-	if (!ctx->init && !blk_queue_registered(q))
-		return -ENOENT;
-
-	if (!strncmp(elevator_name, "none", 4)) {
-		if (q->elevator)
-			elevator_disable(q, ctx);
-		return 0;
-	}
-
-	if (!ctx->force && q->elevator &&
-	    elevator_match(q->elevator->type, elevator_name))
-		return 0;
-
-	e = elevator_find_get(elevator_name);
-	if (!e)
-		return -EINVAL;
-	ret = elevator_switch(q, e, ctx);
-	elevator_put(e);
-	return ret;
-}
-
 /*
  * The I/O scheduler depends on the number of hardware queues, this forces a
  * reattachment when nr_hw_queues changes.
@@ -764,19 +736,25 @@ static int __elevator_change(struct request_queue *q,
 void elv_update_nr_hw_queues(struct request_queue *q)
 {
 	struct elv_change_ctx ctx = {
-		.name	= "none",
-		.force	= true,
 		.uevent	= true,
 	};
-	int ret;
+	bool done = false;
 
 	mutex_lock(&q->elevator_lock);
-	if (q->elevator && !blk_queue_dying(q))
+	if (!blk_queue_registered(q))
+		goto out_unlock;
+
+	if (q->elevator) {
 		ctx.name = q->elevator->type->elevator_name;
-	ret = __elevator_change(q, &ctx);
+		if (elevator_switch(q, &ctx) == 0)
+			done = true;
+	}
+
+out_unlock:
 	mutex_unlock(&q->elevator_lock);
 	blk_mq_unfreeze_queue_nomemrestore(q);
-	if (!ret)
+
+	if (done)
 		WARN_ON_ONCE(elevator_change_done(q, &ctx));
 }
 
@@ -785,7 +763,7 @@ static int elevator_change(struct request_queue *q,
 {
 	struct blk_mq_tag_set *set = q->tag_set;
 	unsigned int memflags;
-	int ret, idx;
+	int ret = 0, idx;
 
 	idx = srcu_read_lock(&set->update_nr_hwq_srcu);
 	if (set->updating_nr_hwq) {
@@ -805,7 +783,16 @@ static int elevator_change(struct request_queue *q,
 	 */
 	blk_mq_cancel_work_sync(q);
 	mutex_lock(&q->elevator_lock);
-	ret = __elevator_change(q, ctx);
+	/* Make sure queue is not in the middle of being removed */
+	if (!ctx->init && !blk_queue_registered(q)) {
+		ret = -ENOENT;
+	} else if (!strncmp(ctx->name, "none", 4)) {
+		if (q->elevator)
+			elevator_disable(q, ctx);
+	} else if (!q->elevator ||
+		   !elevator_match(q->elevator->type, ctx->name)) {
+		ret = elevator_switch(q, ctx);
+	}
 	mutex_unlock(&q->elevator_lock);
 	blk_mq_unfreeze_queue(q, memflags);
 	if (!ret)
-- 
2.47.2

[PATCH V2 13/20] block: unifying elevator change

[Ming Lei]

elevator change is one well-define behavior:

- tear down current elevator if it exists

- setup new elevator

It is supposed to cover any case for changing elevator by single
internal API, typically the following cases:

- setup default elevator in add_disk()

- switch to none in del_disk()

- reset elevator in blk_mq_update_nr_hw_queues()

- switch elevator in sysfs `store` elevator attribute

This patch uses elevator_change() to cover all above cases:

- every elevator switch is serialized with each other: add_disk/del_disk/
store elevator is serialized already, blk_mq_update_nr_hw_queues() uses
srcu for syncing with the other three cases

- for both add_disk()/del_disk(), queue freeze works at atomic mode
or has been froze, so the freeze in elevator_change() won't add extra
delay

- `struct elev_change_ctx` instance holds any info for changing elevator

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-sysfs.c | 18 ++++-------
 block/blk.h       |  5 ++-
 block/elevator.c  | 81 ++++++++++++++++++++++++++---------------------
 block/elevator.h  |  1 +
 block/genhd.c     | 19 +----------
 5 files changed, 55 insertions(+), 69 deletions(-)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index a2882751f0d2..58c50709bc14 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -869,14 +869,8 @@ int blk_register_queue(struct gendisk *disk)
 	if (ret)
 		goto out_unregister_ia_ranges;
 
+	elevator_set_default(q);
 	mutex_lock(&q->elevator_lock);
-	if (q->elevator) {
-		ret = elv_register_queue(q, false);
-		if (ret) {
-			mutex_unlock(&q->elevator_lock);
-			goto out_crypto_sysfs_unregister;
-		}
-	}
 	wbt_enable_default(disk);
 	mutex_unlock(&q->elevator_lock);
 
@@ -902,8 +896,6 @@ int blk_register_queue(struct gendisk *disk)
 
 	return ret;
 
-out_crypto_sysfs_unregister:
-	blk_crypto_sysfs_unregister(disk);
 out_unregister_ia_ranges:
 	disk_unregister_independent_access_ranges(disk);
 out_debugfs_remove:
@@ -949,9 +941,11 @@ void blk_unregister_queue(struct gendisk *disk)
 		blk_mq_sysfs_unregister(disk);
 	blk_crypto_sysfs_unregister(disk);
 
-	mutex_lock(&q->elevator_lock);
-	elv_unregister_queue(q);
-	mutex_unlock(&q->elevator_lock);
+	if (q->elevator) {
+		blk_mq_quiesce_queue(q);
+		elevator_set_none(q);
+		blk_mq_unquiesce_queue(q);
+	}
 
 	mutex_lock(&q->sysfs_lock);
 	disk_unregister_independent_access_ranges(disk);
diff --git a/block/blk.h b/block/blk.h
index be01cb9f3910..0e19c09009ed 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -321,9 +321,8 @@ bool blk_bio_list_merge(struct request_queue *q, struct list_head *list,
 bool blk_insert_flush(struct request *rq);
 
 int __elevator_change(struct request_queue *q, struct elv_change_ctx *ctx);
-void elevator_exit(struct request_queue *q);
-int elv_register_queue(struct request_queue *q, bool uevent);
-void elv_unregister_queue(struct request_queue *q);
+void elevator_set_default(struct request_queue *q);
+void elevator_set_none(struct request_queue *q);
 
 ssize_t part_size_show(struct device *dev, struct device_attribute *attr,
 		char *buf);
diff --git a/block/elevator.c b/block/elevator.c
index 836138fc148a..936d8ec9e9f0 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -151,7 +151,7 @@ static void elevator_release(struct kobject *kobj)
 	kfree(e);
 }
 
-void elevator_exit(struct request_queue *q)
+static void elevator_exit(struct request_queue *q)
 {
 	struct elevator_queue *e = q->elevator;
 
@@ -455,7 +455,7 @@ static const struct kobj_type elv_ktype = {
 	.release	= elevator_release,
 };
 
-int elv_register_queue(struct request_queue *q, bool uevent)
+static int elv_register_queue(struct request_queue *q, bool uevent)
 {
 	struct elevator_queue *e = q->elevator;
 	int error;
@@ -485,7 +485,7 @@ int elv_register_queue(struct request_queue *q, bool uevent)
 	return error;
 }
 
-void elv_unregister_queue(struct request_queue *q)
+static void elv_unregister_queue(struct request_queue *q)
 {
 	struct elevator_queue *e = q->elevator;
 
@@ -562,60 +562,59 @@ EXPORT_SYMBOL_GPL(elv_unregister);
  * For single queue devices, default to using mq-deadline. If we have multiple
  * queues or mq-deadline is not available, default to "none".
  */
-static struct elevator_type *elevator_get_default(struct request_queue *q)
+static bool use_default_elevator(struct request_queue *q)
 {
 	if (q->tag_set->flags & BLK_MQ_F_NO_SCHED_BY_DEFAULT)
-		return NULL;
+		return false;
 
 	if (q->nr_hw_queues != 1 &&
 	    !blk_mq_is_shared_tags(q->tag_set->flags))
-		return NULL;
+		return false;
 
-	return elevator_find_get("mq-deadline");
+	return true;
 }
 
 /*
  * Use the default elevator settings. If the chosen elevator initialization
  * fails, fall back to the "none" elevator (no elevator).
  */
-void elevator_init_mq(struct request_queue *q)
+void elevator_set_default(struct request_queue *q)
 {
-	struct elevator_type *e;
-	unsigned int memflags;
+	struct elv_change_ctx ctx = {
+		.init = true,
+	};
 	int err;
 
-	WARN_ON_ONCE(blk_queue_registered(q));
-
-	if (unlikely(q->elevator))
+	if (!queue_is_mq(q))
 		return;
 
-	e = elevator_get_default(q);
-	if (!e)
+	ctx.name = use_default_elevator(q) ? "mq-deadline" : "none";
+	if (!q->elevator && !strcmp(ctx.name, "none"))
 		return;
+	err = elevator_change(q, &ctx);
+	if (err < 0)
+		pr_warn("\"%s\" set elevator failed %d, "
+			"falling back to \"none\"\n", ctx.name, err);
+}
 
-	/*
-	 * We are called before adding disk, when there isn't any FS I/O,
-	 * so freezing queue plus canceling dispatch work is enough to
-	 * drain any dispatch activities originated from passthrough
-	 * requests, then no need to quiesce queue which may add long boot
-	 * latency, especially when lots of disks are involved.
-	 *
-	 * Disk isn't added yet, so verifying queue lock only manually.
-	 */
-	memflags = blk_mq_freeze_queue(q);
-
-	blk_mq_cancel_work_sync(q);
-
-	err = blk_mq_init_sched(q, e);
+void elevator_set_none(struct request_queue *q)
+{
+	struct elv_change_ctx ctx = {
+		.name	= "none",
+		.uevent = true,
+		.init = true,
+	};
+	int err;
 
-	blk_mq_unfreeze_queue(q, memflags);
+	if (!queue_is_mq(q))
+		return;
 
-	if (err) {
-		pr_warn("\"%s\" elevator initialization failed, "
-			"falling back to \"none\"\n", e->elevator_name);
-	}
+	if (!q->elevator)
+		return;
 
-	elevator_put(e);
+	err = elevator_change(q, &ctx);
+	if (err < 0)
+		pr_warn("%s: set none elevator failed %d\n", __func__, err);
 }
 
 /*
@@ -688,7 +687,7 @@ int __elevator_change(struct request_queue *q, struct elv_change_ctx *ctx)
 	int ret;
 
 	/* Make sure queue is not in the middle of being removed */
-	if (!blk_queue_registered(q))
+	if (!ctx->init && !blk_queue_registered(q))
 		return -ENOENT;
 
 	if (!strncmp(elevator_name, "none", 4)) {
@@ -723,6 +722,16 @@ static int elevator_change(struct request_queue *q,
 	}
 
 	memflags = blk_mq_freeze_queue(q);
+	/*
+	 * May be called before adding disk, when there isn't any FS I/O,
+	 * so freezing queue plus canceling dispatch work is enough to
+	 * drain any dispatch activities originated from passthrough
+	 * requests, then no need to quiesce queue which may add long boot
+	 * latency, especially when lots of disks are involved.
+	 *
+	 * Disk isn't added yet, so verifying queue lock only manually.
+	 */
+	blk_mq_cancel_work_sync(q);
 	mutex_lock(&q->elevator_lock);
 	ret = __elevator_change(q, ctx);
 	mutex_unlock(&q->elevator_lock);
diff --git a/block/elevator.h b/block/elevator.h
index 63fc4cad16cc..e74e27dd6586 100644
--- a/block/elevator.h
+++ b/block/elevator.h
@@ -127,6 +127,7 @@ struct elv_change_ctx {
 	const char *name;
 	bool force;
 	bool uevent;
+	bool init;
 };
 
 /*
diff --git a/block/genhd.c b/block/genhd.c
index 86c3db5b9305..de227aa923ed 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -438,12 +438,6 @@ static int __add_disk_fwnode(struct gendisk_data *data)
 		 */
 		if (disk->fops->submit_bio || disk->fops->poll_bio)
 			return -EINVAL;
-
-		/*
-		 * Initialize the I/O scheduler code and pick a default one if
-		 * needed.
-		 */
-		elevator_init_mq(disk->queue);
 	} else {
 		if (!disk->fops->submit_bio)
 			return -EINVAL;
@@ -587,11 +581,7 @@ static int __add_disk_fwnode(struct gendisk_data *data)
 	if (disk->major == BLOCK_EXT_MAJOR)
 		blk_free_ext_minor(disk->first_minor);
 out_exit_elevator:
-	if (disk->queue->elevator) {
-		mutex_lock(&disk->queue->elevator_lock);
-		elevator_exit(disk->queue);
-		mutex_unlock(&disk->queue->elevator_lock);
-	}
+	elevator_set_none(disk->queue);
 	return ret;
 }
 
@@ -771,14 +761,7 @@ static int __del_gendisk(struct gendisk_data *data)
 	if (queue_is_mq(q))
 		blk_mq_cancel_work_sync(q);
 
-	blk_mq_quiesce_queue(q);
-	if (q->elevator) {
-		mutex_lock(&q->elevator_lock);
-		elevator_exit(q);
-		mutex_unlock(&q->elevator_lock);
-	}
 	rq_qos_exit(q);
-	blk_mq_unquiesce_queue(q);
 
 	/*
 	 * If the disk does not own the queue, allow using passthrough requests
-- 
2.47.0

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

[Nilay Shroff]

On 4/18/25 10:06 PM, Ming Lei wrote:
> elevator change is one well-define behavior:
> 
> - tear down current elevator if it exists
> 
> - setup new elevator
> 
> It is supposed to cover any case for changing elevator by single
> internal API, typically the following cases:
> 
> - setup default elevator in add_disk()
> 
> - switch to none in del_disk()
> 
> - reset elevator in blk_mq_update_nr_hw_queues()
> 
> - switch elevator in sysfs `store` elevator attribute
> 
> This patch uses elevator_change() to cover all above cases:
> 
> - every elevator switch is serialized with each other: add_disk/del_disk/
> store elevator is serialized already, blk_mq_update_nr_hw_queues() uses
> srcu for syncing with the other three cases
> 
> - for both add_disk()/del_disk(), queue freeze works at atomic mode
> or has been froze, so the freeze in elevator_change() won't add extra
> delay
> 
> - `struct elev_change_ctx` instance holds any info for changing elevator
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-sysfs.c | 18 ++++-------
>  block/blk.h       |  5 ++-
>  block/elevator.c  | 81 ++++++++++++++++++++++++++---------------------
>  block/elevator.h  |  1 +
>  block/genhd.c     | 19 +----------
>  5 files changed, 55 insertions(+), 69 deletions(-)
> 
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index a2882751f0d2..58c50709bc14 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -869,14 +869,8 @@ int blk_register_queue(struct gendisk *disk)
>  	if (ret)
>  		goto out_unregister_ia_ranges;
>  
> +	elevator_set_default(q);
>  	mutex_lock(&q->elevator_lock);
> -	if (q->elevator) {
> -		ret = elv_register_queue(q, false);
> -		if (ret) {
> -			mutex_unlock(&q->elevator_lock);
> -			goto out_crypto_sysfs_unregister;
> -		}
> -	}
>  	wbt_enable_default(disk);
>  	mutex_unlock(&q->elevator_lock);
>  
[...]
[...]
>  /*
> diff --git a/block/genhd.c b/block/genhd.c
> index 86c3db5b9305..de227aa923ed 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -438,12 +438,6 @@ static int __add_disk_fwnode(struct gendisk_data *data)
>  		 */
>  		if (disk->fops->submit_bio || disk->fops->poll_bio)
>  			return -EINVAL;
> -
> -		/*
> -		 * Initialize the I/O scheduler code and pick a default one if
> -		 * needed.
> -		 */
> -		elevator_init_mq(disk->queue);
>  	} else {
>  		if (!disk->fops->submit_bio)
>  			return -EINVAL;
> @@ -587,11 +581,7 @@ static int __add_disk_fwnode(struct gendisk_data *data)
>  	if (disk->major == BLOCK_EXT_MAJOR)
>  		blk_free_ext_minor(disk->first_minor);
>  out_exit_elevator:
> -	if (disk->queue->elevator) {
> -		mutex_lock(&disk->queue->elevator_lock);
> -		elevator_exit(disk->queue);
> -		mutex_unlock(&disk->queue->elevator_lock);
> -	}
> +	elevator_set_none(disk->queue);
>  	return ret;
>  }
As we moved elevator init function (elevator_set_default) inside blk_register_queue, 
we probably now don't need label out_exit_elevator in __add_disk_fwnode. In fact,
no code in __add_disk_fwnode shall jump to this label.
I think, elevator_set_none may be called anytime after we see failure post 
blk_register_queue returns.
Thanks,
--Nilay 

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

[Ming Lei]

On Sat, Apr 19, 2025 at 06:56:14PM +0530, Nilay Shroff wrote:
> 
> 
> On 4/18/25 10:06 PM, Ming Lei wrote:
> > elevator change is one well-define behavior:
> > 
> > - tear down current elevator if it exists
> > 
> > - setup new elevator
> > 
> > It is supposed to cover any case for changing elevator by single
> > internal API, typically the following cases:
> > 
> > - setup default elevator in add_disk()
> > 
> > - switch to none in del_disk()
> > 
> > - reset elevator in blk_mq_update_nr_hw_queues()
> > 
> > - switch elevator in sysfs `store` elevator attribute
> > 
> > This patch uses elevator_change() to cover all above cases:
> > 
> > - every elevator switch is serialized with each other: add_disk/del_disk/
> > store elevator is serialized already, blk_mq_update_nr_hw_queues() uses
> > srcu for syncing with the other three cases
> > 
> > - for both add_disk()/del_disk(), queue freeze works at atomic mode
> > or has been froze, so the freeze in elevator_change() won't add extra
> > delay
> > 
> > - `struct elev_change_ctx` instance holds any info for changing elevator
> > 
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  block/blk-sysfs.c | 18 ++++-------
> >  block/blk.h       |  5 ++-
> >  block/elevator.c  | 81 ++++++++++++++++++++++++++---------------------
> >  block/elevator.h  |  1 +
> >  block/genhd.c     | 19 +----------
> >  5 files changed, 55 insertions(+), 69 deletions(-)
> > 
> > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> > index a2882751f0d2..58c50709bc14 100644
> > --- a/block/blk-sysfs.c
> > +++ b/block/blk-sysfs.c
> > @@ -869,14 +869,8 @@ int blk_register_queue(struct gendisk *disk)
> >  	if (ret)
> >  		goto out_unregister_ia_ranges;
> >  
> > +	elevator_set_default(q);
> >  	mutex_lock(&q->elevator_lock);
> > -	if (q->elevator) {
> > -		ret = elv_register_queue(q, false);
> > -		if (ret) {
> > -			mutex_unlock(&q->elevator_lock);
> > -			goto out_crypto_sysfs_unregister;
> > -		}
> > -	}
> >  	wbt_enable_default(disk);
> >  	mutex_unlock(&q->elevator_lock);
> >  
> [...]
> [...]
> >  /*
> > diff --git a/block/genhd.c b/block/genhd.c
> > index 86c3db5b9305..de227aa923ed 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -438,12 +438,6 @@ static int __add_disk_fwnode(struct gendisk_data *data)
> >  		 */
> >  		if (disk->fops->submit_bio || disk->fops->poll_bio)
> >  			return -EINVAL;
> > -
> > -		/*
> > -		 * Initialize the I/O scheduler code and pick a default one if
> > -		 * needed.
> > -		 */
> > -		elevator_init_mq(disk->queue);
> >  	} else {
> >  		if (!disk->fops->submit_bio)
> >  			return -EINVAL;
> > @@ -587,11 +581,7 @@ static int __add_disk_fwnode(struct gendisk_data *data)
> >  	if (disk->major == BLOCK_EXT_MAJOR)
> >  		blk_free_ext_minor(disk->first_minor);
> >  out_exit_elevator:
> > -	if (disk->queue->elevator) {
> > -		mutex_lock(&disk->queue->elevator_lock);
> > -		elevator_exit(disk->queue);
> > -		mutex_unlock(&disk->queue->elevator_lock);
> > -	}
> > +	elevator_set_none(disk->queue);
> >  	return ret;
> >  }
> As we moved elevator init function (elevator_set_default) inside blk_register_queue, 
> we probably now don't need label out_exit_elevator in __add_disk_fwnode. In fact,
> no code in __add_disk_fwnode shall jump to this label.
> I think, elevator_set_none may be called anytime after we see failure post 
> blk_register_queue returns.

Good catch, 'out_exit_elevator' can rename as 'out', and the last
elevator_set_none() can be dropped.

Thanks,
Ming

[PATCH V2 14/20] block: pass elevator_queue to elv_register_queue & unregister_queue

[Ming Lei]

Pass elevator_queue reference to elv_register_queue() & elv_unregister_queue().

No functional change, and prepare for moving the two out of elevator
lock & freezing queue, when we need to store the old & new elevator
queue in `struct elv_change_ctx` instance, then both two can co-exist
for short while, so we have to pass the specific elevator_queue instance
to elv_register_queue & unregister_queue.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/elevator.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index 936d8ec9e9f0..568457e01d28 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -455,9 +455,10 @@ static const struct kobj_type elv_ktype = {
 	.release	= elevator_release,
 };
 
-static int elv_register_queue(struct request_queue *q, bool uevent)
+static int elv_register_queue(struct request_queue *q,
+			      struct elevator_queue *e,
+			      bool uevent)
 {
-	struct elevator_queue *e = q->elevator;
 	int error;
 
 	lockdep_assert_held(&q->elevator_lock);
@@ -485,10 +486,9 @@ static int elv_register_queue(struct request_queue *q, bool uevent)
 	return error;
 }
 
-static void elv_unregister_queue(struct request_queue *q)
+static void elv_unregister_queue(struct request_queue *q,
+				 struct elevator_queue *e)
 {
-	struct elevator_queue *e = q->elevator;
-
 	lockdep_assert_held(&q->elevator_lock);
 
 	if (e && test_and_clear_bit(ELEVATOR_FLAG_REGISTERED, &e->flags)) {
@@ -634,7 +634,7 @@ static int elevator_switch(struct request_queue *q, struct elevator_type *new_e,
 	blk_mq_quiesce_queue(q);
 
 	if (q->elevator) {
-		elv_unregister_queue(q);
+		elv_unregister_queue(q, q->elevator);
 		elevator_exit(q);
 	}
 
@@ -642,7 +642,7 @@ static int elevator_switch(struct request_queue *q, struct elevator_type *new_e,
 	if (ret)
 		goto out_unfreeze;
 
-	ret = elv_register_queue(q, ctx->uevent);
+	ret = elv_register_queue(q, q->elevator, ctx->uevent);
 	if (ret) {
 		elevator_exit(q);
 		goto out_unfreeze;
@@ -667,7 +667,7 @@ static void elevator_disable(struct request_queue *q)
 
 	blk_mq_quiesce_queue(q);
 
-	elv_unregister_queue(q);
+	elv_unregister_queue(q, q->elevator);
 	elevator_exit(q);
 	blk_queue_flag_clear(QUEUE_FLAG_SQ_SCHED, q);
 	q->elevator = NULL;
-- 
2.47.0

Re: [PATCH V2 14/20] block: pass elevator_queue to elv_register_queue & unregister_queue

[Nilay Shroff]

On 4/18/25 10:06 PM, Ming Lei wrote:
> Pass elevator_queue reference to elv_register_queue() & elv_unregister_queue().
> 
> No functional change, and prepare for moving the two out of elevator
> lock & freezing queue, when we need to store the old & new elevator
> queue in `struct elv_change_ctx` instance, then both two can co-exist
> for short while, so we have to pass the specific elevator_queue instance
> to elv_register_queue & unregister_queue.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Looks good to me:
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

[PATCH V2 15/20] block: fail to show/store elevator sysfs attribute if elevator is dying

[Ming Lei]

Prepare for moving elv_register[unregister]_queue out of elevator_lock
& queue freezing, so we may have to call elv_unregister_queue() after
elevator ->exit() is called, then there is small window for user to
run into ->show()/store(), and user-after-free can be caused.

Fail to show/store elevator sysfs attribute if elevator is dying by
adding one new flag of ELEVATOR_FLAG_DYNG, which is protected by
elevator ->sysfs_lock.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq-sched.c |  1 +
 block/elevator.c     | 10 ++++++++--
 block/elevator.h     |  1 +
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 336a15ffecfa..55a0fd105147 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -551,5 +551,6 @@ void blk_mq_exit_sched(struct request_queue *q, struct elevator_queue *e)
 	if (e->type->ops.exit_sched)
 		e->type->ops.exit_sched(e);
 	blk_mq_sched_tags_teardown(q, flags);
+	set_bit(ELEVATOR_FLAG_DYING, &q->elevator->flags);
 	q->elevator = NULL;
 }
diff --git a/block/elevator.c b/block/elevator.c
index 568457e01d28..16171ea92f80 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -422,7 +422,10 @@ elv_attr_show(struct kobject *kobj, struct attribute *attr, char *page)
 
 	e = container_of(kobj, struct elevator_queue, kobj);
 	mutex_lock(&e->sysfs_lock);
-	error = e->type ? entry->show(e, page) : -ENOENT;
+	if (test_bit(ELEVATOR_FLAG_DYING, &e->flags))
+		error = -ENODEV;
+	else
+		error = e->type ? entry->show(e, page) : -ENOENT;
 	mutex_unlock(&e->sysfs_lock);
 	return error;
 }
@@ -440,7 +443,10 @@ elv_attr_store(struct kobject *kobj, struct attribute *attr,
 
 	e = container_of(kobj, struct elevator_queue, kobj);
 	mutex_lock(&e->sysfs_lock);
-	error = e->type ? entry->store(e, page, length) : -ENOENT;
+	if (test_bit(ELEVATOR_FLAG_DYING, &e->flags))
+		error = -ENODEV;
+	else
+		error = e->type ? entry->store(e, page, length) : -ENOENT;
 	mutex_unlock(&e->sysfs_lock);
 	return error;
 }
diff --git a/block/elevator.h b/block/elevator.h
index e74e27dd6586..16d8888fa2b2 100644
--- a/block/elevator.h
+++ b/block/elevator.h
@@ -121,6 +121,7 @@ struct elevator_queue
 };
 
 #define ELEVATOR_FLAG_REGISTERED	0
+#define ELEVATOR_FLAG_DYING		1
 
 /* Holding context data for changing elevator */
 struct elv_change_ctx {
-- 
2.47.0

Re: [PATCH V2 15/20] block: fail to show/store elevator sysfs attribute if elevator is dying

[Nilay Shroff]

On 4/18/25 10:06 PM, Ming Lei wrote:
> Prepare for moving elv_register[unregister]_queue out of elevator_lock
> & queue freezing, so we may have to call elv_unregister_queue() after
> elevator ->exit() is called, then there is small window for user to
> run into ->show()/store(), and user-after-free can be caused.
> 
> Fail to show/store elevator sysfs attribute if elevator is dying by
> adding one new flag of ELEVATOR_FLAG_DYNG, which is protected by
> elevator ->sysfs_lock.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Looks good to me:
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

[PATCH V2 16/20] block: move elv_register[unregister]_queue out of elevator_lock

[Ming Lei]

Move elv_register[unregister]_queue out of ->elevator_lock & queue freezing,
so we can kill many lockdep warnings.

elv_register[unregister]_queue() is serialized, and just dealing with sysfs/
debugfs things, no need to be done with queue frozen.

With this change, elevator's ->exit() is called before calling
elv_unregister_queue, then user may call into ->show()/store() of elevator's
sysfs attributes, and we have covered this issue by adding `ELEVATOR_FLAG_DYNG`.

For blk-mq debugfs, hctx->sched_tags is always checked with ->elevator_lock by
debugfs code, meantime hctx->sched_tags is updated with ->elevator_lock, so
there isn't such issue.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c   |  9 ++++----
 block/blk.h      |  1 +
 block/elevator.c | 58 ++++++++++++++++++++++++++++++++++--------------
 block/elevator.h |  5 +++++
 4 files changed, 52 insertions(+), 21 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 1a287c2e791c..9a361a173a8e 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4993,16 +4993,17 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 			.force	= true,
 			.uevent	= true,
 		};
+		int ret = -ENODEV;
 
 		mutex_lock(&q->elevator_lock);
 		if (q->elevator && !blk_queue_dying(q))
 			ctx.name = q->elevator->type->elevator_name;
-		__elevator_change(q, &ctx);
+		ret = __elevator_change(q, &ctx);
 		mutex_unlock(&q->elevator_lock);
-	}
-
-	list_for_each_entry(q, &set->tag_list, tag_set_list)
 		blk_mq_unfreeze_queue_nomemrestore(q);
+		if (!ret)
+			WARN_ON_ONCE(elevator_change_done(q, &ctx));
+	}
 	memalloc_noio_restore(memflags);
 
 	/* Free the excess tags when nr_hw_queues shrink. */
diff --git a/block/blk.h b/block/blk.h
index 0e19c09009ed..48cf6b1c36fe 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -323,6 +323,7 @@ bool blk_insert_flush(struct request *rq);
 int __elevator_change(struct request_queue *q, struct elv_change_ctx *ctx);
 void elevator_set_default(struct request_queue *q);
 void elevator_set_none(struct request_queue *q);
+int elevator_change_done(struct request_queue *q, struct elv_change_ctx *ctx);
 
 ssize_t part_size_show(struct device *dev, struct device_attribute *attr,
 		char *buf);
diff --git a/block/elevator.c b/block/elevator.c
index 16171ea92f80..8652fe45a2db 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -151,18 +151,24 @@ static void elevator_release(struct kobject *kobj)
 	kfree(e);
 }
 
-static void elevator_exit(struct request_queue *q)
+static void __elevator_exit(struct request_queue *q)
 {
 	struct elevator_queue *e = q->elevator;
 
+	lockdep_assert_held(&q->elevator_lock);
+
 	ioc_clear_queue(q);
 	blk_mq_sched_free_rqs(q);
 
 	mutex_lock(&e->sysfs_lock);
 	blk_mq_exit_sched(q, e);
 	mutex_unlock(&e->sysfs_lock);
+}
 
-	kobject_put(&e->kobj);
+static void elevator_exit(struct request_queue *q)
+{
+	__elevator_exit(q);
+	kobject_put(&q->elevator->kobj);
 }
 
 static inline void __elv_rqhash_del(struct request *rq)
@@ -467,8 +473,6 @@ static int elv_register_queue(struct request_queue *q,
 {
 	int error;
 
-	lockdep_assert_held(&q->elevator_lock);
-
 	error = kobject_add(&e->kobj, &q->disk->queue_kobj, "iosched");
 	if (!error) {
 		const struct elv_fs_entry *attr = e->type->elevator_attrs;
@@ -495,8 +499,6 @@ static int elv_register_queue(struct request_queue *q,
 static void elv_unregister_queue(struct request_queue *q,
 				 struct elevator_queue *e)
 {
-	lockdep_assert_held(&q->elevator_lock);
-
 	if (e && test_and_clear_bit(ELEVATOR_FLAG_REGISTERED, &e->flags)) {
 		kobject_uevent(&e->kobj, KOBJ_REMOVE);
 		kobject_del(&e->kobj);
@@ -640,19 +642,15 @@ static int elevator_switch(struct request_queue *q, struct elevator_type *new_e,
 	blk_mq_quiesce_queue(q);
 
 	if (q->elevator) {
-		elv_unregister_queue(q, q->elevator);
-		elevator_exit(q);
+		ctx->old = q->elevator;
+		__elevator_exit(q);
 	}
 
 	ret = blk_mq_init_sched(q, new_e);
 	if (ret)
 		goto out_unfreeze;
 
-	ret = elv_register_queue(q, q->elevator, ctx->uevent);
-	if (ret) {
-		elevator_exit(q);
-		goto out_unfreeze;
-	}
+	ctx->new = q->elevator;
 	blk_add_trace_msg(q, "elv switch: %s", new_e->elevator_name);
 
 out_unfreeze:
@@ -666,15 +664,16 @@ static int elevator_switch(struct request_queue *q, struct elevator_type *new_e,
 	return ret;
 }
 
-static void elevator_disable(struct request_queue *q)
+static void elevator_disable(struct request_queue *q,
+			     struct elv_change_ctx *ctx)
 {
 	WARN_ON_ONCE(q->mq_freeze_depth == 0);
 	lockdep_assert_held(&q->elevator_lock);
 
 	blk_mq_quiesce_queue(q);
 
-	elv_unregister_queue(q, q->elevator);
-	elevator_exit(q);
+	ctx->old = q->elevator;
+	__elevator_exit(q);
 	blk_queue_flag_clear(QUEUE_FLAG_SQ_SCHED, q);
 	q->elevator = NULL;
 	q->nr_requests = q->tag_set->queue_depth;
@@ -683,6 +682,28 @@ static void elevator_disable(struct request_queue *q)
 	blk_mq_unquiesce_queue(q);
 }
 
+int elevator_change_done(struct request_queue *q, struct elv_change_ctx *ctx)
+{
+	int ret = 0;
+
+	if (ctx->old) {
+		elv_unregister_queue(q, ctx->old);
+		kobject_put(&ctx->old->kobj);
+	}
+	if (ctx->new) {
+		ret = elv_register_queue(q, ctx->new, ctx->uevent);
+		if (ret) {
+			unsigned memflags = blk_mq_freeze_queue(q);
+
+			mutex_lock(&q->elevator_lock);
+			elevator_exit(q);
+			mutex_unlock(&q->elevator_lock);
+			blk_mq_unfreeze_queue(q, memflags);
+		}
+	}
+	return 0;
+}
+
 /*
  * Switch this queue to the given IO scheduler.
  */
@@ -698,7 +719,7 @@ int __elevator_change(struct request_queue *q, struct elv_change_ctx *ctx)
 
 	if (!strncmp(elevator_name, "none", 4)) {
 		if (q->elevator)
-			elevator_disable(q);
+			elevator_disable(q, ctx);
 		return 0;
 	}
 
@@ -742,6 +763,9 @@ static int elevator_change(struct request_queue *q,
 	ret = __elevator_change(q, ctx);
 	mutex_unlock(&q->elevator_lock);
 	blk_mq_unfreeze_queue(q, memflags);
+	if (!ret)
+		ret = elevator_change_done(q, ctx);
+
 exit:
 	srcu_read_unlock(&set->update_nr_hwq_srcu, idx);
 	return ret;
diff --git a/block/elevator.h b/block/elevator.h
index 16d8888fa2b2..486be0690499 100644
--- a/block/elevator.h
+++ b/block/elevator.h
@@ -129,6 +129,11 @@ struct elv_change_ctx {
 	bool force;
 	bool uevent;
 	bool init;
+
+	/* for unregistering old elevator */
+	struct elevator_queue *old;
+	/* for registering new elevator */
+	struct elevator_queue *new;
 };
 
 /*
-- 
2.47.0

Re: [PATCH V2 16/20] block: move elv_register[unregister]_queue out of elevator_lock

[Nilay Shroff]

On 4/18/25 10:06 PM, Ming Lei wrote:
> Move elv_register[unregister]_queue out of ->elevator_lock & queue freezing,
> so we can kill many lockdep warnings.
> 
> elv_register[unregister]_queue() is serialized, and just dealing with sysfs/
> debugfs things, no need to be done with queue frozen.
> 
> With this change, elevator's ->exit() is called before calling
> elv_unregister_queue, then user may call into ->show()/store() of elevator's
> sysfs attributes, and we have covered this issue by adding `ELEVATOR_FLAG_DYNG`.
> 
> For blk-mq debugfs, hctx->sched_tags is always checked with ->elevator_lock by
> debugfs code, meantime hctx->sched_tags is updated with ->elevator_lock, so
> there isn't such issue.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq.c   |  9 ++++----
>  block/blk.h      |  1 +
>  block/elevator.c | 58 ++++++++++++++++++++++++++++++++++--------------
>  block/elevator.h |  5 +++++
>  4 files changed, 52 insertions(+), 21 deletions(-)
> 

[...]

> +static void elevator_exit(struct request_queue *q)
> +{
> +	__elevator_exit(q);
> +	kobject_put(&q->elevator->kobj);
>  }

[...]  
> +int elevator_change_done(struct request_queue *q, struct elv_change_ctx *ctx)
> +{
> +	int ret = 0;
> +
> +	if (ctx->old) {
> +		elv_unregister_queue(q, ctx->old);
> +		kobject_put(&ctx->old->kobj);
> +	}
> +	if (ctx->new) {
> +		ret = elv_register_queue(q, ctx->new, ctx->uevent);
> +		if (ret) {
> +			unsigned memflags = blk_mq_freeze_queue(q);
> +
> +			mutex_lock(&q->elevator_lock);
> +			elevator_exit(q);
> +			mutex_unlock(&q->elevator_lock);
> +			blk_mq_unfreeze_queue(q, memflags);
> +		}
> +	}
> +	return 0;
> +}
> +
It seems that we're still leaking ->elevator_lock in sysfs/kernfs with
the above elevator_exit call. I think we probably want to move out 
kobject_put(&q->elevator->kobj) from elevator_exit and invoke it 
after we release ->elevator_lock in elevator_change_done.

Thanks,
--Nilay

Re: [PATCH V2 16/20] block: move elv_register[unregister]_queue out of elevator_lock

[Ming Lei]

On Sat, Apr 19, 2025 at 07:25:31PM +0530, Nilay Shroff wrote:
> 
> 
> On 4/18/25 10:06 PM, Ming Lei wrote:
> > Move elv_register[unregister]_queue out of ->elevator_lock & queue freezing,
> > so we can kill many lockdep warnings.
> > 
> > elv_register[unregister]_queue() is serialized, and just dealing with sysfs/
> > debugfs things, no need to be done with queue frozen.
> > 
> > With this change, elevator's ->exit() is called before calling
> > elv_unregister_queue, then user may call into ->show()/store() of elevator's
> > sysfs attributes, and we have covered this issue by adding `ELEVATOR_FLAG_DYNG`.
> > 
> > For blk-mq debugfs, hctx->sched_tags is always checked with ->elevator_lock by
> > debugfs code, meantime hctx->sched_tags is updated with ->elevator_lock, so
> > there isn't such issue.
> > 
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  block/blk-mq.c   |  9 ++++----
> >  block/blk.h      |  1 +
> >  block/elevator.c | 58 ++++++++++++++++++++++++++++++++++--------------
> >  block/elevator.h |  5 +++++
> >  4 files changed, 52 insertions(+), 21 deletions(-)
> > 
> 
> [...]
> 
> > +static void elevator_exit(struct request_queue *q)
> > +{
> > +	__elevator_exit(q);
> > +	kobject_put(&q->elevator->kobj);
> >  }
> 
> [...]  
> > +int elevator_change_done(struct request_queue *q, struct elv_change_ctx *ctx)
> > +{
> > +	int ret = 0;
> > +
> > +	if (ctx->old) {
> > +		elv_unregister_queue(q, ctx->old);
> > +		kobject_put(&ctx->old->kobj);
> > +	}
> > +	if (ctx->new) {
> > +		ret = elv_register_queue(q, ctx->new, ctx->uevent);
> > +		if (ret) {
> > +			unsigned memflags = blk_mq_freeze_queue(q);
> > +
> > +			mutex_lock(&q->elevator_lock);
> > +			elevator_exit(q);
> > +			mutex_unlock(&q->elevator_lock);
> > +			blk_mq_unfreeze_queue(q, memflags);
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> It seems that we're still leaking ->elevator_lock in sysfs/kernfs with
> the above elevator_exit call. I think we probably want to move out 
> kobject_put(&q->elevator->kobj) from elevator_exit and invoke it 
> after we release ->elevator_lock in elevator_change_done.

q->elevator_lock is owned by request queue instead of elevator queue, so it
shouldn't be one issue, or can you explain in details?

Thanks,
Ming

Re: [PATCH V2 16/20] block: move elv_register[unregister]_queue out of elevator_lock

[Nilay Shroff]

On 4/22/25 4:23 PM, Ming Lei wrote:
> On Sat, Apr 19, 2025 at 07:25:31PM +0530, Nilay Shroff wrote:
>>
>>
>> On 4/18/25 10:06 PM, Ming Lei wrote:
>>> Move elv_register[unregister]_queue out of ->elevator_lock & queue freezing,
>>> so we can kill many lockdep warnings.
>>>
>>> elv_register[unregister]_queue() is serialized, and just dealing with sysfs/
>>> debugfs things, no need to be done with queue frozen.
>>>
>>> With this change, elevator's ->exit() is called before calling
>>> elv_unregister_queue, then user may call into ->show()/store() of elevator's
>>> sysfs attributes, and we have covered this issue by adding `ELEVATOR_FLAG_DYNG`.
>>>
>>> For blk-mq debugfs, hctx->sched_tags is always checked with ->elevator_lock by
>>> debugfs code, meantime hctx->sched_tags is updated with ->elevator_lock, so
>>> there isn't such issue.
>>>
>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>> ---
>>>  block/blk-mq.c   |  9 ++++----
>>>  block/blk.h      |  1 +
>>>  block/elevator.c | 58 ++++++++++++++++++++++++++++++++++--------------
>>>  block/elevator.h |  5 +++++
>>>  4 files changed, 52 insertions(+), 21 deletions(-)
>>>
>>
>> [...]
>>
>>> +static void elevator_exit(struct request_queue *q)
>>> +{
>>> +	__elevator_exit(q);
>>> +	kobject_put(&q->elevator->kobj);
>>>  }
>>
>> [...]  
>>> +int elevator_change_done(struct request_queue *q, struct elv_change_ctx *ctx)
>>> +{
>>> +	int ret = 0;
>>> +
>>> +	if (ctx->old) {
>>> +		elv_unregister_queue(q, ctx->old);
>>> +		kobject_put(&ctx->old->kobj);
>>> +	}
>>> +	if (ctx->new) {
>>> +		ret = elv_register_queue(q, ctx->new, ctx->uevent);
>>> +		if (ret) {
>>> +			unsigned memflags = blk_mq_freeze_queue(q);
>>> +
>>> +			mutex_lock(&q->elevator_lock);
>>> +			elevator_exit(q);
>>> +			mutex_unlock(&q->elevator_lock);
>>> +			blk_mq_unfreeze_queue(q, memflags);
>>> +		}
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>> It seems that we're still leaking ->elevator_lock in sysfs/kernfs with
>> the above elevator_exit call. I think we probably want to move out 
>> kobject_put(&q->elevator->kobj) from elevator_exit and invoke it 
>> after we release ->elevator_lock in elevator_change_done.
> 
> q->elevator_lock is owned by request queue instead of elevator queue, so it
> shouldn't be one issue, or can you explain in details?
> 
I meant that one of the objective of this patchset is to ensure that
we don't grab any block/queue lock and enter the kernefs/sysfs/debugfs 
code. However here in elevator_change_done() we invoke elevator_exit()
after grabbing q->elevator_lock. Now if we look at the definition of
elevator_exit() then it appears that we invoke kobject_put(&ctx->old->kobj)
from it. So that means that while holding q->elevator_lock we enter in 
kernfs/sysfs code.

So my point was to move kobject_put() out of the elevator_exit() and 
call it after we release q->elevator_lock, maybe something like shown
below:

elevator_change_done()
{
        ...
        ...
	if (ctx->new) {
		ret = elv_register_queue(q, ctx->new, ctx->uevent);
		if (ret) {
			unsigned memflags = blk_mq_freeze_queue(q);

			mutex_lock(&q->elevator_lock);
			__elevator_exit(q);
			mutex_unlock(&q->elevator_lock);

			kobject_put(&q->elevator->kobj);

			blk_mq_unfreeze_queue(q, memflags);
		}
	}    
}

Thanks,
--Nilay

Re: [PATCH V2 16/20] block: move elv_register[unregister]_queue out of elevator_lock

[Ming Lei]

On Tue, Apr 22, 2025 at 04:36:37PM +0530, Nilay Shroff wrote:
> 
> 
> On 4/22/25 4:23 PM, Ming Lei wrote:
> > On Sat, Apr 19, 2025 at 07:25:31PM +0530, Nilay Shroff wrote:
> >>
> >>
> >> On 4/18/25 10:06 PM, Ming Lei wrote:
> >>> Move elv_register[unregister]_queue out of ->elevator_lock & queue freezing,
> >>> so we can kill many lockdep warnings.
> >>>
> >>> elv_register[unregister]_queue() is serialized, and just dealing with sysfs/
> >>> debugfs things, no need to be done with queue frozen.
> >>>
> >>> With this change, elevator's ->exit() is called before calling
> >>> elv_unregister_queue, then user may call into ->show()/store() of elevator's
> >>> sysfs attributes, and we have covered this issue by adding `ELEVATOR_FLAG_DYNG`.
> >>>
> >>> For blk-mq debugfs, hctx->sched_tags is always checked with ->elevator_lock by
> >>> debugfs code, meantime hctx->sched_tags is updated with ->elevator_lock, so
> >>> there isn't such issue.
> >>>
> >>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> >>> ---
> >>>  block/blk-mq.c   |  9 ++++----
> >>>  block/blk.h      |  1 +
> >>>  block/elevator.c | 58 ++++++++++++++++++++++++++++++++++--------------
> >>>  block/elevator.h |  5 +++++
> >>>  4 files changed, 52 insertions(+), 21 deletions(-)
> >>>
> >>
> >> [...]
> >>
> >>> +static void elevator_exit(struct request_queue *q)
> >>> +{
> >>> +	__elevator_exit(q);
> >>> +	kobject_put(&q->elevator->kobj);
> >>>  }
> >>
> >> [...]  
> >>> +int elevator_change_done(struct request_queue *q, struct elv_change_ctx *ctx)
> >>> +{
> >>> +	int ret = 0;
> >>> +
> >>> +	if (ctx->old) {
> >>> +		elv_unregister_queue(q, ctx->old);
> >>> +		kobject_put(&ctx->old->kobj);
> >>> +	}
> >>> +	if (ctx->new) {
> >>> +		ret = elv_register_queue(q, ctx->new, ctx->uevent);
> >>> +		if (ret) {
> >>> +			unsigned memflags = blk_mq_freeze_queue(q);
> >>> +
> >>> +			mutex_lock(&q->elevator_lock);
> >>> +			elevator_exit(q);
> >>> +			mutex_unlock(&q->elevator_lock);
> >>> +			blk_mq_unfreeze_queue(q, memflags);
> >>> +		}
> >>> +	}
> >>> +	return 0;
> >>> +}
> >>> +
> >> It seems that we're still leaking ->elevator_lock in sysfs/kernfs with
> >> the above elevator_exit call. I think we probably want to move out 
> >> kobject_put(&q->elevator->kobj) from elevator_exit and invoke it 
> >> after we release ->elevator_lock in elevator_change_done.
> > 
> > q->elevator_lock is owned by request queue instead of elevator queue, so it
> > shouldn't be one issue, or can you explain in details?
> > 
> I meant that one of the objective of this patchset is to ensure that
> we don't grab any block/queue lock and enter the kernefs/sysfs/debugfs 
> code. However here in elevator_change_done() we invoke elevator_exit()
> after grabbing q->elevator_lock. Now if we look at the definition of
> elevator_exit() then it appears that we invoke kobject_put(&ctx->old->kobj)
> from it. So that means that while holding q->elevator_lock we enter in 
> kernfs/sysfs code.
> 
> So my point was to move kobject_put() out of the elevator_exit() and 
> call it after we release q->elevator_lock, maybe something like shown
> below:
> 
> elevator_change_done()
> {
>         ...
>         ...
> 	if (ctx->new) {
> 		ret = elv_register_queue(q, ctx->new, ctx->uevent);
> 		if (ret) {
> 			unsigned memflags = blk_mq_freeze_queue(q);
> 
> 			mutex_lock(&q->elevator_lock);
> 			__elevator_exit(q);
> 			mutex_unlock(&q->elevator_lock);
> 
> 			kobject_put(&q->elevator->kobj);

This way is correct, but not necessary because elevator_release()
contributes nothing to the current lock problem.

However, this way aligns with normal code path wrt. elevator_lock &
freezing queue, and can save elevator_exit(), and will take it in V3.

Thanks,
Ming

[PATCH V2 17/20] block: move debugfs/sysfs register out of freezing queue

[Ming Lei]

Move debugfs/sysfs register out of freezing queue in
__blk_mq_update_nr_hw_queues(), so that the following lockdep dependency
can be killed:

	#2 (&q->q_usage_counter(io)#16){++++}-{0:0}:
	#1 (fs_reclaim){+.+.}-{0:0}:
	#0 (&sb->s_type->i_mutex_key#3){+.+.}-{4:4}: //debugfs

And registering/un-registering debugfs/sysfs does not require queue to be
frozen.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 9a361a173a8e..8d08127e40be 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4946,15 +4946,15 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 	if (set->nr_maps == 1 && nr_hw_queues == set->nr_hw_queues)
 		return;
 
-	memflags = memalloc_noio_save();
-	list_for_each_entry(q, &set->tag_list, tag_set_list)
-		blk_mq_freeze_queue_nomemsave(q);
-
 	list_for_each_entry(q, &set->tag_list, tag_set_list) {
 		blk_mq_debugfs_unregister_hctxs(q);
 		blk_mq_sysfs_unregister_hctxs(q);
 	}
 
+	memflags = memalloc_noio_save();
+	list_for_each_entry(q, &set->tag_list, tag_set_list)
+		blk_mq_freeze_queue_nomemsave(q);
+
 	if (blk_mq_realloc_tag_set_tags(set, nr_hw_queues) < 0)
 		goto reregister;
 
@@ -4977,12 +4977,6 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 		blk_mq_map_swqueue(q);
 	}
 
-reregister:
-	list_for_each_entry(q, &set->tag_list, tag_set_list) {
-		blk_mq_sysfs_register_hctxs(q);
-		blk_mq_debugfs_register_hctxs(q);
-	}
-
 	list_for_each_entry(q, &set->tag_list, tag_set_list) {
 		/*
 		 * nr_hw_queues is changed and elevator data depends on
@@ -5006,6 +5000,12 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 	}
 	memalloc_noio_restore(memflags);
 
+reregister:
+	list_for_each_entry(q, &set->tag_list, tag_set_list) {
+		blk_mq_sysfs_register_hctxs(q);
+		blk_mq_debugfs_register_hctxs(q);
+	}
+
 	/* Free the excess tags when nr_hw_queues shrink. */
 	for (i = set->nr_hw_queues; i < prev_nr_hw_queues; i++)
 		__blk_mq_free_map_and_rqs(set, i);
-- 
2.47.0

Re: [PATCH V2 17/20] block: move debugfs/sysfs register out of freezing queue

[Nilay Shroff]

On 4/18/25 10:06 PM, Ming Lei wrote:
> Move debugfs/sysfs register out of freezing queue in
> __blk_mq_update_nr_hw_queues(), so that the following lockdep dependency
> can be killed:
> 
> 	#2 (&q->q_usage_counter(io)#16){++++}-{0:0}:
> 	#1 (fs_reclaim){+.+.}-{0:0}:
> 	#0 (&sb->s_type->i_mutex_key#3){+.+.}-{4:4}: //debugfs
> 
> And registering/un-registering debugfs/sysfs does not require queue to be
> frozen.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Looks good to me:
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

[PATCH V2 18/20] block: remove several ->elevator_lock

[Ming Lei]

Both blk_mq_map_swqueue() and blk_mq_realloc_hw_ctxs() are called before
the request queue is added to tagset list, so the two won't run concurrently
with blk_mq_update_nr_hw_queues().

When the two functions are only called from queue initialization or
blk_mq_update_nr_hw_queues(), elevator switch can't happen.

So remove these ->elevator_lock uses.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 8d08127e40be..4de3287ce6e3 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4092,8 +4092,6 @@ static void blk_mq_map_swqueue(struct request_queue *q)
 	struct blk_mq_ctx *ctx;
 	struct blk_mq_tag_set *set = q->tag_set;
 
-	mutex_lock(&q->elevator_lock);
-
 	queue_for_each_hw_ctx(q, hctx, i) {
 		cpumask_clear(hctx->cpumask);
 		hctx->nr_ctx = 0;
@@ -4198,8 +4196,6 @@ static void blk_mq_map_swqueue(struct request_queue *q)
 		hctx->next_cpu = blk_mq_first_mapped_cpu(hctx);
 		hctx->next_cpu_batch = BLK_MQ_CPU_WORK_BATCH;
 	}
-
-	mutex_unlock(&q->elevator_lock);
 }
 
 /*
@@ -4503,16 +4499,9 @@ static void __blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
 }
 
 static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
-				   struct request_queue *q, bool lock)
+				   struct request_queue *q)
 {
-	if (lock) {
-		/* protect against switching io scheduler  */
-		mutex_lock(&q->elevator_lock);
-		__blk_mq_realloc_hw_ctxs(set, q);
-		mutex_unlock(&q->elevator_lock);
-	} else {
-		__blk_mq_realloc_hw_ctxs(set, q);
-	}
+	__blk_mq_realloc_hw_ctxs(set, q);
 
 	/* unregister cpuhp callbacks for exited hctxs */
 	blk_mq_remove_hw_queues_cpuhp(q);
@@ -4544,7 +4533,7 @@ int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
 
 	xa_init(&q->hctx_table);
 
-	blk_mq_realloc_hw_ctxs(set, q, false);
+	blk_mq_realloc_hw_ctxs(set, q);
 	if (!q->nr_hw_queues)
 		goto err_hctxs;
 
@@ -4961,7 +4950,7 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 fallback:
 	blk_mq_update_queue_map(set);
 	list_for_each_entry(q, &set->tag_list, tag_set_list) {
-		blk_mq_realloc_hw_ctxs(set, q, true);
+		blk_mq_realloc_hw_ctxs(set, q);
 
 		if (q->nr_hw_queues != set->nr_hw_queues) {
 			int i = prev_nr_hw_queues;
-- 
2.47.0

Re: [PATCH V2 18/20] block: remove several ->elevator_lock

[Nilay Shroff]

On 4/18/25 10:06 PM, Ming Lei wrote:
> Both blk_mq_map_swqueue() and blk_mq_realloc_hw_ctxs() are called before
> the request queue is added to tagset list, so the two won't run concurrently
> with blk_mq_update_nr_hw_queues().
> 
> When the two functions are only called from queue initialization or
> blk_mq_update_nr_hw_queues(), elevator switch can't happen.
> 
> So remove these ->elevator_lock uses.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Looks good to me:
Reviewed-by : Nilay Shroff <nilay@linux.ibm.com>

[PATCH V2 19/20] block: move hctx cpuhp add/del out of queue freezing

[Ming Lei]

Move hctx cpuhp add/del out of queue freezing for not connecting freeze
lock with cpuhp locks, then lockdep warning can be avoided.

This way is safe because both needn't queue to be frozen and scheduler
switch isn't allowed.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4de3287ce6e3..72f106163466 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4950,7 +4950,7 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 fallback:
 	blk_mq_update_queue_map(set);
 	list_for_each_entry(q, &set->tag_list, tag_set_list) {
-		blk_mq_realloc_hw_ctxs(set, q);
+		__blk_mq_realloc_hw_ctxs(set, q);
 
 		if (q->nr_hw_queues != set->nr_hw_queues) {
 			int i = prev_nr_hw_queues;
@@ -4993,6 +4993,9 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 	list_for_each_entry(q, &set->tag_list, tag_set_list) {
 		blk_mq_sysfs_register_hctxs(q);
 		blk_mq_debugfs_register_hctxs(q);
+
+		blk_mq_remove_hw_queues_cpuhp(q);
+		blk_mq_add_hw_queues_cpuhp(q);
 	}
 
 	/* Free the excess tags when nr_hw_queues shrink. */
-- 
2.47.0

Re: [PATCH V2 19/20] block: move hctx cpuhp add/del out of queue freezing

[Nilay Shroff]

On 4/18/25 10:07 PM, Ming Lei wrote:
> Move hctx cpuhp add/del out of queue freezing for not connecting freeze
> lock with cpuhp locks, then lockdep warning can be avoided.
> 
> This way is safe because both needn't queue to be frozen and scheduler
> switch isn't allowed.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Looks good to me:
Reviewed-by : Nilay Shroff <nilay@linux.ibm.com>

[PATCH V2 20/20] block: move wbt_enable_default() out of queue freezing from sched ->exit()

[Ming Lei]

scheduler's ->exit() is called with queue frozen and elevator lock is held, and
wbt_enable_default() can't be called with queue frozen, otherwise the
following lockdep warning is triggered:

	#6 (&q->rq_qos_mutex){+.+.}-{4:4}:
	#5 (&eq->sysfs_lock){+.+.}-{4:4}:
	#4 (&q->elevator_lock){+.+.}-{4:4}:
	#3 (&q->q_usage_counter(io)#3){++++}-{0:0}:
	#2 (fs_reclaim){+.+.}-{0:0}:
	#1 (&sb->s_type->i_mutex_key#3){+.+.}-{4:4}:
	#0 (&q->debugfs_mutex){+.+.}-{4:4}:

Fix the issue by moving wbt_enable_default() out of bfq's exit(), and
call it from elevator_change_done().

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/bfq-iosched.c | 2 +-
 block/elevator.c    | 5 +++++
 block/elevator.h    | 1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 40e4106a71e7..310ce1d8c41e 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -7211,7 +7211,7 @@ static void bfq_exit_queue(struct elevator_queue *e)
 
 	blk_stat_disable_accounting(bfqd->queue);
 	blk_queue_flag_clear(QUEUE_FLAG_DISABLE_WBT, bfqd->queue);
-	wbt_enable_default(bfqd->queue->disk);
+	set_bit(ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT, &e->flags);
 
 	kfree(bfqd);
 }
diff --git a/block/elevator.c b/block/elevator.c
index 8652fe45a2db..378553fce5d8 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -687,8 +687,13 @@ int elevator_change_done(struct request_queue *q, struct elv_change_ctx *ctx)
 	int ret = 0;
 
 	if (ctx->old) {
+		bool enable_wbt = test_bit(ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT,
+				&ctx->old->flags);
+
 		elv_unregister_queue(q, ctx->old);
 		kobject_put(&ctx->old->kobj);
+		if (enable_wbt)
+			wbt_enable_default(q->disk);
 	}
 	if (ctx->new) {
 		ret = elv_register_queue(q, ctx->new, ctx->uevent);
diff --git a/block/elevator.h b/block/elevator.h
index 486be0690499..b14c611c74b6 100644
--- a/block/elevator.h
+++ b/block/elevator.h
@@ -122,6 +122,7 @@ struct elevator_queue
 
 #define ELEVATOR_FLAG_REGISTERED	0
 #define ELEVATOR_FLAG_DYING		1
+#define ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT	2
 
 /* Holding context data for changing elevator */
 struct elv_change_ctx {
-- 
2.47.0

Re: [PATCH V2 20/20] block: move wbt_enable_default() out of queue freezing from sched ->exit()

[Nilay Shroff]

On 4/18/25 10:07 PM, Ming Lei wrote:
> scheduler's ->exit() is called with queue frozen and elevator lock is held, and
> wbt_enable_default() can't be called with queue frozen, otherwise the
> following lockdep warning is triggered:
> 
> 	#6 (&q->rq_qos_mutex){+.+.}-{4:4}:
> 	#5 (&eq->sysfs_lock){+.+.}-{4:4}:
> 	#4 (&q->elevator_lock){+.+.}-{4:4}:
> 	#3 (&q->q_usage_counter(io)#3){++++}-{0:0}:
> 	#2 (fs_reclaim){+.+.}-{0:0}:
> 	#1 (&sb->s_type->i_mutex_key#3){+.+.}-{4:4}:
> 	#0 (&q->debugfs_mutex){+.+.}-{4:4}:
> 
> Fix the issue by moving wbt_enable_default() out of bfq's exit(), and
> call it from elevator_change_done().
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/bfq-iosched.c | 2 +-
>  block/elevator.c    | 5 +++++
>  block/elevator.h    | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 40e4106a71e7..310ce1d8c41e 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -7211,7 +7211,7 @@ static void bfq_exit_queue(struct elevator_queue *e)
>  
>  	blk_stat_disable_accounting(bfqd->queue);
>  	blk_queue_flag_clear(QUEUE_FLAG_DISABLE_WBT, bfqd->queue);
> -	wbt_enable_default(bfqd->queue->disk);
> +	set_bit(ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT, &e->flags);
>  
>  	kfree(bfqd);
>  }
> diff --git a/block/elevator.c b/block/elevator.c
> index 8652fe45a2db..378553fce5d8 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -687,8 +687,13 @@ int elevator_change_done(struct request_queue *q, struct elv_change_ctx *ctx)
>  	int ret = 0;
>  
>  	if (ctx->old) {
> +		bool enable_wbt = test_bit(ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT,
> +				&ctx->old->flags);
> +
>  		elv_unregister_queue(q, ctx->old);
>  		kobject_put(&ctx->old->kobj);
> +		if (enable_wbt)
> +			wbt_enable_default(q->disk);
>  	}
>  	if (ctx->new) {
>  		ret = elv_register_queue(q, ctx->new, ctx->uevent);
> diff --git a/block/elevator.h b/block/elevator.h
> index 486be0690499..b14c611c74b6 100644
> --- a/block/elevator.h
> +++ b/block/elevator.h
> @@ -122,6 +122,7 @@ struct elevator_queue
>  
>  #define ELEVATOR_FLAG_REGISTERED	0
>  #define ELEVATOR_FLAG_DYING		1
> +#define ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT	2
>  
>  /* Holding context data for changing elevator */
>  struct elv_change_ctx {

It seems invoking wbt_enable_default from elevator_change_done could probably
still race with ioc_qos_write or queue_wb_lat_store. Both ioc_qos_write and 
queue_wb_lat_store run with ->freeze_lock and ->elevator_lock protection.

Thanks,
--Nilay

Re: [PATCH V2 20/20] block: move wbt_enable_default() out of queue freezing from sched ->exit()

[Ming Lei]

On Sat, Apr 19, 2025 at 08:09:04PM +0530, Nilay Shroff wrote:
> 
> 
> On 4/18/25 10:07 PM, Ming Lei wrote:
> > scheduler's ->exit() is called with queue frozen and elevator lock is held, and
> > wbt_enable_default() can't be called with queue frozen, otherwise the
> > following lockdep warning is triggered:
> > 
> > 	#6 (&q->rq_qos_mutex){+.+.}-{4:4}:
> > 	#5 (&eq->sysfs_lock){+.+.}-{4:4}:
> > 	#4 (&q->elevator_lock){+.+.}-{4:4}:
> > 	#3 (&q->q_usage_counter(io)#3){++++}-{0:0}:
> > 	#2 (fs_reclaim){+.+.}-{0:0}:
> > 	#1 (&sb->s_type->i_mutex_key#3){+.+.}-{4:4}:
> > 	#0 (&q->debugfs_mutex){+.+.}-{4:4}:
> > 
> > Fix the issue by moving wbt_enable_default() out of bfq's exit(), and
> > call it from elevator_change_done().
> > 
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  block/bfq-iosched.c | 2 +-
> >  block/elevator.c    | 5 +++++
> >  block/elevator.h    | 1 +
> >  3 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> > index 40e4106a71e7..310ce1d8c41e 100644
> > --- a/block/bfq-iosched.c
> > +++ b/block/bfq-iosched.c
> > @@ -7211,7 +7211,7 @@ static void bfq_exit_queue(struct elevator_queue *e)
> >  
> >  	blk_stat_disable_accounting(bfqd->queue);
> >  	blk_queue_flag_clear(QUEUE_FLAG_DISABLE_WBT, bfqd->queue);
> > -	wbt_enable_default(bfqd->queue->disk);
> > +	set_bit(ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT, &e->flags);
> >  
> >  	kfree(bfqd);
> >  }
> > diff --git a/block/elevator.c b/block/elevator.c
> > index 8652fe45a2db..378553fce5d8 100644
> > --- a/block/elevator.c
> > +++ b/block/elevator.c
> > @@ -687,8 +687,13 @@ int elevator_change_done(struct request_queue *q, struct elv_change_ctx *ctx)
> >  	int ret = 0;
> >  
> >  	if (ctx->old) {
> > +		bool enable_wbt = test_bit(ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT,
> > +				&ctx->old->flags);
> > +
> >  		elv_unregister_queue(q, ctx->old);
> >  		kobject_put(&ctx->old->kobj);
> > +		if (enable_wbt)
> > +			wbt_enable_default(q->disk);
> >  	}
> >  	if (ctx->new) {
> >  		ret = elv_register_queue(q, ctx->new, ctx->uevent);
> > diff --git a/block/elevator.h b/block/elevator.h
> > index 486be0690499..b14c611c74b6 100644
> > --- a/block/elevator.h
> > +++ b/block/elevator.h
> > @@ -122,6 +122,7 @@ struct elevator_queue
> >  
> >  #define ELEVATOR_FLAG_REGISTERED	0
> >  #define ELEVATOR_FLAG_DYING		1
> > +#define ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT	2
> >  
> >  /* Holding context data for changing elevator */
> >  struct elv_change_ctx {
> 
> It seems invoking wbt_enable_default from elevator_change_done could probably
> still race with ioc_qos_write or queue_wb_lat_store. Both ioc_qos_write and 
> queue_wb_lat_store run with ->freeze_lock and ->elevator_lock protection.

Actually wbt_enable_default() and wbt_init() needn't the above protection,
especially since the patch 2/20 removes q->elevator use in
wbt_enable_default().

Thanks,
Ming

Re: [PATCH V2 20/20] block: move wbt_enable_default() out of queue freezing from sched ->exit()

[Nilay Shroff]

On 4/21/25 12:57 PM, Ming Lei wrote:
> On Sat, Apr 19, 2025 at 08:09:04PM +0530, Nilay Shroff wrote:
>>
>>
>> On 4/18/25 10:07 PM, Ming Lei wrote:
>>> scheduler's ->exit() is called with queue frozen and elevator lock is held, and
>>> wbt_enable_default() can't be called with queue frozen, otherwise the
>>> following lockdep warning is triggered:
>>>
>>> 	#6 (&q->rq_qos_mutex){+.+.}-{4:4}:
>>> 	#5 (&eq->sysfs_lock){+.+.}-{4:4}:
>>> 	#4 (&q->elevator_lock){+.+.}-{4:4}:
>>> 	#3 (&q->q_usage_counter(io)#3){++++}-{0:0}:
>>> 	#2 (fs_reclaim){+.+.}-{0:0}:
>>> 	#1 (&sb->s_type->i_mutex_key#3){+.+.}-{4:4}:
>>> 	#0 (&q->debugfs_mutex){+.+.}-{4:4}:
>>>
>>> Fix the issue by moving wbt_enable_default() out of bfq's exit(), and
>>> call it from elevator_change_done().
>>>
>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>> ---
>>>  block/bfq-iosched.c | 2 +-
>>>  block/elevator.c    | 5 +++++
>>>  block/elevator.h    | 1 +
>>>  3 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
>>> index 40e4106a71e7..310ce1d8c41e 100644
>>> --- a/block/bfq-iosched.c
>>> +++ b/block/bfq-iosched.c
>>> @@ -7211,7 +7211,7 @@ static void bfq_exit_queue(struct elevator_queue *e)
>>>  
>>>  	blk_stat_disable_accounting(bfqd->queue);
>>>  	blk_queue_flag_clear(QUEUE_FLAG_DISABLE_WBT, bfqd->queue);
>>> -	wbt_enable_default(bfqd->queue->disk);
>>> +	set_bit(ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT, &e->flags);
>>>  
>>>  	kfree(bfqd);
>>>  }
>>> diff --git a/block/elevator.c b/block/elevator.c
>>> index 8652fe45a2db..378553fce5d8 100644
>>> --- a/block/elevator.c
>>> +++ b/block/elevator.c
>>> @@ -687,8 +687,13 @@ int elevator_change_done(struct request_queue *q, struct elv_change_ctx *ctx)
>>>  	int ret = 0;
>>>  
>>>  	if (ctx->old) {
>>> +		bool enable_wbt = test_bit(ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT,
>>> +				&ctx->old->flags);
>>> +
>>>  		elv_unregister_queue(q, ctx->old);
>>>  		kobject_put(&ctx->old->kobj);
>>> +		if (enable_wbt)
>>> +			wbt_enable_default(q->disk);
>>>  	}
>>>  	if (ctx->new) {
>>>  		ret = elv_register_queue(q, ctx->new, ctx->uevent);
>>> diff --git a/block/elevator.h b/block/elevator.h
>>> index 486be0690499..b14c611c74b6 100644
>>> --- a/block/elevator.h
>>> +++ b/block/elevator.h
>>> @@ -122,6 +122,7 @@ struct elevator_queue
>>>  
>>>  #define ELEVATOR_FLAG_REGISTERED	0
>>>  #define ELEVATOR_FLAG_DYING		1
>>> +#define ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT	2
>>>  
>>>  /* Holding context data for changing elevator */
>>>  struct elv_change_ctx {
>>
>> It seems invoking wbt_enable_default from elevator_change_done could probably
>> still race with ioc_qos_write or queue_wb_lat_store. Both ioc_qos_write and 
>> queue_wb_lat_store run with ->freeze_lock and ->elevator_lock protection.
> 
> Actually wbt_enable_default() and wbt_init() needn't the above protection,
> especially since the patch 2/20 removes q->elevator use in
> wbt_enable_default().
> 
Yes agreed, and as I understand XXX_FLAG_DISABLE_WBT was earlier elevator_queue->flags 
but now (with patch 2/20) it has been moved to request_queue->flags. As elevator_change_done 
first puts elevator_queue object which would potentially releases/frees the  elevator_queue 
object. Next while we enable wbt (in elevator_change_done)  we may not have access to the 
elevator_queue object and so now we reference  QUEUE_FLAG_DISABLE_WBT using request_queue->flags. 
That's, I believe, the purpose of patch 2/20.

However even with patch 2/20 change, both elevator_change_done and ioc_qos_write or
queue_wb_lat_store may run in parallel, isn't it?

therad1:
blk_mq_update_nr_hw_queues
  -> __blk_mq_update_nr_hw_queues
    -> elevator_change_done
      -> wbt_enable_default
        -> wbt_init
         -> wbt_update_limits

therad2:
queue_wb_lat_store
  -> wbt_set_min_lat
   -> wbt_update_limits

Thanks,
--Nilay


 

Re: [PATCH V2 20/20] block: move wbt_enable_default() out of queue freezing from sched ->exit()

[Ming Lei]

On Tue, Apr 22, 2025 at 11:44:59AM +0530, Nilay Shroff wrote:
> 
> 
> On 4/21/25 12:57 PM, Ming Lei wrote:
> > On Sat, Apr 19, 2025 at 08:09:04PM +0530, Nilay Shroff wrote:
> >>
> >>
> >> On 4/18/25 10:07 PM, Ming Lei wrote:
> >>> scheduler's ->exit() is called with queue frozen and elevator lock is held, and
> >>> wbt_enable_default() can't be called with queue frozen, otherwise the
> >>> following lockdep warning is triggered:
> >>>
> >>> 	#6 (&q->rq_qos_mutex){+.+.}-{4:4}:
> >>> 	#5 (&eq->sysfs_lock){+.+.}-{4:4}:
> >>> 	#4 (&q->elevator_lock){+.+.}-{4:4}:
> >>> 	#3 (&q->q_usage_counter(io)#3){++++}-{0:0}:
> >>> 	#2 (fs_reclaim){+.+.}-{0:0}:
> >>> 	#1 (&sb->s_type->i_mutex_key#3){+.+.}-{4:4}:
> >>> 	#0 (&q->debugfs_mutex){+.+.}-{4:4}:
> >>>
> >>> Fix the issue by moving wbt_enable_default() out of bfq's exit(), and
> >>> call it from elevator_change_done().
> >>>
> >>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> >>> ---
> >>>  block/bfq-iosched.c | 2 +-
> >>>  block/elevator.c    | 5 +++++
> >>>  block/elevator.h    | 1 +
> >>>  3 files changed, 7 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> >>> index 40e4106a71e7..310ce1d8c41e 100644
> >>> --- a/block/bfq-iosched.c
> >>> +++ b/block/bfq-iosched.c
> >>> @@ -7211,7 +7211,7 @@ static void bfq_exit_queue(struct elevator_queue *e)
> >>>  
> >>>  	blk_stat_disable_accounting(bfqd->queue);
> >>>  	blk_queue_flag_clear(QUEUE_FLAG_DISABLE_WBT, bfqd->queue);
> >>> -	wbt_enable_default(bfqd->queue->disk);
> >>> +	set_bit(ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT, &e->flags);
> >>>  
> >>>  	kfree(bfqd);
> >>>  }
> >>> diff --git a/block/elevator.c b/block/elevator.c
> >>> index 8652fe45a2db..378553fce5d8 100644
> >>> --- a/block/elevator.c
> >>> +++ b/block/elevator.c
> >>> @@ -687,8 +687,13 @@ int elevator_change_done(struct request_queue *q, struct elv_change_ctx *ctx)
> >>>  	int ret = 0;
> >>>  
> >>>  	if (ctx->old) {
> >>> +		bool enable_wbt = test_bit(ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT,
> >>> +				&ctx->old->flags);
> >>> +
> >>>  		elv_unregister_queue(q, ctx->old);
> >>>  		kobject_put(&ctx->old->kobj);
> >>> +		if (enable_wbt)
> >>> +			wbt_enable_default(q->disk);
> >>>  	}
> >>>  	if (ctx->new) {
> >>>  		ret = elv_register_queue(q, ctx->new, ctx->uevent);
> >>> diff --git a/block/elevator.h b/block/elevator.h
> >>> index 486be0690499..b14c611c74b6 100644
> >>> --- a/block/elevator.h
> >>> +++ b/block/elevator.h
> >>> @@ -122,6 +122,7 @@ struct elevator_queue
> >>>  
> >>>  #define ELEVATOR_FLAG_REGISTERED	0
> >>>  #define ELEVATOR_FLAG_DYING		1
> >>> +#define ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT	2
> >>>  
> >>>  /* Holding context data for changing elevator */
> >>>  struct elv_change_ctx {
> >>
> >> It seems invoking wbt_enable_default from elevator_change_done could probably
> >> still race with ioc_qos_write or queue_wb_lat_store. Both ioc_qos_write and 
> >> queue_wb_lat_store run with ->freeze_lock and ->elevator_lock protection.
> > 
> > Actually wbt_enable_default() and wbt_init() needn't the above protection,
> > especially since the patch 2/20 removes q->elevator use in
> > wbt_enable_default().
> > 
> Yes agreed, and as I understand XXX_FLAG_DISABLE_WBT was earlier elevator_queue->flags 
> but now (with patch 2/20) it has been moved to request_queue->flags. As elevator_change_done 
> first puts elevator_queue object which would potentially releases/frees the  elevator_queue 
> object. Next while we enable wbt (in elevator_change_done)  we may not have access to the 
> elevator_queue object and so now we reference  QUEUE_FLAG_DISABLE_WBT using request_queue->flags. 
> That's, I believe, the purpose of patch 2/20.
> 
> However even with patch 2/20 change, both elevator_change_done and ioc_qos_write or
> queue_wb_lat_store may run in parallel, isn't it?
> 
> therad1:
> blk_mq_update_nr_hw_queues
>   -> __blk_mq_update_nr_hw_queues
>     -> elevator_change_done
>       -> wbt_enable_default
>         -> wbt_init
>          -> wbt_update_limits

Here wbt_update_limits() is called on un-attached `struct rq_wb` instance,
which is just allocated from heap.

> 
> therad2:
> queue_wb_lat_store
>   -> wbt_set_min_lat
>    -> wbt_update_limits

The above one is run on attached `struct rq_wb` instance.

And there can only be one attached `struct rq_wb` instance, so the above
race doesn't exist because attaching wbt to queue/disk is covered by `q->rq_qos_mutex`.


Thanks,
Ming

Re: [PATCH V2 20/20] block: move wbt_enable_default() out of queue freezing from sched ->exit()

[Nilay Shroff]

On 4/22/25 1:43 PM, Ming Lei wrote:
> On Tue, Apr 22, 2025 at 11:44:59AM +0530, Nilay Shroff wrote:
>>
>>
>> On 4/21/25 12:57 PM, Ming Lei wrote:
>>> On Sat, Apr 19, 2025 at 08:09:04PM +0530, Nilay Shroff wrote:
>>>>
>>>>
>>>> On 4/18/25 10:07 PM, Ming Lei wrote:
>>>>> scheduler's ->exit() is called with queue frozen and elevator lock is held, and
>>>>> wbt_enable_default() can't be called with queue frozen, otherwise the
>>>>> following lockdep warning is triggered:
>>>>>
>>>>> 	#6 (&q->rq_qos_mutex){+.+.}-{4:4}:
>>>>> 	#5 (&eq->sysfs_lock){+.+.}-{4:4}:
>>>>> 	#4 (&q->elevator_lock){+.+.}-{4:4}:
>>>>> 	#3 (&q->q_usage_counter(io)#3){++++}-{0:0}:
>>>>> 	#2 (fs_reclaim){+.+.}-{0:0}:
>>>>> 	#1 (&sb->s_type->i_mutex_key#3){+.+.}-{4:4}:
>>>>> 	#0 (&q->debugfs_mutex){+.+.}-{4:4}:
>>>>>
>>>>> Fix the issue by moving wbt_enable_default() out of bfq's exit(), and
>>>>> call it from elevator_change_done().
>>>>>
>>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>>>> ---
>>>>>  block/bfq-iosched.c | 2 +-
>>>>>  block/elevator.c    | 5 +++++
>>>>>  block/elevator.h    | 1 +
>>>>>  3 files changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
>>>>> index 40e4106a71e7..310ce1d8c41e 100644
>>>>> --- a/block/bfq-iosched.c
>>>>> +++ b/block/bfq-iosched.c
>>>>> @@ -7211,7 +7211,7 @@ static void bfq_exit_queue(struct elevator_queue *e)
>>>>>  
>>>>>  	blk_stat_disable_accounting(bfqd->queue);
>>>>>  	blk_queue_flag_clear(QUEUE_FLAG_DISABLE_WBT, bfqd->queue);
>>>>> -	wbt_enable_default(bfqd->queue->disk);
>>>>> +	set_bit(ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT, &e->flags);
>>>>>  
>>>>>  	kfree(bfqd);
>>>>>  }
>>>>> diff --git a/block/elevator.c b/block/elevator.c
>>>>> index 8652fe45a2db..378553fce5d8 100644
>>>>> --- a/block/elevator.c
>>>>> +++ b/block/elevator.c
>>>>> @@ -687,8 +687,13 @@ int elevator_change_done(struct request_queue *q, struct elv_change_ctx *ctx)
>>>>>  	int ret = 0;
>>>>>  
>>>>>  	if (ctx->old) {
>>>>> +		bool enable_wbt = test_bit(ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT,
>>>>> +				&ctx->old->flags);
>>>>> +
>>>>>  		elv_unregister_queue(q, ctx->old);
>>>>>  		kobject_put(&ctx->old->kobj);
>>>>> +		if (enable_wbt)
>>>>> +			wbt_enable_default(q->disk);
>>>>>  	}
>>>>>  	if (ctx->new) {
>>>>>  		ret = elv_register_queue(q, ctx->new, ctx->uevent);
>>>>> diff --git a/block/elevator.h b/block/elevator.h
>>>>> index 486be0690499..b14c611c74b6 100644
>>>>> --- a/block/elevator.h
>>>>> +++ b/block/elevator.h
>>>>> @@ -122,6 +122,7 @@ struct elevator_queue
>>>>>  
>>>>>  #define ELEVATOR_FLAG_REGISTERED	0
>>>>>  #define ELEVATOR_FLAG_DYING		1
>>>>> +#define ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT	2
>>>>>  
>>>>>  /* Holding context data for changing elevator */
>>>>>  struct elv_change_ctx {
>>>>
>>>> It seems invoking wbt_enable_default from elevator_change_done could probably
>>>> still race with ioc_qos_write or queue_wb_lat_store. Both ioc_qos_write and 
>>>> queue_wb_lat_store run with ->freeze_lock and ->elevator_lock protection.
>>>
>>> Actually wbt_enable_default() and wbt_init() needn't the above protection,
>>> especially since the patch 2/20 removes q->elevator use in
>>> wbt_enable_default().
>>>
>> Yes agreed, and as I understand XXX_FLAG_DISABLE_WBT was earlier elevator_queue->flags 
>> but now (with patch 2/20) it has been moved to request_queue->flags. As elevator_change_done 
>> first puts elevator_queue object which would potentially releases/frees the  elevator_queue 
>> object. Next while we enable wbt (in elevator_change_done)  we may not have access to the 
>> elevator_queue object and so now we reference  QUEUE_FLAG_DISABLE_WBT using request_queue->flags. 
>> That's, I believe, the purpose of patch 2/20.
>>
>> However even with patch 2/20 change, both elevator_change_done and ioc_qos_write or
>> queue_wb_lat_store may run in parallel, isn't it?
>>
>> therad1:
>> blk_mq_update_nr_hw_queues
>>   -> __blk_mq_update_nr_hw_queues
>>     -> elevator_change_done
>>       -> wbt_enable_default
>>         -> wbt_init
>>          -> wbt_update_limits
> 
> Here wbt_update_limits() is called on un-attached `struct rq_wb` instance,
> which is just allocated from heap.
> 
>>
>> therad2:
>> queue_wb_lat_store
>>   -> wbt_set_min_lat
>>    -> wbt_update_limits
> 
> The above one is run on attached `struct rq_wb` instance.
> 
> And there can only be one attached `struct rq_wb` instance, so the above
> race doesn't exist because attaching wbt to queue/disk is covered by `q->rq_qos_mutex`.
> 
Yes you were correct, however, what if throttling is already enabled/attached to 
the queue? In that case we'd race updating rq_wb->enable_state, no? For instance,

thread1:
blk_mq_update_nr_hw_queues
  -> elevator_change_done
    -> wbt_enable_default ==> (updates ->enable_state)

thread2: 
queue_wb_lat_store
  -> wbt_set_min_lat ==> (updates ->enable_state)

thread3:
ioc_qos_write
  -> wbt_disable_default ==> (updates ->enable_state)

Thanks,
--Nilay

Re: [PATCH V2 20/20] block: move wbt_enable_default() out of queue freezing from sched ->exit()

[Ming Lei]

On Tue, Apr 22, 2025 at 02:57:01PM +0530, Nilay Shroff wrote:
> 
> 
> On 4/22/25 1:43 PM, Ming Lei wrote:
> > On Tue, Apr 22, 2025 at 11:44:59AM +0530, Nilay Shroff wrote:
> >>
> >>
> >> On 4/21/25 12:57 PM, Ming Lei wrote:
> >>> On Sat, Apr 19, 2025 at 08:09:04PM +0530, Nilay Shroff wrote:
> >>>>
> >>>>
> >>>> On 4/18/25 10:07 PM, Ming Lei wrote:
> >>>>> scheduler's ->exit() is called with queue frozen and elevator lock is held, and
> >>>>> wbt_enable_default() can't be called with queue frozen, otherwise the
> >>>>> following lockdep warning is triggered:
> >>>>>
> >>>>> 	#6 (&q->rq_qos_mutex){+.+.}-{4:4}:
> >>>>> 	#5 (&eq->sysfs_lock){+.+.}-{4:4}:
> >>>>> 	#4 (&q->elevator_lock){+.+.}-{4:4}:
> >>>>> 	#3 (&q->q_usage_counter(io)#3){++++}-{0:0}:
> >>>>> 	#2 (fs_reclaim){+.+.}-{0:0}:
> >>>>> 	#1 (&sb->s_type->i_mutex_key#3){+.+.}-{4:4}:
> >>>>> 	#0 (&q->debugfs_mutex){+.+.}-{4:4}:
> >>>>>
> >>>>> Fix the issue by moving wbt_enable_default() out of bfq's exit(), and
> >>>>> call it from elevator_change_done().
> >>>>>
> >>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> >>>>> ---
> >>>>>  block/bfq-iosched.c | 2 +-
> >>>>>  block/elevator.c    | 5 +++++
> >>>>>  block/elevator.h    | 1 +
> >>>>>  3 files changed, 7 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> >>>>> index 40e4106a71e7..310ce1d8c41e 100644
> >>>>> --- a/block/bfq-iosched.c
> >>>>> +++ b/block/bfq-iosched.c
> >>>>> @@ -7211,7 +7211,7 @@ static void bfq_exit_queue(struct elevator_queue *e)
> >>>>>  
> >>>>>  	blk_stat_disable_accounting(bfqd->queue);
> >>>>>  	blk_queue_flag_clear(QUEUE_FLAG_DISABLE_WBT, bfqd->queue);
> >>>>> -	wbt_enable_default(bfqd->queue->disk);
> >>>>> +	set_bit(ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT, &e->flags);
> >>>>>  
> >>>>>  	kfree(bfqd);
> >>>>>  }
> >>>>> diff --git a/block/elevator.c b/block/elevator.c
> >>>>> index 8652fe45a2db..378553fce5d8 100644
> >>>>> --- a/block/elevator.c
> >>>>> +++ b/block/elevator.c
> >>>>> @@ -687,8 +687,13 @@ int elevator_change_done(struct request_queue *q, struct elv_change_ctx *ctx)
> >>>>>  	int ret = 0;
> >>>>>  
> >>>>>  	if (ctx->old) {
> >>>>> +		bool enable_wbt = test_bit(ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT,
> >>>>> +				&ctx->old->flags);
> >>>>> +
> >>>>>  		elv_unregister_queue(q, ctx->old);
> >>>>>  		kobject_put(&ctx->old->kobj);
> >>>>> +		if (enable_wbt)
> >>>>> +			wbt_enable_default(q->disk);
> >>>>>  	}
> >>>>>  	if (ctx->new) {
> >>>>>  		ret = elv_register_queue(q, ctx->new, ctx->uevent);
> >>>>> diff --git a/block/elevator.h b/block/elevator.h
> >>>>> index 486be0690499..b14c611c74b6 100644
> >>>>> --- a/block/elevator.h
> >>>>> +++ b/block/elevator.h
> >>>>> @@ -122,6 +122,7 @@ struct elevator_queue
> >>>>>  
> >>>>>  #define ELEVATOR_FLAG_REGISTERED	0
> >>>>>  #define ELEVATOR_FLAG_DYING		1
> >>>>> +#define ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT	2
> >>>>>  
> >>>>>  /* Holding context data for changing elevator */
> >>>>>  struct elv_change_ctx {
> >>>>
> >>>> It seems invoking wbt_enable_default from elevator_change_done could probably
> >>>> still race with ioc_qos_write or queue_wb_lat_store. Both ioc_qos_write and 
> >>>> queue_wb_lat_store run with ->freeze_lock and ->elevator_lock protection.
> >>>
> >>> Actually wbt_enable_default() and wbt_init() needn't the above protection,
> >>> especially since the patch 2/20 removes q->elevator use in
> >>> wbt_enable_default().
> >>>
> >> Yes agreed, and as I understand XXX_FLAG_DISABLE_WBT was earlier elevator_queue->flags 
> >> but now (with patch 2/20) it has been moved to request_queue->flags. As elevator_change_done 
> >> first puts elevator_queue object which would potentially releases/frees the  elevator_queue 
> >> object. Next while we enable wbt (in elevator_change_done)  we may not have access to the 
> >> elevator_queue object and so now we reference  QUEUE_FLAG_DISABLE_WBT using request_queue->flags. 
> >> That's, I believe, the purpose of patch 2/20.
> >>
> >> However even with patch 2/20 change, both elevator_change_done and ioc_qos_write or
> >> queue_wb_lat_store may run in parallel, isn't it?
> >>
> >> therad1:
> >> blk_mq_update_nr_hw_queues
> >>   -> __blk_mq_update_nr_hw_queues
> >>     -> elevator_change_done
> >>       -> wbt_enable_default
> >>         -> wbt_init
> >>          -> wbt_update_limits
> > 
> > Here wbt_update_limits() is called on un-attached `struct rq_wb` instance,
> > which is just allocated from heap.
> > 
> >>
> >> therad2:
> >> queue_wb_lat_store
> >>   -> wbt_set_min_lat
> >>    -> wbt_update_limits
> > 
> > The above one is run on attached `struct rq_wb` instance.
> > 
> > And there can only be one attached `struct rq_wb` instance, so the above
> > race doesn't exist because attaching wbt to queue/disk is covered by `q->rq_qos_mutex`.
> > 
> Yes you were correct, however, what if throttling is already enabled/attached to 
> the queue? In that case we'd race updating rq_wb->enable_state, no? For instance,
> 
> thread1:
> blk_mq_update_nr_hw_queues
>   -> elevator_change_done
>     -> wbt_enable_default ==> (updates ->enable_state)
> 
> thread2: 
> queue_wb_lat_store
>   -> wbt_set_min_lat ==> (updates ->enable_state)
> 
> thread3:
> ioc_qos_write
>   -> wbt_disable_default ==> (updates ->enable_state)

OK, that is one race, but should have been handled by one rqos dedicated
lock instead of ->elevator_lock.


Thanks,
Ming