[PATCH V3 0/7] nvme-core: timeout related fixes and cleanup

[PATCH V3 0/7] nvme-core: timeout related fixes and cleanup

[Chaitanya Kulkarni]

Hi,

This patch series uses NVME_IO_TIMEOUT for the sync request submission
and user request submission when the timeout is not specified by the caller
and request queue data is set to NULL which is true for admin queue.

Also in this version, I've added timeout values setting for the NVMeOF
passthru controller given that passthru VUCs (both admin and I/O VUCs)
can differ in execution time than general-purpose admin and I/O command
set.

The last patch is just my personal preference, feel free to ignore it.

Regards,
Chaitanya

* Changes from V2:-

1. Introduce nvme_defalt_timeout() helper and use it in host/core.c.
2. Use nvme_default_timeout() in the lightnvme.c

* Changes from V1:-

1. Instead of using qid to decide IO or ADMIN timeout use request
   queue's queuedata whch we only set for non admin queue
   __nvme_submit_sync_cmd().
2. Add second patch to set IO timeout for nvme_submit_user_cmd().
3. Set the NVMeOF passthru ctrl timeout values with default values from
   nvme-core module.
4. Add admin and I/O timeout configfs attributes for NVMeOF passthru
   controller.

Chaitanya Kulkarni (7):
  nvme-core: use I/O timeout in submit sync cmd
  nvme-core: use I/O timeout in nvme_submit_user_cmd
  lightnvm: use I/O timeout in nvm submit user cmd
  nvmet: set default timeout for passthru requests
  nvmet: add passthru admin timeout value attr
  nvmet: add passthru io timeout value attr
  nvme: use consistent macro name for timeout

 drivers/nvme/host/core.c       |  6 +--
 drivers/nvme/host/fc.c         |  2 +-
 drivers/nvme/host/lightnvm.c   |  2 +-
 drivers/nvme/host/nvme.h       | 17 ++++++++-
 drivers/nvme/host/pci.c        |  8 ++--
 drivers/nvme/host/rdma.c       |  2 +-
 drivers/nvme/host/tcp.c        |  2 +-
 drivers/nvme/target/configfs.c | 68 ++++++++++++++++++++++++++++++++++
 drivers/nvme/target/loop.c     |  2 +-
 drivers/nvme/target/nvmet.h    |  2 +
 drivers/nvme/target/passthru.c | 10 +++++
 11 files changed, 108 insertions(+), 13 deletions(-)

-- 
2.22.1


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

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

[Chaitanya Kulkarni]

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;
+}
+
 extern const struct attribute_group *nvme_ns_id_attr_groups[];
 extern const struct block_device_operations nvme_ns_head_ops;
 
-- 
2.22.1


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

[PATCH V3 2/7] nvme-core: use I/O timeout in nvme_submit_user_cmd

[Chaitanya Kulkarni]

In the function nvme_submit_user_cmd() it uses ADMIN_TIMEOUT when
caller doesn't specify value for the timeout variable. This function is
also called from the user I/O command contexts (nvme_submit_io,
nvme_user_cmdXXX)) 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.

