Re: [PATCH V2 5/5] blk-mq: support concurrent queue quiesce/unquiesce
From: Bart Van Assche <bvanassche@acm.org>
Date: 2021-09-30 15:56:37
Also in:
linux-block
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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
@@ -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. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme