On Fri, Oct 04, 2024 at 04:48:18PM +0900, Sergey Senozhatsky wrote:
> diff --git a/block/blk-core.c b/block/blk-core.c
> index bc5e8c5eaac9..ccd36cb5ada7 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -292,6 +292,16 @@ void blk_queue_start_drain(struct request_queue *q)
> wake_up_all(&q->mq_freeze_wq);
> }
>
> +void blk_queue_disk_dead(struct request_queue *q)
> +{
> + struct gendisk *disk = q->disk;
> +
> + if (WARN_ON_ONCE(!test_bit(GD_DEAD, &disk->state)))
> + return;
> + /* Make blk_queue_enter() reexamine the GD_DEAD flag. */
> + wake_up_all(&q->mq_freeze_wq);
> +}
Why is this a separate helper vs just doing the wake_up_all in the
only caller that sets (with the suggested fixup anyway) GD_DEAD?
> + blk_queue_dying(q) ||
> + test_bit(GD_DEAD, &disk->state));
This needs to check for a NULL disk. And now that I'm looking at the
code a bit more this makes me worried that checking for q->disk here
sounds like a good way to hit a race with clearing it. So I fear we
need the other hack variant that sets QUEUE_FLAG_DYING unconditionally
in __blk_mark_disk_dead and then clears it again (for GD_OWNS_QUEUE
only) toward the end of del_gendisk.
