Re: [PATCH] blk-mq: implement queue quiesce via percpu_ref for BLK_MQ_F_BLOCKING

[Chao Leng <lengchao@huawei.com>]

On 2020/8/26 1:38, Sagi Grimberg wrote:
> 
>>>>> Good, but I'd also won't want to get this without making sure the async
>>>>> quiesce works well on large number of namespaces (the reason why this
>>>>> is proposed in the first place). Not sure who is planning to do that...
>>>>
>>>> That can be added when async quiesce is done.
>>>
>>> Chao, are you looking into that? I'd really hate to find out we have an
>>> issue there post conversion...
>>
>> Now we config CONFIG_TREE_SRCU, the size of TREE_SRCU is too big. I
>> really appreciate the work of Ming.
>>
>> I review the patch, I think the patch may work well now, but do extra
>> works for exception scenario. Percpu_ref is not disigned for
>> serialization which read low cost. If we replace SRCU with percpu_ref,
>> the benefit is save memory for blocking queue, the price is limit future
>> changes or do more extra works.
>>
>> I do not think replace SRCU with percpu_ref is a good idea, because it's
>> hard to predict how much we'll lose.
> 
> Not sure I understand your point, can you clarify what is the poor
> design of percpu_ref and for which use-case?
> .
1.percpu_ref need introduce fail status for hctc_lock to avoid possible
long waits for synchronization, need do some extra work for failed hctx_lock .
Just like this which in the patch:
@@ -2057,11 +2051,14 @@ static void blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
  		struct request *rq, blk_qc_t *cookie)
  {
  	blk_status_t ret;
-	int srcu_idx;

  	might_sleep_if(hctx->flags & BLK_MQ_F_BLOCKING);

-	hctx_lock(hctx, &srcu_idx);
+	/* Insert request to queue in case of being quiesced */
+	if (!hctx_lock(hctx)) {
+		blk_mq_sched_insert_request(rq, false, false, false);
+		return;
+	}

Now is simple, the code can work well. If the logic gets complicated,
it's probably hard to handle.
For example: for some unkown reason, if we may introduce some mechanism(such
as check other flag or state) for dispatch or issue requests, we may need do
more extra works in the branch of failed hctx_lock. perhaps more seriously,
it may hard to be handled in this branch. If we need do like this
in __blk_mq_try_issue_directly.
	if (blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)) {
		run_queue = false;
		bypass_insert = false;
		goto insert;
+	} else if (blk_queue_xxx(q)) {
+		/*do some other things*/
+       }
Of course, there's a good chance it won't happen.