Re: [PATCH V9 3/4] blk-mq: issue directly with bypass 'false' in blk_mq_sched_insert_requests

[jianchao.wang]

Hi Jens

On 12/6/18 12:30 AM, Jens Axboe wrote:
> On 12/5/18 12:44 AM, Jianchao Wang wrote:
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index fe92e52..0dfa269 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -1899,32 +1899,23 @@ blk_status_t blk_mq_request_issue_directly(struct request *rq, bool last)
>>  void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx,
>>  		struct list_head *list)
>>  {
>> +	blk_qc_t cookie = BLK_QC_T_INVALID;
>> +
> 
> I'm fine with adding this, but I think we need some sort of check for
> that not being a valid cookie. That isn't new, we really should have
> already.
> 
>>  	while (!list_empty(list)) {
>> -		blk_status_t ret;
>>  		struct request *rq = list_first_entry(list, struct request,
>>  				queuelist);
>>  
>> -		if (!blk_rq_can_direct_dispatch(rq))
>> -			break;
>> -
>>  		list_del_init(&rq->queuelist);
>> -		ret = blk_mq_request_issue_directly(rq, list_empty(list));
>> -		if (ret != BLK_STS_OK) {
>> -			if (ret == BLK_STS_RESOURCE ||
>> -					ret == BLK_STS_DEV_RESOURCE) {
>> -				list_add(&rq->queuelist, list);
>> -				break;
>> -			}
>> -			blk_mq_end_request(rq, ret);
>> -		}
>> +		blk_mq_try_issue_directly(hctx, rq, &cookie, false,
>> +				list_empty(list));
> 
> Indent the list_empty() one more tab, should be after the ( if possible.

Yes, I will do it

> 
>> -	 * If we didn't flush the entire list, we could have told
>> -	 * the driver there was more coming, but that turned out to
>> -	 * be a lie.
>> +	 * cookie is set to a valid value only when reqeust is issued successfully.
>> +	 * We only need to care about the last request's result, if it is inserted,
>> +	 * kick the hardware with commit_rqs hook.
> 
> reqeust -> request
> 
> Also lines are too long, limit to 80 chars please.

Yes, I will do it.

> 
> And why aren't we just using the list_empty() check like before, and not
> having to add the inval cookie value?

Because we use 'bypass == false' here, so blk_mq_try_issue_directly will take
over the request totally, so the request will always be removed from the list
and finally, the list must be empty.

There is another way to identify the result of blk_mq_try_issue_directly.
Currently,
for the 'bypass == true' case,
    it always return BLK_STS_OK,
for the 'bypass == false' case,
    it return the actual result, except for 'force == true' case
    where the request has to be inserted into hctx dispatch list
    and return a BLK_STS_OK.

We could let the 'bypass == true' case also return the actual result to
show what has been done in the blk_mq_try_issue_directly and thus we could
get the actual result of the last request.

Would you mind we handle it like this ?

>> -	if (!list_empty(list) && hctx->queue->mq_ops->commit_rqs)
>> +	if ((cookie == BLK_QC_T_INVALID) && hctx->queue->mq_ops->commit_rqs)
>>  		hctx->queue->mq_ops->commit_rqs(hctx);
> 
> Redundant parens around the cookie check.
> 

Yes.

Thanks
Jianchao