On 7/27/20 3:00 PM, Sagi Grimberg wrote:
>
>>>>>>> +void blk_mq_quiesce_queue_async(struct request_queue *q)
>>>>>>> +{
>>>>>>> + struct blk_mq_hw_ctx *hctx;
>>>>>>> + unsigned int i;
>>>>>>> +
>>>>>>> + blk_mq_quiesce_queue_nowait(q);
>>>>>>> +
>>>>>>> + queue_for_each_hw_ctx(q, hctx, i) {
>>>>>>> + init_completion(&hctx->rcu_sync.completion);
>>>>>>> + init_rcu_head(&hctx->rcu_sync.head);
>>>>>>> + if (hctx->flags & BLK_MQ_F_BLOCKING)
>>>>>>> + call_srcu(hctx->srcu, &hctx->rcu_sync.head,
>>>>>>> + wakeme_after_rcu);
>>>>>>> + else
>>>>>>> + call_rcu(&hctx->rcu_sync.head,
>>>>>>> + wakeme_after_rcu);
>>>>>>> + }
>>>>>>
>>>>>> Looks not necessary to do anything in case of !BLK_MQ_F_BLOCKING, and single
>>>>>> synchronize_rcu() is OK for all hctx during waiting.
>>>>>
>>>>> That's true, but I want a single interface for both. v2 had exactly
>>>>> that, but I decided that this approach is better.
>>>>
>>>> Not sure one new interface is needed, and one simple way is to:
>>>>
>>>> 1) call blk_mq_quiesce_queue_nowait() for each request queue
>>>>
>>>> 2) wait in driver specific way
>>>>
>>>> Or just wondering why nvme doesn't use set->tag_list to retrieve NS,
>>>> then you may add per-tagset APIs for the waiting.
>>>
>>> Because it puts assumptions on how quiesce works, which is something
>>> I'd like to avoid because I think its cleaner, what do others think?
>>> Jens? Christoph?
>>
>> I'd prefer to have it in a helper, and just have blk_mq_quiesce_queue()
>> call that.
>
> I agree with this approach as well.
>
> Jens, this mean that we use the call_rcu mechanism also for non-blocking
> hctxs, because the caller will call it for multiple request queues (see
> patch 2) and we don't want to call synchronize_rcu for every request
> queue serially, we want it to happen in parallel.
>
> Which leaves us with the patchset as it is, just to convert the
> rcu_synchronize structure to be dynamically allocated on the heap
> rather than keeping it statically allocated in the hctx.
>
> This is how it looks:
OK, so maybe I'm not fully up-to-date on the thread, but why aren't we
just having a blk_mq_quiesce_queue_wait() that does something ala:
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 667155f752f7..b4ceaaed2f9c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -209,32 +209,37 @@ void blk_mq_quiesce_queue_nowait(struct request_queue *q)
}
EXPORT_SYMBOL_GPL(blk_mq_quiesce_queue_nowait);
-/**
- * blk_mq_quiesce_queue() - wait until all ongoing dispatches have finished
- * @q: request queue.
- *
- * Note: this function does not prevent that the struct request end_io()
- * callback function is invoked. Once this function is returned, we make
- * sure no dispatch can happen until the queue is unquiesced via
- * blk_mq_unquiesce_queue().
- */
-void blk_mq_quiesce_queue(struct request_queue *q)
+void blk_mq_quiesce_queue_wait(struct request_queue *q)
{
struct blk_mq_hw_ctx *hctx;
unsigned int i;
bool rcu = false;
- blk_mq_quiesce_queue_nowait(q);
-
queue_for_each_hw_ctx(q, hctx, i) {
if (hctx->flags & BLK_MQ_F_BLOCKING)
synchronize_srcu(hctx->srcu);
else
rcu = true;
}
+
if (rcu)
synchronize_rcu();
}
+
+/**
+ * blk_mq_quiesce_queue() - wait until all ongoing dispatches have finished
+ * @q: request queue.
+ *
+ * Note: this function does not prevent that the struct request end_io()
+ * callback function is invoked. Once this function is returned, we make
+ * sure no dispatch can happen until the queue is unquiesced via
+ * blk_mq_unquiesce_queue().
+ */
+void blk_mq_quiesce_queue(struct request_queue *q)
+{
+ blk_mq_quiesce_queue_nowait(q);
+ blk_mq_quiesce_queue_wait(q);
+}
EXPORT_SYMBOL_GPL(blk_mq_quiesce_queue);
/*
Do we care about BLK_MQ_F_BLOCKING runtime at all?
--
Jens Axboe
