bdev/nvme: add timeout option to start_discovery

It's now possible to specify a time to wait until a connection to the
discovery controller and the NVM controllers it exposes is made.

Whenever that time is exceeded, a callback is immediately executed.
However, depending on the stage of the discovery process, we might need
to wait a while before actually stopping it (e.g. because a controller
attach is in progress).  That means that a discovery service might be
visible for a while after it timed out.

Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>
Change-Id: I2d01837b581e0fa24c8e777730d88d990c94b1d8
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/12684
Community-CI: Broadcom CI <spdk-ci.pdl@broadcom.com>
Community-CI: Mellanox Build Bot
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Shuhei Matsumoto <smatsumoto@nvidia.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>

doc/jsonrpc.md

@@ -3275,6 +3275,7 @@ adrfam                     | Optional | string      | NVMe-oF target adrfam: ipv
 trsvcid                    | Optional | string      | NVMe-oF target trsvcid: port number
 hostnqn                    | Optional | string      | NVMe-oF target hostnqn
 wait_for_attach            | Optional | bool        | Wait to complete until all discovered NVM subsystems are attached
+attach_timeout_ms          | Optional | number      | Time to wait until the discovery and all discovered NVM subsystems are attached
 ctrlr_loss_timeout_sec     | Optional | number      | Time to wait until ctrlr is reconnected before deleting ctrlr.  -1 means infinite reconnects. 0 means no reconnect.
 reconnect_delay_sec        | Optional | number      | Time to delay a reconnect trial. 0 means no reconnect.
 fast_io_fail_timeout_sec   | Optional | number      | Time to wait until ctrlr is reconnected before failing I/O to ctrlr. 0 means no such timeout.

module/bdev/nvme/bdev_nvme.c

