On (24/10/06 23:10), Christoph Hellwig wrote: > On Fri, Oct 04, 2024 at 11:32:34PM +0900, Sergey Senozhatsky wrote: > > Hmm, setting QUEUE_FLAG_DYING unconditionally in __blk_mark_disk_dead() > > implies moving it up, to the very top of del_gendisk(), before the first > > time we grab ->open_mutex, because that's the issue that we are having. > > Does this sound like re-introducing the previous deadlock scenario (the > > one you pointed at previously) because of that "don't acquire ->open_mutex > > after freezing the queue" thing? > > So the trace of that one is literally the same as the one you reported, > and I'm still wondering how they are related (I hope Yang Yang can > chime in) > > I suspect that if we mark both the disk and queue dead > early that will error out everything and should fix it. Does the diff below look like something that you are thinking about? __blk_mark_disk_dead() cannot be moved alone, we need blk_report_disk_dead() before it. And all of these should be done before the first time we take ->open_mutex. I keep __blk_mark_disk_dead() the way it is and just forcibly set QUEUE_FLAG_DYING right before __blk_mark_disk_dead(), so that hopefully bio_queue_enter() can detect DYING. --- diff --git a/block/genhd.c b/block/genhd.c index 1c05dd4c6980..3b2a7e0f2176 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -649,6 +649,16 @@ void del_gendisk(struct gendisk *disk) disk_del_events(disk); + /* + * Tell the file system to write back all dirty data and shut down if + * it hasn't been notified earlier. + */ + if (!test_bit(GD_DEAD, &disk->state)) + blk_report_disk_dead(disk, false); + /* TODO: big fat comment */ + blk_queue_flag_set(QUEUE_FLAG_DYING, disk->queue); + __blk_mark_disk_dead(disk); + /* * Prevent new openers by unlinked the bdev inode. */ @@ -657,18 +667,10 @@ void del_gendisk(struct gendisk *disk) bdev_unhash(part); mutex_unlock(&disk->open_mutex); - /* - * Tell the file system to write back all dirty data and shut down if - * it hasn't been notified earlier. - */ - if (!test_bit(GD_DEAD, &disk->state)) - blk_report_disk_dead(disk, false); - /* * Drop all partitions now that the disk is marked dead. */ mutex_lock(&disk->open_mutex); - __blk_mark_disk_dead(disk); xa_for_each_start(&disk->part_tbl, idx, part, 1) drop_partition(part); mutex_unlock(&disk->open_mutex); @@ -714,6 +716,10 @@ void del_gendisk(struct gendisk *disk) rq_qos_exit(q); blk_mq_unquiesce_queue(q); + /* TODO: big fat comment */ + if (test_bit(GD_OWNS_QUEUE, &disk->state)) + blk_queue_flag_clear(QUEUE_FLAG_DYING, disk->queue); + /* * If the disk does not own the queue, allow using passthrough requests * again. Else leave the queue frozen to fail all I/O.
