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.
