Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH V9 3/4] blk-mq: issue directly with bypass 'false' in
 blk_mq_sched_insert_requests
To:     Jens Axboe <axboe@kernel.dk>
Cc:     ming.lei@redhat.com, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <1543995842-20704-1-git-send-email-jianchao.w.wang@oracle.com>
 <1543995842-20704-4-git-send-email-jianchao.w.wang@oracle.com>
 <6cb12913-3575-d3aa-ff08-f89bcb38b3d6@kernel.dk>
From:   "jianchao.wang" <jianchao.w.wang@oracle.com>
Message-ID: <b57bbedf-bc7e-7b8d-2217-3e8628ab657c@oracle.com>
Date:   Thu, 6 Dec 2018 09:11:22 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <6cb12913-3575-d3aa-ff08-f89bcb38b3d6@kernel.dk>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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