Re: [PATCH 6/9] nvme: add support for batched completion of polled IO

[Jens Axboe <axboe@kernel.dk>]

On 10/14/21 1:43 AM, Christoph Hellwig wrote:
> On Wed, Oct 13, 2021 at 10:54:13AM -0600, Jens Axboe wrote:
>> +void nvme_complete_batch_req(struct request *req)
>> +{
>> +	nvme_cleanup_cmd(req);
>> +	nvme_end_req_zoned(req);
>> +	req->status = BLK_STS_OK;
>> +}
>> +EXPORT_SYMBOL_GPL(nvme_complete_batch_req);
>> +
> 
> I'd be tempted to just merge this helper into the only caller.
> nvme_cleanup_cmd is exported anyway, so this would just add an export
> for nvme_end_req_zoned.

Sure, I can do that.

>> +static __always_inline void nvme_complete_batch(struct io_batch *iob,
>> +						void (*fn)(struct request *rq))
>> +{
>> +	struct request *req;
>> +
>> +	req = rq_list_peek(&iob->req_list);
>> +	while (req) {
>> +		fn(req);
>> +		nvme_complete_batch_req(req);
>> +		req = rq_list_next(req);
>> +	}
>> +
>> +	blk_mq_end_request_batch(iob);
> 
> Can we turn this into a normal for loop?
> 
> 	for (req = rq_list_peek(&iob->req_list); req; req = rq_list_next(req)) {
> 		..
> 	}

If you prefer it that way for nvme, for me the while () setup is much
easier to read than a really long for line.

>> +	if (!nvme_try_complete_req(req, cqe->status, cqe->result)) {
>> +		/*
>> +		 * Do normal inline completion if we don't have a batch
>> +		 * list, if we have an end_io handler, or if the status of
>> +		 * the request isn't just normal success.
>> +		 */
>> +		if (!iob || req->end_io || nvme_req(req)->status)
>> +			nvme_pci_complete_rq(req);
>> +		else
>> +			rq_list_add_tail(&iob->req_list, req);
>> +	}
> 
> The check for the conditions where we can or cannot batch complete
> really should go into a block layer helper.  Something like the
> incremental patch below:

That's a good idea, I'll add that.

-- 
Jens Axboe