diff --git a/doc/jsonrpc.md b/doc/jsonrpc.md index 4c791d0b8..33dca1106 100644 --- a/doc/jsonrpc.md +++ b/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. diff --git a/module/bdev/nvme/bdev_nvme.c b/module/bdev/nvme/bdev_nvme.c index eff2d194b..2551e0c30 100644 --- a/module/bdev/nvme/bdev_nvme.c +++ b/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) { - ctx->start_cb_fn(ctx->cb_ctx); - ctx->start_cb_fn = NULL; - ctx->cb_ctx = NULL; + complete_discovery_start(ctx, ctx->rc); + if (ctx->initializing && ctx->rc != 0) { + DISCOVERY_ERRLOG(ctx, "stopping discovery due to errors: %d\n", ctx->rc); + 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"); - ctx->rc = rc; - if (rc == 0) { - get_discovery_log_page(ctx); + if (ctx->rc != 0) { + assert(ctx->initializing); + stop_discovery(ctx, NULL, ctx->cb_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; } diff --git a/module/bdev/nvme/bdev_nvme.h b/module/bdev/nvme/bdev_nvme.h index e84abcea0..43ad65417 100644 --- a/module/bdev/nvme/bdev_nvme.h +++ b/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); diff --git a/module/bdev/nvme/bdev_nvme_rpc.c b/module/bdev/nvme/bdev_nvme_rpc.c index 80aac95b6..0c920b35e 100644 --- a/module/bdev/nvme/bdev_nvme_rpc.c +++ b/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: diff --git a/python/spdk/rpc/bdev.py b/python/spdk/rpc/bdev.py index 4b344ec61..0c54a071d 100644 --- a/python/spdk/rpc/bdev.py +++ b/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 diff --git a/scripts/rpc.py b/scripts/rpc.py index 64c5feb21..9b2c84e57 100755 --- a/scripts/rpc.py +++ b/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. diff --git a/test/nvmf/host/discovery.sh b/test/nvmf/host/discovery.sh index b0add5c9b..53bd05fd8 100755 --- a/test/nvmf/host/discovery.sh +++ b/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