Use newly introduced helper function nvme_default_timeout() to set the
block layer request timeout value.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 0f878f5e07f0..41177a181c8b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1082,7 +1082,7 @@ static int nvme_submit_user_cmd(struct request_queue *q,
 	if (IS_ERR(req))
 		return PTR_ERR(req);
 
-	req->timeout = timeout ? timeout : ADMIN_TIMEOUT;
+	nvme_default_timeout(req, timeout);
 	nvme_req(req)->flags |= NVME_REQ_USERCMD;
 
 	if (ubuffer && bufflen) {
-- 
2.22.1


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

[PATCH V3 3/7] lightnvm: use I/O timeout in nvm submit user cmd

[Chaitanya Kulkarni]

In the function nvme_nvm_submit_user_cmd() it uses ADMIN_TIMEOUT when
caller doesn't specify value for the timeout variable.

For now we don't set the request queue's queuedata for admin command.

Use newly introduced helper function nvme_default_timeout() to set the
block layer request timeout value which sets the appropriate I/O or
Admin timeout values when caller doesn't specify the timeout value.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/lightnvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index 8e562d0f2c30..74ad5427846a 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -774,7 +774,7 @@ static int nvme_nvm_submit_user_cmd(struct request_queue *q,
 		goto err_cmd;
 	}
 
-	rq->timeout = timeout ? timeout : ADMIN_TIMEOUT;
+	nvme_default_timeout(rq, timeout);
 
 	if (ppa_buf && ppa_len) {
 		ppa_list = dma_pool_alloc(dev->dma_pool, GFP_KERNEL, &ppa_dma);
-- 
2.22.1


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

[PATCH V3 4/7] nvmet: set default timeout for passthru requests

[Chaitanya Kulkarni]

In nvmet_passthru_execute_cmd() we don't set the default timeout values
for passthru requests. Use the default values ro set the passthru
request timeout.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/passthru.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index 8bd7f656e240..53a3906bd913 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -242,6 +242,8 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 		goto out_put_ns;
 	}
 
+	rq->timeout = q->queuedata ? NVME_IO_TIMEOUT : ADMIN_TIMEOUT;
+
 	if (req->sg_cnt) {
 		ret = nvmet_passthru_map_sg(req, rq);
 		if (unlikely(ret)) {
-- 
2.22.1


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

[PATCH V3 5/7] nvmet: add passthru admin timeout value attr

[Chaitanya Kulkarni]

NVMeOF controller in the passsthru mode is capable of handling wide set
of admin commands including Vender unique passhtru admin comands (VUAC).

The VUACs are used to read the large driver logs and can take longer
than default NVMe commands, that is for passthru requests the timeout
value may differ from the passthru controller's default timeout values
(nvme-core:admin_timeout).

Add a configfs attribute so that user can set the admin timeout values.
In case if this configfs value is not set default to ADMIN_TIMEOUT
value.

This attribute setting is only allowed when ctrl is disable to avoid
rcu calls in the fast path, in future when needed we can always make it
fast path friendly using RCU.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/configfs.c | 34 ++++++++++++++++++++++++++++++++++
 drivers/nvme/target/nvmet.h    |  1 +
 drivers/nvme/target/passthru.c |  6 +++++-
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 37e1d7784e17..b3b57add53d1 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -736,7 +736,41 @@ static ssize_t nvmet_passthru_enable_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_passthru_, enable);
 
+static ssize_t nvmet_passthru_admin_timeout_show(struct config_item *item,
+		char *page)
+{
+	struct nvmet_subsys *subsys = to_subsys(item->ci_parent);
+
+	return sprintf(page, "%u\n", subsys->admin_timeout);
+}
+
+static ssize_t nvmet_passthru_admin_timeout_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item->ci_parent);
+	unsigned int admin_timeout;
+	int ret = 0;
+
+	mutex_lock(&subsys->lock);
+	if (subsys->passthru_ctrl) {
+		pr_err("disable passthru ctrl before setting admin_timeout\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (kstrtouint(page, 0, &admin_timeout)) {
+		ret = -EINVAL;
+		goto out;
+	}
+	subsys->admin_timeout = admin_timeout;
+out:
+	mutex_unlock(&subsys->lock);
+	return ret ? ret : count;
+}
+CONFIGFS_ATTR(nvmet_passthru_, admin_timeout);
+
 static struct configfs_attribute *nvmet_passthru_attrs[] = {
+	&nvmet_passthru_attr_admin_timeout,
 	&nvmet_passthru_attr_device_path,
 	&nvmet_passthru_attr_enable,
 	NULL,
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 47ee3fb193bd..13266e413ccb 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -249,6 +249,7 @@ struct nvmet_subsys {
 	struct nvme_ctrl	*passthru_ctrl;
 	char			*passthru_ctrl_path;
 	struct config_group	passthru_group;
+	unsigned int		admin_timeout;
 #endif /* CONFIG_NVME_TARGET_PASSTHRU */
 };
 
diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index 53a3906bd913..3a364a12e8c9 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -215,6 +215,7 @@ static int nvmet_passthru_map_sg(struct nvmet_req *req, struct request *rq)
 
 static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 {
+	unsigned int admin_timeout = req->sq->ctrl->subsys->admin_timeout;
 	struct nvme_ctrl *ctrl = nvmet_req_passthru_ctrl(req);
 	struct request_queue *q = ctrl->admin_q;
 	struct nvme_ns *ns = NULL;
@@ -242,7 +243,7 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 		goto out_put_ns;
 	}
 
-	rq->timeout = q->queuedata ? NVME_IO_TIMEOUT : ADMIN_TIMEOUT;
+	rq->timeout = q->queuedata ? NVME_IO_TIMEOUT : admin_timeout;
 
 	if (req->sg_cnt) {
 		ret = nvmet_passthru_map_sg(req, rq);
@@ -519,6 +520,9 @@ int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys)
 		subsys->ver = NVME_VS(1, 2, 1);
 	}
 
+	if (!subsys->admin_timeout)
+		subsys->admin_timeout = ADMIN_TIMEOUT;
+
 	mutex_unlock(&subsys->lock);
 	return 0;
 
-- 
2.22.1


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

[PATCH V3 6/7] nvmet: add passthru io timeout value attr

[Chaitanya Kulkarni]

NVMe controller in the passsthru mode is capable of handling wide set
of admin commands including Vender unique passhtru I/O comands
(VUICs).

The VUICs can take longer than default NVMe commands, that is for
passthru requests the timeout value may differ from the passthru
controller's default timeout values (nvme-core:io_timeout).

Add configfs attribute so that user can set the I/O timeout values.
In case if this configfs value is not set default to NVME_IO_TIMEOUT
value.

This attribute setting is only allowed when ctrl is disable to avoid
rcu calls in the fast path, in future when needed we can always make it
fast path friendly using RCU.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/configfs.c | 34 ++++++++++++++++++++++++++++++++++
 drivers/nvme/target/nvmet.h    |  1 +
 drivers/nvme/target/passthru.c |  6 +++++-
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index b3b57add53d1..506da13ac083 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -769,8 +769,42 @@ static ssize_t nvmet_passthru_admin_timeout_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_passthru_, admin_timeout);
 
+static ssize_t nvmet_passthru_io_timeout_show(struct config_item *item,
+		char *page)
+{
+	struct nvmet_subsys *subsys = to_subsys(item->ci_parent);
+
+	return sprintf(page, "%u\n", subsys->io_timeout);
+}
+
+static ssize_t nvmet_passthru_io_timeout_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item->ci_parent);
+	unsigned int io_timeout;
+	int ret = 0;
+
+	mutex_lock(&subsys->lock);
+	if (subsys->passthru_ctrl) {
+		pr_err("disable passthru ctrl before setting io_timeout\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (kstrtouint(page, 0, &io_timeout)) {
+		ret = -EINVAL;
+		goto out;
+	}
+	subsys->io_timeout = io_timeout;
+out:
+	mutex_unlock(&subsys->lock);
+	return ret ? ret : count;
+}
+CONFIGFS_ATTR(nvmet_passthru_, io_timeout);
+
 static struct configfs_attribute *nvmet_passthru_attrs[] = {
 	&nvmet_passthru_attr_admin_timeout,
+	&nvmet_passthru_attr_io_timeout,
 	&nvmet_passthru_attr_device_path,
 	&nvmet_passthru_attr_enable,
 	NULL,
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 13266e413ccb..a24b4c7a3ab4 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -250,6 +250,7 @@ struct nvmet_subsys {
 	char			*passthru_ctrl_path;
 	struct config_group	passthru_group;
 	unsigned int		admin_timeout;
+	unsigned int		io_timeout;
 #endif /* CONFIG_NVME_TARGET_PASSTHRU */
 };
 
diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index 3a364a12e8c9..2564b6769b9e 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -216,6 +216,7 @@ static int nvmet_passthru_map_sg(struct nvmet_req *req, struct request *rq)
 static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 {
 	unsigned int admin_timeout = req->sq->ctrl->subsys->admin_timeout;
+	unsigned int io_timeout = req->sq->ctrl->subsys->io_timeout;
 	struct nvme_ctrl *ctrl = nvmet_req_passthru_ctrl(req);
 	struct request_queue *q = ctrl->admin_q;
 	struct nvme_ns *ns = NULL;
@@ -243,7 +244,7 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 		goto out_put_ns;
 	}
 
-	rq->timeout = q->queuedata ? NVME_IO_TIMEOUT : admin_timeout;
+	rq->timeout = q->queuedata ? io_timeout : admin_timeout;
 
 	if (req->sg_cnt) {
 		ret = nvmet_passthru_map_sg(req, rq);
@@ -523,6 +524,9 @@ int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys)
 	if (!subsys->admin_timeout)
 		subsys->admin_timeout = ADMIN_TIMEOUT;
 
+	if (!subsys->io_timeout)
+		subsys->io_timeout = NVME_IO_TIMEOUT;
+
 	mutex_unlock(&subsys->lock);
 	return 0;
 
-- 
2.22.1


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

[PATCH V3 7/7] nvme: use consistent macro name for timeout

[Chaitanya Kulkarni]

This is purely a clenaup patch, add prefix NVME to the ADMIN_TIMEOUT
to make consistent with NVME_IO_TIMEOUT.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/core.c       | 2 +-
 drivers/nvme/host/fc.c         | 2 +-
 drivers/nvme/host/nvme.h       | 4 ++--
 drivers/nvme/host/pci.c        | 8 ++++----
 drivers/nvme/host/rdma.c       | 2 +-
 drivers/nvme/host/tcp.c        | 2 +-
 drivers/nvme/target/loop.c     | 2 +-
 drivers/nvme/target/passthru.c | 2 +-
 8 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 41177a181c8b..b86f13c137a2 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2327,7 +2327,7 @@ int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
 	cmd.common.cdw11 = cpu_to_le32(len);
 
 	return __nvme_submit_sync_cmd(ctrl->admin_q, &cmd, NULL, buffer, len,
-				      ADMIN_TIMEOUT, NVME_QID_ANY, 1, 0, false);
+				      NVME_ADMIN_TIMEOUT, NVME_QID_ANY, 1, 0, false);
 }
 EXPORT_SYMBOL_GPL(nvme_sec_submit);
 #endif /* CONFIG_BLK_SED_OPAL */
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index e8ef42b9d50c..7ad3fbebab95 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -3480,7 +3480,7 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 			    ctrl->lport->ops->fcprqst_priv_sz);
 	ctrl->admin_tag_set.driver_data = ctrl;
 	ctrl->admin_tag_set.nr_hw_queues = 1;
