[PATCH V3 2/6] nvme-core: split nvme_alloc_request()

[Chaitanya Kulkarni]

On 11/3/20 10:24, Christoph Hellwig wrote:
> On Wed, Oct 21, 2020 at 06:02:30PM -0700, Chaitanya Kulkarni wrote:
>> +static inline unsigned int nvme_req_op(struct nvme_command *cmd)
>> +{
>> +	return nvme_is_write(cmd) ? REQ_OP_DRV_OUT : REQ_OP_DRV_IN;
>> +}
> Why is this added here while nvme_init_req_from_cmd is added in a prep
> patch?  I'm actually fine either way, but doing it differnetly for the
> different helpers is a little inconsistent.

I'll move this into the first prep patch.

>> +
>> +struct request *nvme_alloc_request_qid_any(struct request_queue *q,
>> +		struct nvme_command *cmd, blk_mq_req_flags_t flags)
> I'd call this just nvme_alloc_request to keep the short name for the
> normal use case.
Okay.
>
>> +	struct request *req;
>> +
>> +	req = blk_mq_alloc_request(q, nvme_req_op(cmd), flags);
>> +	if (unlikely(IS_ERR(req)))
>> +		return req;
>> +
>> +	nvme_init_req_from_cmd(req, cmd);
>> +	return req;
> Could be simplified to:
>
> 	req = blk_mq_alloc_request(q, nvme_req_op(cmd), flags);
> 	if (!IS_ERR(req))
> 		nvme_init_req_from_cmd(req, cmd);
> 	return req;
>
> Note that IS_ERR already contains an embedded unlikely().
Sure.
>> +static struct request *nvme_alloc_request_qid(struct request_queue *q,
>>  		struct nvme_command *cmd, blk_mq_req_flags_t flags, int qid)
>>  {
>>  	struct request *req;
>>  
>> +	req = blk_mq_alloc_request_hctx(q, nvme_req_op(cmd), flags,
>> +			qid ? qid - 1 : 0);
>>  	if (IS_ERR(req))
>>  		return req;
>>  
>>  	nvme_init_req_from_cmd(req, cmd);
>>  	return req;
> Same here.
Will do.
>>  }
>> -EXPORT_SYMBOL_GPL(nvme_alloc_request);
> I think nvme_alloc_request_qid needs to be exported as well.
>
> FYI, this also doesn't apply to the current nvme-5.10 tree any more.
>
Since it conflicts with the timeout series will rebase and resend once
we get

the timeout series in, otherwise it makes reviews confusing and stale at
times.