Re: [PATCH V3 1/7] nvme-core: use I/O timeout in submit sync cmd

From: Christoph Hellwig <hch@lst.de>
To: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Cc: kbusch@kernel.org, hch@lst.de, linux-nvme@lists.infradead.org,
	sagi@grimberg.me
Subject: Re: [PATCH V3 1/7] nvme-core: use I/O timeout in submit sync cmd
Date: Wed, 16 Sep 2020 09:09:21 +0200	[thread overview]
Message-ID: <20200916070921.GA9592@lst.de> (raw)
In-Reply-To: <20200915215118.3934-2-chaitanya.kulkarni@wdc.com>

On Tue, Sep 15, 2020 at 02:51:12PM -0700, Chaitanya Kulkarni wrote:
> In the function __nvme_submit_sync_cmd() it uses ADMIN_TIMEOUT when
> caller doesn't specify value for the timeout variable. This function is
> also called from the NVMe commands contexts (nvme_pr_command()/
> nvme_ns_report_zones()) where NVME_IO_TIMEOUT can be used instead of
> ADMIN_TIMEOUT.
> 
> For now we don't set the request queue's queuedata for admin command.
> 
> Introduce a helper nvme_default_timeout() and when timeout is not set
> for the block layer request  based on the request queue's queuedata,
> set Admin timeout else I/O timeout.
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  drivers/nvme/host/core.c |  2 +-
>  drivers/nvme/host/nvme.h | 15 +++++++++++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 8b75f6ca0b61..0f878f5e07f0 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -885,7 +885,7 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
>  	if (IS_ERR(req))
>  		return PTR_ERR(req);
>  
> -	req->timeout = timeout ? timeout : ADMIN_TIMEOUT;
> +	nvme_default_timeout(req, timeout);
>  
>  	if (buffer && bufflen) {
>  		ret = blk_rq_map_kern(q, req, buffer, bufflen, GFP_KERNEL);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 9fd45ff656da..78dc422ee42c 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -638,6 +638,21 @@ struct nvme_ns *nvme_get_ns_from_disk(struct gendisk *disk,
>  		struct nvme_ns_head **head, int *srcu_idx);
>  void nvme_put_ns_from_disk(struct nvme_ns_head *head, int idx);
>  
> +static inline void nvme_default_timeout(struct request *req,
> +					unsigned int timeout)
> +{
> +	void *queuedata = req->q->queuedata;
> +
> +	/*
> +	 * For now admin request queue's queue data == NULL, if that assumption
> +	 * changes it should reflect here.
> +	 */
> +	if (!timeout)
> +		timeout = queuedata ? NVME_IO_TIMEOUT : ADMIN_TIMEOUT;
> +
> +	req->timeout = timeout;

The void *queuedata thing is really weird.

I'd keep the check for an existing timeout in the callers and just
write this as:

static inline void nvme_req_set_default_timeout(struct request *req)
{
	if (req->queuedata)
		req->timeout = NVME_IO_TIMEOUT
	else /* no queuedata implies admin queue */
		req->timeout = ADMIN_TIMEOUT;
}

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme