Re: [PATCH 1/3] nvme: split __nvme_submit_sync_cmd()

[Hannes Reinecke <hare@suse.de>]

On 2/9/23 06:33, Christoph Hellwig wrote:
> On Wed, Feb 08, 2023 at 04:17:18PM +0100, Hannes Reinecke wrote:
>> Split a __nvme_alloc_rq() function from __nvme_submit_sync_cmd()
>> to reduce the number of arguments.
> 
> But now everyone has to call both?
> 
Yes, that's the downside of it.

>>   6 files changed, 83 insertions(+), 45 deletions(-)
> 
> .. and the code is a lot longer.  So this doesn't really seem like
> much of a win?
>> +struct request *__nvme_alloc_rq(struct request_queue *q,
>> +				struct nvme_command *cmd, int qid,
>> +				blk_mq_req_flags_t flags)
> 
> Why the double underscore profix?  Why _rq instead of _request
> like blk_mq_alloc_request and nvme_init_request?
> 
Ok, will be renaming to nvme_alloc_request().

>>   {
>>   	struct request *req;
>>   
>>   	if (qid == NVME_QID_ANY)
>>   		req = blk_mq_alloc_request(q, nvme_req_op(cmd), flags);
>>   	else
>>   		req = blk_mq_alloc_request_hctx(q, nvme_req_op(cmd), flags,
>>   						qid - 1);
>> +	if (!IS_ERR(req))
>> +		nvme_init_request(req, cmd);
>>   
>> +	return req;
> 
> And I'd at very least split out the qid case as that is substantially
> different and only used in very specific places.
> 
>> + */
>> +int __nvme_submit_sync_cmd(struct request *req, union nvme_result *result,
>> +			   void *buffer, unsigned bufflen, int at_head)
>> +{
>> +	int ret;
>>   
>>   	if (buffer && bufflen) {
>> -		ret = blk_rq_map_kern(q, req, buffer, bufflen, GFP_KERNEL);
>> +		ret = blk_rq_map_kern(req->q, req, buffer, bufflen, GFP_KERNEL);
>>   		if (ret)
>>   			goto out;
> 
> This new __nvme_submit_sync_cmd now consumes the request, which
> is an odd calling conventions.
> 
> What do you think about:
> 
>   - passing the "union nvme_result *result" to nvme_execute_rq, and do the
>     conditional assignment to it there, where it fits along with all the
>     status management
>   - make blk_rq_map_kern handle a NULL kbuf gracefully instead of doing
>     that in a lot of the callers
>   - just open code __nvme_submit_sync_cmd using these building blocks.

Hmm. Let me see how this pans out.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman