nvme: separate admin timeout vs. io timeout

Signed-off-by: Matt Dumm <matt.dumm@hpe.com>
Change-Id: Ia1f105fdf154aae034ccfca2f0dc3b4c43c9fc84
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/8072
Reviewed-by: Michael Haeuptle <michaelhaeuptle@gmail.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: Aleksey Marchuk <alexeymar@mellanox.com>
Reviewed-by: Ziye Yang <ziye.yang@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Community-CI: Broadcom CI <spdk-ci.pdl@broadcom.com>

CHANGELOG.md

@@ -72,6 +72,9 @@ the spdk_nvme_ctrlr_opts structure prior to controller attach.
 Add a new function `spdk_nvme_detach_poll` to simplify a common use case to continue
 polling until all detachments complete.
 
+Added new argument 'timeout_admin_us' to 'spdk_nvme_ctrlr_register_timeout_callback' so callers
+can specify a different timeout for admin commands vs. io commands.
+
 An existing function `spdk_nvme_detach_async` was updated to add one or more detachments
 to an active context while it is being polled.
 

examples/nvme/hotplug/hotplug.c

@@ -101,7 +101,8 @@ register_dev(struct spdk_nvme_ctrlr *ctrlr)
 	dev->is_removed = false;
 	dev->is_draining = false;
 
-	spdk_nvme_ctrlr_register_timeout_callback(ctrlr, g_timeout_in_us, timeout_cb, NULL);
+	spdk_nvme_ctrlr_register_timeout_callback(ctrlr, g_timeout_in_us, g_timeout_in_us, timeout_cb,
+			NULL);
 
 	dev->ns = spdk_nvme_ctrlr_get_ns(ctrlr, 1);
 

include/spdk/nvme.h

@@ -1294,12 +1294,14 @@ typedef void (*spdk_nvme_timeout_cb)(void *cb_arg,
  * for timeout callback.
  *
  * \param ctrlr NVMe controller on which to monitor for timeout.
- * \param timeout_us Timeout value in microseconds.
+ * \param timeout_io_us Timeout value in microseconds for io commands.
+ * \param timeout_admin_us Timeout value in microseconds for admin commands.
  * \param cb_fn A function pointer that points to the callback function.
  * \param cb_arg Argument to the callback function.
  */
 void spdk_nvme_ctrlr_register_timeout_callback(struct spdk_nvme_ctrlr *ctrlr,
-		uint64_t timeout_us, spdk_nvme_timeout_cb cb_fn, void *cb_arg);
+		uint64_t timeout_io_us, uint64_t timeout_admin_us,
+		spdk_nvme_timeout_cb cb_fn, void *cb_arg);
 
 /**
  * NVMe I/O queue pair initialization options.

lib/nvme/nvme.c

@@ -476,6 +476,8 @@ nvme_request_check_timeout(struct nvme_request *req, uint16_t cid,
 {
 	struct spdk_nvme_qpair *qpair = req->qpair;
 	struct spdk_nvme_ctrlr *ctrlr = qpair->ctrlr;
+	uint64_t timeout_ticks = nvme_qpair_is_admin_queue(qpair) ?
+				 active_proc->timeout_admin_ticks : active_proc->timeout_io_ticks;
 
 	assert(active_proc->timeout_cb_fn != NULL);
 
@@ -492,7 +494,7 @@ nvme_request_check_timeout(struct nvme_request *req, uint16_t cid,
 		return 0;
 	}
 
-	if (req->submit_tick + active_proc->timeout_ticks > now_tick) {
+	if (req->submit_tick + timeout_ticks > now_tick) {
 		return 1;
 	}
 

lib/nvme/nvme_ctrlr.c

@@ -3854,7 +3854,8 @@ spdk_nvme_ctrlr_register_aer_callback(struct spdk_nvme_ctrlr *ctrlr,
 
 void
 spdk_nvme_ctrlr_register_timeout_callback(struct spdk_nvme_ctrlr *ctrlr,
-		uint64_t timeout_us, spdk_nvme_timeout_cb cb_fn, void *cb_arg)
+		uint64_t timeout_io_us, uint64_t timeout_admin_us,
+		spdk_nvme_timeout_cb cb_fn, void *cb_arg)
 {
 	struct spdk_nvme_ctrlr_process	*active_proc;
 
@@ -3862,7 +3863,8 @@ spdk_nvme_ctrlr_register_timeout_callback(struct spdk_nvme_ctrlr *ctrlr,
 
 	active_proc = nvme_ctrlr_get_current_process(ctrlr);
 	if (active_proc) {
-		active_proc->timeout_ticks = timeout_us * spdk_get_ticks_hz() / 1000000ULL;
+		active_proc->timeout_io_ticks = timeout_io_us * spdk_get_ticks_hz() / 1000000ULL;
+		active_proc->timeout_admin_ticks = timeout_admin_us * spdk_get_ticks_hz() / 1000000ULL;
 		active_proc->timeout_cb_fn = cb_fn;
 		active_proc->timeout_cb_arg = cb_arg;
 	}

lib/nvme/nvme_internal.h

@@ -749,7 +749,9 @@ struct spdk_nvme_ctrlr_process {
 	 */
 	spdk_nvme_timeout_cb		timeout_cb_fn;
 	void				*timeout_cb_arg;
