[PATCH V2 5/5] blk-mq: support concurrent queue quiesce/unquiesce

[Bart Van Assche]

On 9/30/21 5:56 AM, Ming Lei wrote:
> Turns out that blk_mq_freeze_queue() isn't stronger[1] than
> blk_mq_quiesce_queue() because dispatch may still be in-progress after
> queue is frozen, and in several cases, such as switching io scheduler,
> updating nr_requests & wbt latency, we still need to quiesce queue as a
> supplement of freezing queue.

Is there agreement about this? If not, how about leaving out the above from the
patch description?

> As we need to extend uses of blk_mq_quiesce_queue(), it is inevitable
> for us to need support nested quiesce, especially we can't let
> unquiesce happen when there is quiesce originated from other contexts.
> 
> This patch introduces q->mq_quiesce_depth to deal concurrent quiesce,
> and we only unquiesce queue when it is the last/outer-most one of all
> contexts.
> 
> One kernel panic issue has been reported[2] when running stress test on
> dm-mpath's updating nr_requests and suspending queue, and the similar
> issue should exist on almost all drivers which use quiesce/unquiesce.
> 
> [1] https://marc.info/?l=linux-block&m=150993988115872&w=2
> [2] https://listman.redhat.com/archives/dm-devel/2021-September/msg00189.html

Please share the call stack of the kernel oops fixed by [2] since that
call stack is not in the patch description.

> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 21bf4c3f0825..10f8a3d4e3a1 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -209,7 +209,12 @@ EXPORT_SYMBOL_GPL(blk_mq_unfreeze_queue);
>    */
>   void blk_mq_quiesce_queue_nowait(struct request_queue *q)
>   {
> -	blk_queue_flag_set(QUEUE_FLAG_QUIESCED, q);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&q->queue_lock, flags);
> +	if (!q->quiesce_depth++)
> +		blk_queue_flag_set(QUEUE_FLAG_QUIESCED, q);
> +	spin_unlock_irqrestore(&q->queue_lock, flags);
>   }
>   EXPORT_SYMBOL_GPL(blk_mq_quiesce_queue_nowait);

Consider using == 0 instead of ! to check whether or not quiesce_depth is
zero to improve code readability.

> @@ -250,10 +255,19 @@ EXPORT_SYMBOL_GPL(blk_mq_quiesce_queue);
>    */
>   void blk_mq_unquiesce_queue(struct request_queue *q)
>   {
> -	blk_queue_flag_clear(QUEUE_FLAG_QUIESCED, q);
> +	unsigned long flags;
> +	bool run_queue = false;
> +
> +	spin_lock_irqsave(&q->queue_lock, flags);
> +	if (q->quiesce_depth > 0 && !--q->quiesce_depth) {
> +		blk_queue_flag_clear(QUEUE_FLAG_QUIESCED, q);
> +		run_queue = true;
> +	}
> +	spin_unlock_irqrestore(&q->queue_lock, flags);
>   
>   	/* dispatch requests which are inserted during quiescing */
> -	blk_mq_run_hw_queues(q, true);
> +	if (run_queue)
> +		blk_mq_run_hw_queues(q, true);
>   }

So calling with blk_mq_unquiesce_queue() q->quiesce_depth <= 0 is ignored
quietly? How about triggering a kernel warning for that condition?

Otherwise the code changes look good to me.

Thanks,

Bart.