Re: block: del_gendisk() vs blk_queue_enter() race condition

[Sergey Senozhatsky <senozhatsky@chromium.org>]

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.