-	ctrl->admin_tag_set.timeout = ADMIN_TIMEOUT;
+	ctrl->admin_tag_set.timeout = NVME_ADMIN_TIMEOUT;
 	ctrl->admin_tag_set.flags = BLK_MQ_F_NO_SCHED;
 
 	ret = blk_mq_alloc_tag_set(&ctrl->admin_tag_set);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 78dc422ee42c..e198fb37bcb6 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -24,7 +24,7 @@ extern unsigned int nvme_io_timeout;
 #define NVME_IO_TIMEOUT	(nvme_io_timeout * HZ)
 
 extern unsigned int admin_timeout;
-#define ADMIN_TIMEOUT	(admin_timeout * HZ)
+#define NVME_ADMIN_TIMEOUT	(admin_timeout * HZ)
 
 #define NVME_DEFAULT_KATO	5
 #define NVME_KATO_GRACE		10
@@ -648,7 +648,7 @@ static inline void nvme_default_timeout(struct request *req,
 	 * changes it should reflect here.
 	 */
 	if (!timeout)
-		timeout = queuedata ? NVME_IO_TIMEOUT : ADMIN_TIMEOUT;
+		timeout = queuedata ? NVME_IO_TIMEOUT : NVME_ADMIN_TIMEOUT;
 
 	req->timeout = timeout;
 }
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 5e07d5628864..e3386423d823 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1296,7 +1296,7 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
 		return BLK_EH_RESET_TIMER;
 	}
 
-	abort_req->timeout = ADMIN_TIMEOUT;
+	abort_req->timeout = NVME_ADMIN_TIMEOUT;
 	abort_req->end_io_data = NULL;
 	blk_execute_rq_nowait(abort_req->q, NULL, abort_req, 0, abort_endio);
 
@@ -1592,7 +1592,7 @@ static int nvme_alloc_admin_tags(struct nvme_dev *dev)
 		dev->admin_tagset.nr_hw_queues = 1;
 
 		dev->admin_tagset.queue_depth = NVME_AQ_MQ_TAG_DEPTH;
