Re: [PATCH v3 1/2] blk-mq: add async quiesce interface

[Jens Axboe <axboe@kernel.dk>]

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


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme