[PATCH 0/2] block: two fixes on recent elevator change

[PATCH 0/2] block: two fixes on recent elevator change

[Ming Lei]

Hello Jens,

The 1st patch fixes one hang in del_gendisk().

The 2nd patch fixes one race in case that add_disk() fails.

Thanks,
Ming


Ming Lei (2):
  block: don't quiesce queue for calling elevator_set_none()
  block: move removing elevator after deleting disk->queue_kobj

 block/blk-sysfs.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

-- 
2.47.0

[PATCH 1/2] block: don't quiesce queue for calling elevator_set_none()

[Ming Lei]

blk_mq_freeze_queue() can't be called on quiesced queue, otherwise it may
never return if there is any queued requests.

Fix it by removing quiesce queue around elevator_set_none() because
elevator_switch() does quiesce queue in case that we need to switch
to none really.

Fixes: 1e44bedbc921 ("block: unifying elevator change")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-sysfs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 386374ff655b..8be2390c3c19 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -948,11 +948,8 @@ void blk_unregister_queue(struct gendisk *disk)
 		blk_mq_sysfs_unregister(disk);
 	blk_crypto_sysfs_unregister(disk);
 
-	if (queue_is_mq(q)) {
-		blk_mq_quiesce_queue(q);
+	if (queue_is_mq(q))
 		elevator_set_none(q);
-		blk_mq_unquiesce_queue(q);
-	}
 
 	mutex_lock(&q->sysfs_lock);
 	disk_unregister_independent_access_ranges(disk);
-- 
2.47.0

[PATCH 2/2] block: move removing elevator after deleting disk->queue_kobj

[Ming Lei]

When blk_unregister_queue() is called from add_disk() failure path,
there is race in registering/unregistering elevator queue kobject
from the two code paths, because commit 559dc11143eb ("block: move
elv_register[unregister]_queue out of elevator_lock") moves elevator
queue register/unregister out of elevator lock.

Fix the race by removing elevator after deleting disk->queue_kobj,
because kobject_del(&disk->queue_kobj) drains in-progress sysfs
show()/store() of all attributes.

Fixes: 559dc11143eb ("block: move elv_register[unregister]_queue out of elevator_lock")
Reported-by: Nilay Shroff <nilay@linux.ibm.com>
Suggested-by: Nilay Shroff <nilay@linux.ibm.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-sysfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 8be2390c3c19..b2b9b89d6967 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -948,9 +948,6 @@ void blk_unregister_queue(struct gendisk *disk)
 		blk_mq_sysfs_unregister(disk);
 	blk_crypto_sysfs_unregister(disk);
 
-	if (queue_is_mq(q))
-		elevator_set_none(q);
-
 	mutex_lock(&q->sysfs_lock);
 	disk_unregister_independent_access_ranges(disk);
 	mutex_unlock(&q->sysfs_lock);
@@ -959,5 +956,8 @@ void blk_unregister_queue(struct gendisk *disk)
 	kobject_uevent(&disk->queue_kobj, KOBJ_REMOVE);
 	kobject_del(&disk->queue_kobj);
 
+	if (queue_is_mq(q))
+		elevator_set_none(q);
+
 	blk_debugfs_remove(disk);
 }
-- 
2.47.0

Re: [PATCH 1/2] block: don't quiesce queue for calling elevator_set_none()

[Nilay Shroff]

On 5/8/25 2:28 PM, Ming Lei wrote:
> blk_mq_freeze_queue() can't be called on quiesced queue, otherwise it may
> never return if there is any queued requests.
> 
> Fix it by removing quiesce queue around elevator_set_none() because
> elevator_switch() does quiesce queue in case that we need to switch
> to none really.
> 
> Fixes: 1e44bedbc921 ("block: unifying elevator change")
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Maybe you should add a link to the issue this commit fixed. I just
saw Shinichiro reproduced it here [1].

[1] https://lore.kernel.org/all/mlycu6p6zl5z5mmqau7otbfw35kcvnajpsnm3hokpfnafc3bwh@m5dp43ypdfpz/

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

Re: [PATCH 2/2] block: move removing elevator after deleting disk->queue_kobj

[Nilay Shroff]

On 5/8/25 2:28 PM, Ming Lei wrote:
> When blk_unregister_queue() is called from add_disk() failure path,
> there is race in registering/unregistering elevator queue kobject
> from the two code paths, because commit 559dc11143eb ("block: move
> elv_register[unregister]_queue out of elevator_lock") moves elevator
> queue register/unregister out of elevator lock.
> 
> Fix the race by removing elevator after deleting disk->queue_kobj,
> because kobject_del(&disk->queue_kobj) drains in-progress sysfs
> show()/store() of all attributes.
> 
> Fixes: 559dc11143eb ("block: move elv_register[unregister]_queue out of elevator_lock")
> Reported-by: Nilay Shroff <nilay@linux.ibm.com>
> Suggested-by: Nilay Shroff <nilay@linux.ibm.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

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

Re: [PATCH 1/2] block: don't quiesce queue for calling elevator_set_none()

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 2/2] block: move removing elevator after deleting disk->queue_kobj

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 0/2] block: two fixes on recent elevator change

[Jens Axboe]

On Thu, 08 May 2025 16:58:03 +0800, Ming Lei wrote:
> The 1st patch fixes one hang in del_gendisk().
> 
> The 2nd patch fixes one race in case that add_disk() fails.
> 
> Thanks,
> Ming
> 
> [...]

Applied, thanks!

[1/2] block: don't quiesce queue for calling elevator_set_none()
      commit: 8336d18c6b57a603aaa4db76bbf4f8cb08acfa5e
[2/2] block: move removing elevator after deleting disk->queue_kobj
      commit: 824afb9b04648ea11531fc9047923ec07e7a943d

Best regards,
-- 
Jens Axboe