-	uint64_t			timeout_ticks;
+	/** separate timeout values for io vs. admin reqs */
+	uint64_t			timeout_io_ticks;
+	uint64_t			timeout_admin_ticks;
 };
 
 struct spdk_nvme_ctrlr_aer_completion_list {

module/bdev/nvme/bdev_nvme.c

@@ -117,6 +117,7 @@ static TAILQ_HEAD(, nvme_probe_skip_entry) g_skipped_nvme_ctrlrs = TAILQ_HEAD_IN
 static struct spdk_bdev_nvme_opts g_opts = {
 	.action_on_timeout = SPDK_BDEV_NVME_TIMEOUT_ACTION_NONE,
 	.timeout_us = 0,
+	.timeout_admin_us = 0,
 	.keep_alive_timeout_ms = SPDK_BDEV_NVME_DEFAULT_KEEP_ALIVE_TIMEOUT_IN_MS,
 	.retry_count = 4,
 	.arbitration_burst = 0,
@@ -1896,8 +1897,12 @@ nvme_ctrlr_create(struct spdk_nvme_ctrlr *ctrlr,
 	pthread_mutex_unlock(&g_bdev_nvme_mutex);
 
 	if (g_opts.timeout_us > 0) {
+		/* Register timeout callback. Timeout values for IO vs. admin reqs can be different. */
+		/* If timeout_admin_us is 0 (not specified), admin uses same timeout as IO. */
+		uint64_t adm_timeout_us = (g_opts.timeout_admin_us == 0) ?
+					  g_opts.timeout_us : g_opts.timeout_admin_us;
 		spdk_nvme_ctrlr_register_timeout_callback(ctrlr, g_opts.timeout_us,
-				timeout_cb, nvme_ctrlr);
+				adm_timeout_us, timeout_cb, nvme_ctrlr);
 	}
 
 	spdk_nvme_ctrlr_register_aer_callback(ctrlr, aer_cb, nvme_ctrlr);

module/bdev/nvme/bdev_nvme.h

@@ -51,6 +51,7 @@ enum spdk_bdev_timeout_action {
 struct spdk_bdev_nvme_opts {
 	enum spdk_bdev_timeout_action action_on_timeout;
 	uint64_t timeout_us;
+	uint64_t timeout_admin_us;
 	uint32_t keep_alive_timeout_ms;
 	uint32_t retry_count;
 	uint32_t arbitration_burst;

test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c

@@ -97,7 +97,7 @@ DEFINE_STUB_V(spdk_nvme_ctrlr_register_aer_callback, (struct spdk_nvme_ctrlr *ct
 		spdk_nvme_aer_cb aer_cb_fn, void *aer_cb_arg));
 
 DEFINE_STUB_V(spdk_nvme_ctrlr_register_timeout_callback, (struct spdk_nvme_ctrlr *ctrlr,
-		uint64_t timeout_us, spdk_nvme_timeout_cb cb_fn, void *cb_arg));
+		uint64_t timeout_io_us, uint64_t timeout_admin_us, spdk_nvme_timeout_cb cb_fn, void *cb_arg));
 
 DEFINE_STUB(spdk_nvme_ctrlr_is_ocssd_supported, bool, (struct spdk_nvme_ctrlr *ctrlr), false);