@@ -4787,6 +4787,12 @@ struct discovery_ctx {
 	TAILQ_HEAD(, discovery_entry_ctx)	discovery_entry_ctxs;
 	int					rc;
 	bool					wait_for_attach;
+	uint64_t				timeout_ticks;
+	/* Denotes that the discovery service is being started. We're waiting
+	 * for the initial connection to the discovery controller to be
+	 * established and attach discovered NVM ctrlrs.
+	 */
+	bool					initializing;
 	/* Denotes if a discovery is currently in progress for this context.
 	 * That includes connecting to newly discovered subsystems.  Used to
 	 * ensure we do not start a new discovery until an existing one is
@@ -4829,6 +4835,7 @@ free_discovery_ctx(struct discovery_ctx *ctx)
 static void
 discovery_complete(struct discovery_ctx *ctx)
 {
+	ctx->initializing = false;
 	ctx->in_progress = false;
 	if (ctx->pending) {
 		ctx->pending = false;
@@ -4938,21 +4945,34 @@ discovery_remove_controllers(struct discovery_ctx *ctx)
 	discovery_complete(ctx);
 }
 
+static void
+complete_discovery_start(struct discovery_ctx *ctx, int status)
+{
+	ctx->timeout_ticks = 0;
+	ctx->rc = status;
+	if (ctx->start_cb_fn) {
+		ctx->start_cb_fn(ctx->cb_ctx, status);
+		ctx->start_cb_fn = NULL;
+		ctx->cb_ctx = NULL;
+	}
+}
+
 static void
 discovery_attach_controller_done(void *cb_ctx, size_t bdev_count, int rc)
 {
 	struct discovery_entry_ctx *entry_ctx = cb_ctx;
-	struct discovery_ctx *ctx = entry_ctx->ctx;;
+	struct discovery_ctx *ctx = entry_ctx->ctx;
 
 	DISCOVERY_INFOLOG(ctx, "attach %s done\n", entry_ctx->name);
 	ctx->attach_in_progress--;
 	if (ctx->attach_in_progress == 0) {
-		if (ctx->start_cb_fn) {
+		complete_discovery_start(ctx, ctx->rc);
-			ctx->start_cb_fn(ctx->cb_ctx);
+		if (ctx->initializing && ctx->rc != 0) {
-			ctx->start_cb_fn = NULL;
+			DISCOVERY_ERRLOG(ctx, "stopping discovery due to errors: %d\n", ctx->rc);
-			ctx->cb_ctx = NULL;
+			stop_discovery(ctx, NULL, ctx->cb_ctx);
+		} else {
+			discovery_remove_controllers(ctx);
 		}
-		discovery_remove_controllers(ctx);
 	}
 }
 
@@ -5120,6 +5140,13 @@ discovery_attach_cb(void *cb_ctx, const struct spdk_nvme_transport_id *trid,
 	DISCOVERY_INFOLOG(ctx, "discovery ctrlr attached\n");
 	ctx->probe_ctx = NULL;
 	ctx->ctrlr = ctrlr;
+
+	if (ctx->rc != 0) {
+		DISCOVERY_ERRLOG(ctx, "encountered error while attaching discovery ctrlr: %d\n",
+				 ctx->rc);
+		return;
+	}
+
 	spdk_nvme_ctrlr_register_aer_callback(ctx->ctrlr, discovery_aer_cb, ctx);
 }
 
@@ -5146,9 +5173,23 @@ discovery_poller(void *arg)
 		}
 		spdk_poller_unregister(&ctx->poller);
 		TAILQ_REMOVE(&g_discovery_ctxs, ctx, tailq);
-		ctx->stop_cb_fn(ctx->cb_ctx);
+		assert(ctx->start_cb_fn == NULL);
+		if (ctx->stop_cb_fn != NULL) {
+			ctx->stop_cb_fn(ctx->cb_ctx);
+		}
 		free_discovery_ctx(ctx);
 	} else if (ctx->probe_ctx == NULL && ctx->ctrlr == NULL) {
+		if (ctx->timeout_ticks != 0 && ctx->timeout_ticks < spdk_get_ticks()) {
+			DISCOVERY_ERRLOG(ctx, "timed out while attaching discovery ctrlr\n");
+			assert(ctx->initializing);
+			spdk_poller_unregister(&ctx->poller);
+			TAILQ_REMOVE(&g_discovery_ctxs, ctx, tailq);
+			complete_discovery_start(ctx, -ETIMEDOUT);
+			stop_discovery(ctx, NULL, NULL);
+			free_discovery_ctx(ctx);
+			return SPDK_POLLER_BUSY;
+		}
+
 		assert(ctx->entry_ctx_in_use == NULL);
 		ctx->entry_ctx_in_use = TAILQ_FIRST(&ctx->discovery_entry_ctxs);
 		TAILQ_REMOVE(&ctx->discovery_entry_ctxs, ctx->entry_ctx_in_use, tailq);
@@ -5163,15 +5204,39 @@ discovery_poller(void *arg)
 			ctx->entry_ctx_in_use = NULL;
 		}
 	} else if (ctx->probe_ctx) {
+		if (ctx->timeout_ticks != 0 && ctx->timeout_ticks < spdk_get_ticks()) {
+			DISCOVERY_ERRLOG(ctx, "timed out while attaching discovery ctrlr\n");
+			complete_discovery_start(ctx, -ETIMEDOUT);
+			return SPDK_POLLER_BUSY;
+		}
+
 		rc = spdk_nvme_probe_poll_async(ctx->probe_ctx);
 		if (rc != -EAGAIN) {
-			DISCOVERY_INFOLOG(ctx, "discovery ctrlr connected\n");
+			if (ctx->rc != 0) {
-			ctx->rc = rc;
+				assert(ctx->initializing);
-			if (rc == 0) {
+				stop_discovery(ctx, NULL, ctx->cb_ctx);
-				get_discovery_log_page(ctx);
+			} else {
+				DISCOVERY_INFOLOG(ctx, "discovery ctrlr connected\n");
+				ctx->rc = rc;
+				if (rc == 0) {
+					get_discovery_log_page(ctx);
+				}
 			}
 		}
 	} else {
+		if (ctx->timeout_ticks != 0 && ctx->timeout_ticks < spdk_get_ticks()) {
+			DISCOVERY_ERRLOG(ctx, "timed out while attaching NVM ctrlrs\n");
+			complete_discovery_start(ctx, -ETIMEDOUT);
+			/* We need to wait until all NVM ctrlrs are attached before we stop the
+			 * discovery service to make sure we don't detach a ctrlr that is still
+			 * being attached.
+			 */
+			if (ctx->attach_in_progress == 0) {
+				stop_discovery(ctx, NULL, ctx->cb_ctx);
+				return SPDK_POLLER_BUSY;
+			}
+		}
+
 		rc = spdk_nvme_ctrlr_process_admin_completions(ctx->ctrlr);
 		if (rc < 0) {
 			spdk_poller_unregister(&ctx->poller);
@@ -5204,6 +5269,7 @@ bdev_nvme_start_discovery(struct spdk_nvme_transport_id *trid,
 			  const char *base_name,
 			  struct spdk_nvme_ctrlr_opts *drv_opts,
 			  struct nvme_ctrlr_opts *bdev_opts,
+			  uint64_t attach_timeout,
 			  spdk_bdev_nvme_start_discovery_fn cb_fn, void *cb_ctx)
 {
 	struct discovery_ctx *ctx;
@@ -5244,12 +5310,17 @@ bdev_nvme_start_discovery(struct spdk_nvme_transport_id *trid,
 	ctx->calling_thread = spdk_get_thread();
 	ctx->start_cb_fn = cb_fn;
 	ctx->cb_ctx = cb_ctx;
+	ctx->initializing = true;
 	if (ctx->start_cb_fn) {
 		/* We can use this when dumping json to denote if this RPC parameter
 		 * was specified or not.
 		 */
 		ctx->wait_for_attach = true;
 	}
+	if (attach_timeout != 0) {
+		ctx->timeout_ticks = spdk_get_ticks() + attach_timeout *
+				     spdk_get_ticks_hz() / 1000ull;
+	}
 	TAILQ_INIT(&ctx->nvm_entry_ctxs);
 	TAILQ_INIT(&ctx->discovery_entry_ctxs);
 	memcpy(&ctx->trid, trid, sizeof(*trid));
@@ -5281,6 +5352,12 @@ bdev_nvme_stop_discovery(const char *name, spdk_bdev_nvme_stop_discovery_fn cb_f
 			if (ctx->stop) {
 				return -EALREADY;
 			}
+			/* If we're still starting the discovery service and ->rc is non-zero, we're
+			 * going to stop it as soon as we can
+			 */
+			if (ctx->initializing && ctx->rc != 0) {
+				return -EALREADY;
+			}
 			stop_discovery(ctx, cb_fn, cb_ctx);
 			return 0;
 		}

module/bdev/nvme/bdev_nvme.h

@@ -54,7 +54,7 @@ enum bdev_nvme_multipath_policy {
 };
 
 typedef void (*spdk_bdev_create_nvme_fn)(void *ctx, size_t bdev_count, int rc);
-typedef void (*spdk_bdev_nvme_start_discovery_fn)(void *ctx);
+typedef void (*spdk_bdev_nvme_start_discovery_fn)(void *ctx, int status);
 typedef void (*spdk_bdev_nvme_stop_discovery_fn)(void *ctx);
 
 struct nvme_ctrlr_opts {
@@ -298,7 +298,7 @@ int bdev_nvme_create(struct spdk_nvme_transport_id *trid,
 
 int bdev_nvme_start_discovery(struct spdk_nvme_transport_id *trid, const char *base_name,
 			      struct spdk_nvme_ctrlr_opts *drv_opts, struct nvme_ctrlr_opts *bdev_opts,
-			      spdk_bdev_nvme_start_discovery_fn cb_fn, void *cb_ctx);
+			      uint64_t timeout, spdk_bdev_nvme_start_discovery_fn cb_fn, void *cb_ctx);
 int bdev_nvme_stop_discovery(const char *name, spdk_bdev_nvme_stop_discovery_fn cb_fn,
 			     void *cb_ctx);
 void bdev_nvme_get_discovery_info(struct spdk_json_write_ctx *w);

module/bdev/nvme/bdev_nvme_rpc.c

@@ -1568,6 +1568,7 @@ struct rpc_bdev_nvme_start_discovery {
 	char *trsvcid;
 	char *hostnqn;
 	bool wait_for_attach;
+	uint64_t attach_timeout_ms;
 	struct spdk_nvme_ctrlr_opts opts;
 	struct nvme_ctrlr_opts bdev_opts;
 };
@@ -1591,6 +1592,7 @@ static const struct spdk_json_object_decoder rpc_bdev_nvme_start_discovery_decod
 	{"trsvcid", offsetof(struct rpc_bdev_nvme_start_discovery, trsvcid), spdk_json_decode_string, true},
 	{"hostnqn", offsetof(struct rpc_bdev_nvme_start_discovery, hostnqn), spdk_json_decode_string, true},
 	{"wait_for_attach", offsetof(struct rpc_bdev_nvme_start_discovery, wait_for_attach), spdk_json_decode_bool, true},
+	{"attach_timeout_ms", offsetof(struct rpc_bdev_nvme_start_discovery, attach_timeout_ms), spdk_json_decode_uint64, true},
 	{"ctrlr_loss_timeout_sec", offsetof(struct rpc_bdev_nvme_start_discovery, bdev_opts.ctrlr_loss_timeout_sec), spdk_json_decode_int32, true},
 	{"reconnect_delay_sec", offsetof(struct rpc_bdev_nvme_start_discovery, bdev_opts.reconnect_delay_sec), spdk_json_decode_uint32, true},
 	{"fast_io_fail_timeout_sec", offsetof(struct rpc_bdev_nvme_start_discovery, bdev_opts.fast_io_fail_timeout_sec), spdk_json_decode_uint32, true},
@@ -1602,11 +1604,15 @@ struct rpc_bdev_nvme_start_discovery_ctx {
 };
 
 static void
-rpc_bdev_nvme_start_discovery_done(void *ctx)
+rpc_bdev_nvme_start_discovery_done(void *ctx, int status)
 {
 	struct spdk_jsonrpc_request *request = ctx;
 
-	spdk_jsonrpc_send_bool_response(request, true);
+	if (status != 0) {
+		spdk_jsonrpc_send_error_response(request, status, spdk_strerror(-status));
+	} else {
+		spdk_jsonrpc_send_bool_response(request, true);
+	}
 }
 
 static void
@@ -1688,15 +1694,19 @@ rpc_bdev_nvme_start_discovery(struct spdk_jsonrpc_request *request,
 			 ctx->req.hostnqn);
 	}
 
+	if (ctx->req.attach_timeout_ms != 0) {
+		ctx->req.wait_for_attach = true;
+	}
+
 	ctx->request = request;
 	cb_fn = ctx->req.wait_for_attach ? rpc_bdev_nvme_start_discovery_done : NULL;
 	cb_ctx = ctx->req.wait_for_attach ? request : NULL;
 	rc = bdev_nvme_start_discovery(&trid, ctx->req.name, &ctx->req.opts, &ctx->req.bdev_opts,
-				       cb_fn, cb_ctx);
+				       ctx->req.attach_timeout_ms, cb_fn, cb_ctx);
 	if (rc) {
 		spdk_jsonrpc_send_error_response(request, rc, spdk_strerror(-rc));
 	} else if (!ctx->req.wait_for_attach) {
-		rpc_bdev_nvme_start_discovery_done(request);
+		rpc_bdev_nvme_start_discovery_done(request, 0);
 	}
 
 cleanup:

python/spdk/rpc/bdev.py

@@ -750,7 +750,8 @@ def bdev_nvme_reset_controller(client, name):
 
 def bdev_nvme_start_discovery(client, name, trtype, traddr, adrfam=None, trsvcid=None,
                               hostnqn=None, wait_for_attach=None, ctrlr_loss_timeout_sec=None,
-                              reconnect_delay_sec=None, fast_io_fail_timeout_sec=None):
+                              reconnect_delay_sec=None, fast_io_fail_timeout_sec=None,
+                              attach_timeout_ms=None):
     """Start discovery with the specified discovery subsystem
 
     Args:
@@ -775,6 +776,7 @@ def bdev_nvme_start_discovery(client, name, trtype, traddr, adrfam=None, trsvcid
         0 means no such timeout.
         If fast_io_fail_timeout_sec is not zero, it has to be not less than reconnect_delay_sec and less than
         ctrlr_loss_timeout_sec if ctrlr_loss_timeout_sec is not -1. (optional)
+        attach_timeout_ms: Time to wait until the discovery and all discovered NVM subsystems are attached (optional)
     """
     params = {'name': name,
               'trtype': trtype,
@@ -792,6 +794,9 @@ def bdev_nvme_start_discovery(client, name, trtype, traddr, adrfam=None, trsvcid
     if wait_for_attach:
         params['wait_for_attach'] = True
 
+    if attach_timeout_ms is not None:
+        params['attach_timeout_ms'] = attach_timeout_ms
+
     if ctrlr_loss_timeout_sec is not None:
         params['ctrlr_loss_timeout_sec'] = ctrlr_loss_timeout_sec
 

scripts/rpc.py

@@ -720,6 +720,7 @@ if __name__ == "__main__":
                                            trsvcid=args.trsvcid,
                                            hostnqn=args.hostnqn,
                                            wait_for_attach=args.wait_for_attach,
+                                           attach_timeout_ms=args.attach_timeout_ms,
                                            ctrlr_loss_timeout_sec=args.ctrlr_loss_timeout_sec,
                                            reconnect_delay_sec=args.reconnect_delay_sec,
                                            fast_io_fail_timeout_sec=args.fast_io_fail_timeout_sec)
@@ -737,6 +738,10 @@ if __name__ == "__main__":
     p.add_argument('-q', '--hostnqn', help='NVMe-oF host subnqn')
     p.add_argument('-w', '--wait-for-attach', action='store_true',
                    help='Do not complete RPC until all discovered NVM subsystems are attached')
+    p.add_argument('-T', '--attach-timeout-ms', type=int, required=False,
+                   help="""Time to wait until the discovery and all discovered NVM subsystems
+                        are attached (default: 0, meaning wait indefinitely).  Automatically
+                        selects the --wait-for-attach option.""")
     p.add_argument('-l', '--ctrlr-loss-timeout-sec',
                    help="""Time to wait until ctrlr is reconnected before deleting ctrlr.
                    -1 means infinite reconnect retries. 0 means no reconnect retry.

test/nvmf/host/discovery.sh

@@ -151,6 +151,11 @@ NOT $rpc_py -s $HOST_SOCK bdev_nvme_start_discovery -b nvme_second -t $TEST_TRAN
 [[ $(get_discovery_ctrlrs) == "nvme" ]]
 [[ $(get_bdev_list) == "nvme0n1 nvme0n2" ]]
 
+# Try to connect to a non-existing discovery endpoint and verify that it'll timeout
+NOT $rpc_py -s $HOST_SOCK bdev_nvme_start_discovery -b nvme_second -t $TEST_TRANSPORT \
+	-a $NVMF_FIRST_TARGET_IP -s $((DISCOVERY_PORT + 1)) -f ipv4 -q $HOST_NQN -T 3000
+[[ $(get_discovery_ctrlrs) == "nvme" ]]
+
 trap - SIGINT SIGTERM EXIT
 
 kill $hostpid