-		dev->admin_tagset.timeout = ADMIN_TIMEOUT;
+		dev->admin_tagset.timeout = NVME_ADMIN_TIMEOUT;
 		dev->admin_tagset.numa_node = dev->ctrl.numa_node;
 		dev->admin_tagset.cmd_size = sizeof(struct nvme_iod);
 		dev->admin_tagset.flags = BLK_MQ_F_NO_SCHED;
@@ -2210,7 +2210,7 @@ static int nvme_delete_queue(struct nvme_queue *nvmeq, u8 opcode)
 	if (IS_ERR(req))
 		return PTR_ERR(req);
 
-	req->timeout = ADMIN_TIMEOUT;
+	req->timeout = NVME_ADMIN_TIMEOUT;
 	req->end_io_data = nvmeq;
 
 	init_completion(&nvmeq->delete_done);
@@ -2226,7 +2226,7 @@ static bool __nvme_disable_io_queues(struct nvme_dev *dev, u8 opcode)
 	unsigned long timeout;
 
  retry:
-	timeout = ADMIN_TIMEOUT;
+	timeout = NVME_ADMIN_TIMEOUT;
 	while (nr_queues > 0) {
 		if (nvme_delete_queue(&dev->queues[nr_queues], opcode))
 			break;
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 3247d4ee46b1..4899aa75937a 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -798,7 +798,7 @@ static struct blk_mq_tag_set *nvme_rdma_alloc_tagset(struct nvme_ctrl *nctrl,
 				NVME_RDMA_DATA_SGL_SIZE;
 		set->driver_data = ctrl;
 		set->nr_hw_queues = 1;
-		set->timeout = ADMIN_TIMEOUT;
+		set->timeout = NVME_ADMIN_TIMEOUT;
 		set->flags = BLK_MQ_F_NO_SCHED;
 	} else {
 		set = &ctrl->tag_set;
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 245a73d0a1ca..f86f2ec5734e 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1571,7 +1571,7 @@ static struct blk_mq_tag_set *nvme_tcp_alloc_tagset(struct nvme_ctrl *nctrl,
 		set->cmd_size = sizeof(struct nvme_tcp_request);
 		set->driver_data = ctrl;
 		set->nr_hw_queues = 1;
-		set->timeout = ADMIN_TIMEOUT;
+		set->timeout = NVME_ADMIN_TIMEOUT;
 	} else {
 		set = &ctrl->tag_set;
 		memset(set, 0, sizeof(*set));
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 0d6008cf66a2..b8c611eafd68 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -345,7 +345,7 @@ static int nvme_loop_configure_admin_queue(struct nvme_loop_ctrl *ctrl)
 		NVME_INLINE_SG_CNT * sizeof(struct scatterlist);
 	ctrl->admin_tag_set.driver_data = ctrl;
 	ctrl->admin_tag_set.nr_hw_queues = 1;
-	ctrl->admin_tag_set.timeout = ADMIN_TIMEOUT;
+	ctrl->admin_tag_set.timeout = NVME_ADMIN_TIMEOUT;
 	ctrl->admin_tag_set.flags = BLK_MQ_F_NO_SCHED;
 
 	ctrl->queues[0].ctrl = ctrl;
diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index 2564b6769b9e..55e645163162 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -522,7 +522,7 @@ int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys)
 	}
 
 	if (!subsys->admin_timeout)
-		subsys->admin_timeout = ADMIN_TIMEOUT;
+		subsys->admin_timeout = NVME_ADMIN_TIMEOUT;
 
 	if (!subsys->io_timeout)
 		subsys->io_timeout = NVME_IO_TIMEOUT;
-- 
2.22.1


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

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

[Christoph Hellwig]

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

Re: [PATCH V3 5/7] nvmet: add passthru admin timeout value attr

[Christoph Hellwig]

On Tue, Sep 15, 2020 at 02:51:16PM -0700, Chaitanya Kulkarni wrote:
> NVMeOF controller in the passsthru mode is capable of handling wide set
> of admin commands including Vender unique passhtru admin comands (VUAC).

VUAC sounds like someone is throwing up and certainly is not NVMe
terminology.   I also don't think we should care about stange verndor
specific commands.  If we want to communicate command timeout we need
to add them to the command supported and effects log, similar to what
SCSI does in REPORT SUPPORTED OPERATION CODES.

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

Re: [PATCH V3 7/7] nvme: use consistent macro name for timeout

[Christoph Hellwig]

>  	return __nvme_submit_sync_cmd(ctrl->admin_q, &cmd, NULL, buffer, len,
> -				      ADMIN_TIMEOUT, NVME_QID_ANY, 1, 0, false);
> +				      NVME_ADMIN_TIMEOUT, NVME_QID_ANY, 1, 0, false);

