From 81587f0663497fb6384025543e09a7c8473e45a9 Mon Sep 17 00:00:00 2001 From: Jim Harris Date: Fri, 25 Feb 2022 00:56:04 +0000 Subject: [PATCH] bdev/nvme: always defer start of discovery service We used to wait until the discovery service could connect to the discovery subsystem before calling the callback function provided by the caller (mainly the start_discovery RPC). Moving forward, we will be handling the case where the discovery subsystem is unavailable temporarily. For now, let's not fail the bdev_nvme_start_discovery call if we cannot connect to the discovery subsystem. This will keep the initial service start path the same as the path where the discovery subsystem is temporarily unavailable. In the future, we can consider adding functionality to the start_discovery RPC that waits up to X number of seconds to see if we were able to connect and fail otherwise. Signed-off-by: Jim Harris Change-Id: Icb05523b9d59f508bfbc0233595c8bf58c10488f Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/11768 Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Tested-by: SPDK CI Jenkins Reviewed-by: Shuhei Matsumoto Reviewed-by: Ben Walker Reviewed-by: Aleksey Marchuk --- module/bdev/nvme/bdev_nvme.c | 38 ++++++++------------------------ module/bdev/nvme/bdev_nvme.h | 4 +--- module/bdev/nvme/bdev_nvme_rpc.c | 29 ++++-------------------- 3 files changed, 14 insertions(+), 57 deletions(-) diff --git a/module/bdev/nvme/bdev_nvme.c b/module/bdev/nvme/bdev_nvme.c index 1d1af4551..aca37e2e4 100644 --- a/module/bdev/nvme/bdev_nvme.c +++ b/module/bdev/nvme/bdev_nvme.c @@ -4323,7 +4323,6 @@ struct discovery_entry_ctx { struct discovery_ctx { char *name; - spdk_bdev_nvme_start_discovery_fn start_cb_fn; spdk_bdev_nvme_stop_discovery_fn stop_cb_fn; void *cb_ctx; struct spdk_nvme_probe_ctx *probe_ctx; @@ -4606,20 +4605,6 @@ discovery_aer_cb(void *arg, const struct spdk_nvme_cpl *cpl) get_discovery_log_page(ctx); } -static void -start_discovery_done(void *cb_ctx) -{ - struct discovery_ctx *ctx = cb_ctx; - - DISCOVERY_INFOLOG(ctx, "start discovery done\n"); - ctx->start_cb_fn(ctx->cb_ctx, ctx->rc); - if (ctx->rc != 0) { - DISCOVERY_ERRLOG(ctx, "could not connect to discovery ctrlr\n"); - TAILQ_REMOVE(&g_discovery_ctxs, ctx, tailq); - free_discovery_ctx(ctx); - } -} - static void discovery_attach_cb(void *cb_ctx, const struct spdk_nvme_transport_id *trid, struct spdk_nvme_ctrlr *ctrlr, const struct spdk_nvme_ctrlr_opts *opts) @@ -4644,7 +4629,9 @@ discovery_poller(void *arg) if (ctx->detach) { bool detach_done = false; - if (ctx->detach_ctx == NULL) { + if (ctx->ctrlr == NULL) { + detach_done = true; + } else if (ctx->detach_ctx == NULL) { rc = spdk_nvme_detach_async(ctx->ctrlr, &ctx->detach_ctx); if (rc != 0) { DISCOVERY_ERRLOG(ctx, "could not detach discovery ctrlr\n"); @@ -4662,12 +4649,16 @@ discovery_poller(void *arg) ctx->stop_cb_fn(ctx->cb_ctx); free_discovery_ctx(ctx); } + } else if (ctx->probe_ctx == NULL && ctx->ctrlr == NULL) { + ctx->probe_ctx = spdk_nvme_connect_async(&ctx->trid, &ctx->drv_opts, discovery_attach_cb); + if (ctx->probe_ctx == NULL) { + DISCOVERY_ERRLOG(ctx, "could not start discovery connect\n"); + } } else if (ctx->probe_ctx) { rc = spdk_nvme_probe_poll_async(ctx->probe_ctx); if (rc != -EAGAIN) { DISCOVERY_INFOLOG(ctx, "discovery ctrlr connected\n"); ctx->rc = rc; - spdk_thread_send_msg(ctx->calling_thread, start_discovery_done, ctx); if (rc == 0) { get_discovery_log_page(ctx); } @@ -4691,9 +4682,7 @@ start_discovery_poller(void *arg) int bdev_nvme_start_discovery(struct spdk_nvme_transport_id *trid, const char *base_name, - struct spdk_nvme_ctrlr_opts *drv_opts, - spdk_bdev_nvme_start_discovery_fn cb_fn, - void *cb_ctx) + struct spdk_nvme_ctrlr_opts *drv_opts) { struct discovery_ctx *ctx; @@ -4707,8 +4696,6 @@ bdev_nvme_start_discovery(struct spdk_nvme_transport_id *trid, free_discovery_ctx(ctx); return -ENOMEM; } - ctx->start_cb_fn = cb_fn; - ctx->cb_ctx = cb_ctx; memcpy(&ctx->drv_opts, drv_opts, sizeof(*drv_opts)); ctx->calling_thread = spdk_get_thread(); TAILQ_INIT(&ctx->nvm_entry_ctxs); @@ -4721,13 +4708,6 @@ bdev_nvme_start_discovery(struct spdk_nvme_transport_id *trid, free_discovery_ctx(ctx); return -ENOMEM; } - ctx->probe_ctx = spdk_nvme_connect_async(&ctx->trid, &ctx->drv_opts, discovery_attach_cb); - if (ctx->probe_ctx == NULL) { - DISCOVERY_ERRLOG(ctx, "could not start discovery connect\n"); - free_discovery_ctx(ctx); - return -EIO; - } - spdk_thread_send_msg(g_bdev_nvme_init_thread, start_discovery_poller, ctx); return 0; } diff --git a/module/bdev/nvme/bdev_nvme.h b/module/bdev/nvme/bdev_nvme.h index 51d93b220..21b36248a 100644 --- a/module/bdev/nvme/bdev_nvme.h +++ b/module/bdev/nvme/bdev_nvme.h @@ -49,7 +49,6 @@ extern bool g_bdev_nvme_module_finish; #define NVME_MAX_CONTROLLERS 1024 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, int rc); typedef void (*spdk_bdev_nvme_stop_discovery_fn)(void *ctx); struct nvme_ctrlr_opts { @@ -283,8 +282,7 @@ int bdev_nvme_create(struct spdk_nvme_transport_id *trid, bool multipath); int bdev_nvme_start_discovery(struct spdk_nvme_transport_id *trid, const char *base_name, - struct spdk_nvme_ctrlr_opts *drv_opts, - spdk_bdev_nvme_start_discovery_fn cb_fn, void *cb_ctx); + struct spdk_nvme_ctrlr_opts *drv_opts); int bdev_nvme_stop_discovery(const char *name, spdk_bdev_nvme_stop_discovery_fn cb_fn, void *cb_ctx); diff --git a/module/bdev/nvme/bdev_nvme_rpc.c b/module/bdev/nvme/bdev_nvme_rpc.c index 85f198730..150d7988b 100644 --- a/module/bdev/nvme/bdev_nvme_rpc.c +++ b/module/bdev/nvme/bdev_nvme_rpc.c @@ -1634,25 +1634,6 @@ struct rpc_bdev_nvme_start_discovery_ctx { struct spdk_jsonrpc_request *request; }; -static void -rpc_bdev_nvme_start_discovery_done(void *cb_ctx, int rc) -{ - struct rpc_bdev_nvme_start_discovery_ctx *ctx = cb_ctx; - struct spdk_jsonrpc_request *request = ctx->request; - - if (rc < 0) { - spdk_jsonrpc_send_error_response(request, SPDK_JSONRPC_ERROR_INVALID_PARAMS, "Invalid parameters"); - free_rpc_bdev_nvme_start_discovery(&ctx->req); - free(ctx); - return; - } - - spdk_jsonrpc_send_bool_response(ctx->request, rc == 0); - - free_rpc_bdev_nvme_start_discovery(&ctx->req); - free(ctx); -} - static void rpc_bdev_nvme_start_discovery(struct spdk_jsonrpc_request *request, const struct spdk_json_val *params) @@ -1731,15 +1712,13 @@ rpc_bdev_nvme_start_discovery(struct spdk_jsonrpc_request *request, } ctx->request = request; - rc = bdev_nvme_start_discovery(&trid, ctx->req.name, &ctx->req.opts, - rpc_bdev_nvme_start_discovery_done, ctx); - if (rc) { + rc = bdev_nvme_start_discovery(&trid, ctx->req.name, &ctx->req.opts); + if (rc == 0) { + spdk_jsonrpc_send_bool_response(ctx->request, true); + } else { spdk_jsonrpc_send_error_response(request, rc, spdk_strerror(-rc)); - goto cleanup; } - return; - cleanup: free_rpc_bdev_nvme_start_discovery(&ctx->req); free(ctx);