Re: [PATCH 07/15] block: move blk_unregister_queue() & device_del() after freeze wait

Re: [PATCH 07/15] block: move blk_unregister_queue() & device_del() after freeze wait

[Ming Lei]

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

Sender: ming.lei@redhat.com
Subject: Re: [PATCH 07/15] block: move blk_unregister_queue() & device_del() after freeze wait
Message-Id: <Z_3D0PZWolXUdXnK@fedora>
Recipient: yangpanfei@honor.com

[-- Attachment #2: Type: message/rfc822, Size: 10971 bytes --]

From: Ming Lei <ming.lei@redhat.com>
To: Christoph Hellwig <hch@lst.de>
Cc: "Jens Axboe" <axboe@kernel.dk>, linux-block@vger.kernel.org, "Nilay Shroff" <nilay@linux.ibm.com>, "Shinichiro Kawasaki" <shinichiro.kawasaki@wdc.com>, "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
Subject: Re: [PATCH 07/15] block: move blk_unregister_queue() & device_del() after freeze wait
Date: Tue, 15 Apr 2025 10:26:24 +0800
Message-ID: <Z_3D0PZWolXUdXnK@fedora>

On Mon, Apr 14, 2025 at 08:19:10AM +0200, Christoph Hellwig wrote:
> On Thu, Apr 10, 2025 at 09:30:19PM +0800, 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.
> 
> While I believe this is the right thing to do, moving the freeze wait
> caused issues in the past, so be careful.  Take a look at:
> 
> commit 4c66a326b5ab784cddd72de07ac5b6210e9e1b06
> Author: Christoph Hellwig <hch@lst.de>
> Date:   Mon Sep 19 16:40:49 2022 +0200
> 
>     Revert "block: freeze the queue earlier in del_gendisk"

Yeah, I know this story.

If it is triggered again with this patch, I will help to root cause.

The last thing we still can do is to just moving blk_unregister_queue()
after queue is frozen, or even elevator tear down. It is allowed to delete
children kobjects after their parent is removed. Just the KOBJ_REMOVE
event need to be ordered.


Thanks,
Ming

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

[Ming Lei]

Hello Jens,

The 1st 11 patch 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").

The other 4 ones fixes lockdep warnings from bfq and updating nr_hw_queues
code.

Also the 4th patch fixes kasan warning & oops report from Shinichiro by
preventing elevator switching when updating nr_hw_queues is in-progress.

Thanks,

Ming Lei (15):
  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: prevent elevator switch during updating nr_hw_queues
  block: simplify elevator reset for updating nr_hw_queues
  block: add helper of elevator_change()
  block: move blk_unregister_queue() & device_del() after freeze wait
  block: add `struct elev_change_ctx` for unifying elevator change
  block: unifying elevator change
  block: pass elevator_queue to elv_register_queue & unregister_queue
  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
    scheduler's ->exit()

 block/bfq-iosched.c    |   2 +-
 block/blk-mq-debugfs.c |  13 +--
 block/blk-mq-sched.c   |  39 +++++---
 block/blk-mq.c         | 163 +++++++++---------------------
 block/blk-sysfs.c      |  18 ++--
 block/blk.h            |  10 +-
 block/elevator.c       | 221 ++++++++++++++++++++++++++---------------
 block/elevator.h       |  16 +++
 block/genhd.c          |  25 +----
 include/linux/blk-mq.h |  10 +-
 10 files changed, 256 insertions(+), 261 deletions(-)

-- 
2.47.0

[PATCH 07/15] 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 c2bd86cd09de..f426c13edf55 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -721,8 +721,6 @@ void del_gendisk(struct gendisk *disk)
 		bdi_unregister(disk->bdi);
 	}
 
-	blk_unregister_queue(disk);
-
 	kobject_put(disk->part0->bd_holder_dir);
 	kobject_put(disk->slave_dir);
 	disk->slave_dir = NULL;
@@ -731,10 +729,12 @@ void del_gendisk(struct gendisk *disk)
 	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 07/15] block: move blk_unregister_queue() & device_del() after freeze wait

[Christoph Hellwig]

On Thu, Apr 10, 2025 at 09:30:19PM +0800, 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.

While I believe this is the right thing to do, moving the freeze wait
caused issues in the past, so be careful.  Take a look at:

commit 4c66a326b5ab784cddd72de07ac5b6210e9e1b06
Author: Christoph Hellwig <hch@lst.de>
Date:   Mon Sep 19 16:40:49 2022 +0200

    Revert "block: freeze the queue earlier in del_gendisk"

Re: [PATCH 07/15] block: move blk_unregister_queue() & device_del() after freeze wait

[Ming Lei]

On Mon, Apr 14, 2025 at 08:19:10AM +0200, Christoph Hellwig wrote:
> On Thu, Apr 10, 2025 at 09:30:19PM +0800, 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.
> 
> While I believe this is the right thing to do, moving the freeze wait
> caused issues in the past, so be careful.  Take a look at:
> 
> commit 4c66a326b5ab784cddd72de07ac5b6210e9e1b06
> Author: Christoph Hellwig <hch@lst.de>
> Date:   Mon Sep 19 16:40:49 2022 +0200
> 
>     Revert "block: freeze the queue earlier in del_gendisk"

Yeah, I know this story.

If it is triggered again with this patch, I will help to root cause.

The last thing we still can do is to just moving blk_unregister_queue()
after queue is frozen, or even elevator tear down. It is allowed to delete
children kobjects after their parent is removed. Just the KOBJ_REMOVE
event need to be ordered.


Thanks,
Ming