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

[Christoph Hellwig <hch@lst.de>]

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?  

>  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?

>  {
>  	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.