Please don't add over 80 char lines.

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

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

[Chaitanya Kulkarni]

On 9/16/20 00:09, Christoph Hellwig wrote:
>> +	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;
> }
> 

Okay I'll add in V4.

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

Re: [PATCH V3 4/7] nvmet: set default timeout for passthru requests

[Keith Busch]

On Tue, Sep 15, 2020 at 02:51:15PM -0700, Chaitanya Kulkarni wrote:
> @@ -242,6 +242,8 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
>  		goto out_put_ns;
>  	}
>  
> +	rq->timeout = q->queuedata ? NVME_IO_TIMEOUT : ADMIN_TIMEOUT;
> +

Any reason you didn't use your new nvme_default_timeout() for this
assignment?

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

Re: [PATCH V3 4/7] nvmet: set default timeout for passthru requests

[Chaitanya Kulkarni]

On 9/16/20 13:57, Keith Busch wrote:
> On Tue, Sep 15, 2020 at 02:51:15PM -0700, Chaitanya Kulkarni wrote:
>> @@ -242,6 +242,8 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
>>   		goto out_put_ns;
>>   	}
>>   
>> +	rq->timeout = q->queuedata ? NVME_IO_TIMEOUT : ADMIN_TIMEOUT;
>> +
> Any reason you didn't use your new nvme_default_timeout() for this
> assignment?
> 
Yes, since it is being modified in next patches and easy to review that
way.

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

Re: [PATCH V3 5/7] nvmet: add passthru admin timeout value attr

[Chaitanya Kulkarni]

On 9/16/20 00:12, Christoph Hellwig wrote:
> On Tue, Sep 15, 2020 at 02:51:16PM -0700, Chaitanya Kulkarni wrote:
>> NVMeOF controller in the passsthru mode is capable of handling wide set
>> of admin commands including Vender unique passhtru admin comands (VUAC).
> VUAC sounds like someone is throwing up and certainly is not NVMe
> terminology.   I also don't think we should care about stange verndor
> specific commands.  If we want to communicate command timeout we need

VUCs are Vendor unique NVMe commands. Given that passthru allows the 
wider range of feature and command sets it also needs to handle timeout
for VUCs to avoid getting stuck on over the fabrics.

> to add them to the command supported and effects log, similar to what
> SCSI does in REPORT SUPPORTED OPERATION CODES.
> 
I'm usually against polluting the configfs space.

If I understand correctly you are saying that user should read the 
command effects log and set the timeout per command for the
Passthru mode ?

Here is the detailed scenario :-

1. Vendor A has nvme-xxx admin cmd and nvme-yyy io cmd.
2. The command effects log page has timeout values for these VUCs.
    This value for VUC is larger than the default timeout values
    when the driver is loaded since these are not frequent commands
    e.g. flash-mgmt/Device RMA.
    User still wants to use the driver default timeout values but
    need an ability to override these values time to time on target
    side.
3. On the host side user reads the cmd-effect log and issues the
    passthru cmd ADMIN/IO IOCTL with timeout values from nvme-cli on
    NVMeOF target passthru controller.
4. Timeout value is applied on host side request in the driver :-
     nvme_handle_ctrl_ioctl()
     -NVME_IOCTL_ADMIN_CMD
     --nvme_user_cmd()
     ---nvme_submit_user_cmd()
     ----nvme_execute_passthru_rq()
     -----blk_execute_rq()

5. Now the target side we don't get the timeout value so it uses
    default admin timeout (ADMIN_TIMEOUT, given that we use the newly
    added nvme_default_timeout() helper) which is smaller than the
    timeout specified on the host side.
6. The target side request times out while host side request is
    intact due to default values used for pt-ctrl.

any thoughts ?

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