From 0a162815d6eb6aa4f286e1933d710542e57c1e32 Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Tue, 17 Apr 2018 16:59:07 -0700 Subject: [PATCH] Revert "subsystem.c: make subsystem_remove_ns asynchronous" This reverts commit 498f9add11bd439302f1a3d247b3f4c525696805. Making the subsystem removal asynchronous seems to be triggering an intermittent failure in the NVMe AER test. Let's revert this for now until we can diagnose the issue. Change-Id: Ie1d598f0d5cce07e6869d87cd8388848caa78e46 Signed-off-by: Daniel Verkamp Reviewed-on: https://review.gerrithub.io/408118 Reviewed-by: Jim Harris Tested-by: SPDK Automated Test System --- CHANGELOG.md | 3 - include/spdk/nvmf.h | 5 +- lib/event/subsystems/nvmf/nvmf_rpc.c | 28 ++---- lib/nvmf/nvmf.c | 9 +- lib/nvmf/nvmf_internal.h | 2 - lib/nvmf/subsystem.c | 96 ++----------------- .../ctrlr_discovery.c/ctrlr_discovery_ut.c | 7 -- test/unit/lib/nvmf/subsystem.c/subsystem_ut.c | 27 +----- 8 files changed, 18 insertions(+), 159 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f30ee8600..45d16c271 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,9 +31,6 @@ active namespaces and spdk_nvme_ctrlr_is_active_ns() to check if a ns id is acti Namespaces may now be assigned unique identifiers via new optional "eui64" and "nguid" parameters to the `nvmf_subsystem_add_ns` RPC method. -`spdk_nvmf_subsystem_remove_ns` is now asynchronous and requires two additional arguments, cb_fn and cb_arg. -cb_fn is a function pointer of type spdk_channel_for_each_cpl and cb_arg is a void pointer. - ### Blobstore A number of functions have been renamed: diff --git a/include/spdk/nvmf.h b/include/spdk/nvmf.h index 9f8a7f9a6..9dd6667d9 100644 --- a/include/spdk/nvmf.h +++ b/include/spdk/nvmf.h @@ -501,13 +501,10 @@ uint32_t spdk_nvmf_subsystem_add_ns(struct spdk_nvmf_subsystem *subsystem, struc * * \param subsystem Subsystem the namespace belong to. * \param nsid Namespace ID to be removed. - * \param cb_fn Function to call when all thread local ns information has been updated - * \param cb_arg Context for the above cb_fn * * \return 0 on success, -1 on failure. */ -int spdk_nvmf_subsystem_remove_ns(struct spdk_nvmf_subsystem *subsystem, uint32_t nsid, - spdk_nvmf_subsystem_state_change_done cb_fn, void *cb_arg); +int spdk_nvmf_subsystem_remove_ns(struct spdk_nvmf_subsystem *subsystem, uint32_t nsid); /** * Get the first allocated namespace in a subsystem. diff --git a/lib/event/subsystems/nvmf/nvmf_rpc.c b/lib/event/subsystems/nvmf/nvmf_rpc.c index ecc7c24eb..e1d489cde 100644 --- a/lib/event/subsystems/nvmf/nvmf_rpc.c +++ b/lib/event/subsystems/nvmf/nvmf_rpc.c @@ -1228,13 +1228,14 @@ nvmf_rpc_remove_ns_resumed(struct spdk_nvmf_subsystem *subsystem, } static void -nvmf_rpc_remove_ns_remove_done(struct spdk_nvmf_subsystem *subsystem, void *cb_arg, int status) +nvmf_rpc_remove_ns_paused(struct spdk_nvmf_subsystem *subsystem, + void *cb_arg, int status) { - struct nvmf_rpc_remove_ns_ctx *ctx; + struct nvmf_rpc_remove_ns_ctx *ctx = cb_arg; + int ret; - ctx = cb_arg; - - if (status != 0) { + ret = spdk_nvmf_subsystem_remove_ns(subsystem, ctx->nsid); + if (ret < 0) { SPDK_ERRLOG("Unable to remove namespace ID %u\n", ctx->nsid); spdk_jsonrpc_send_error_response(ctx->request, SPDK_JSONRPC_ERROR_INVALID_PARAMS, "Invalid parameters"); @@ -1248,23 +1249,6 @@ nvmf_rpc_remove_ns_remove_done(struct spdk_nvmf_subsystem *subsystem, void *cb_a } } -static void -nvmf_rpc_remove_ns_paused(struct spdk_nvmf_subsystem *subsystem, - void *cb_arg, int status) -{ - struct nvmf_rpc_remove_ns_ctx *ctx = cb_arg; - int ret; - - ret = spdk_nvmf_subsystem_remove_ns(subsystem, ctx->nsid, nvmf_rpc_remove_ns_remove_done, ctx); - if (ret < 0) { - SPDK_ERRLOG("Unable to remove namespace ID %u\n", ctx->nsid); - spdk_jsonrpc_send_error_response(ctx->request, SPDK_JSONRPC_ERROR_INVALID_PARAMS, - "Invalid parameters"); - ctx->response_sent = true; - spdk_nvmf_subsystem_resume(subsystem, nvmf_rpc_remove_ns_resumed, ctx); - } -} - static void nvmf_rpc_subsystem_remove_ns(struct spdk_jsonrpc_request *request, const struct spdk_json_val *params) diff --git a/lib/nvmf/nvmf.c b/lib/nvmf/nvmf.c index 8d87d2c9f..5103defe5 100644 --- a/lib/nvmf/nvmf.c +++ b/lib/nvmf/nvmf.c @@ -531,19 +531,14 @@ poll_group_update_subsystem(struct spdk_nvmf_poll_group *group, sgroup->channels[i] = spdk_bdev_get_io_channel(ns->desc); } else { /* A namespace was present before and didn't change. */ + + /* TODO: Handle namespaces where the bdev was swapped out for a different one */ } } return 0; } -int -spdk_nvmf_poll_group_update_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem) -{ - return poll_group_update_subsystem(group, subsystem); -} - int spdk_nvmf_poll_group_add_subsystem(struct spdk_nvmf_poll_group *group, struct spdk_nvmf_subsystem *subsystem) diff --git a/lib/nvmf/nvmf_internal.h b/lib/nvmf/nvmf_internal.h index 2945e205a..196f978ba 100644 --- a/lib/nvmf/nvmf_internal.h +++ b/lib/nvmf/nvmf_internal.h @@ -234,8 +234,6 @@ struct spdk_nvmf_transport *spdk_nvmf_tgt_get_transport(struct spdk_nvmf_tgt *tg int spdk_nvmf_poll_group_add_transport(struct spdk_nvmf_poll_group *group, struct spdk_nvmf_transport *transport); -int spdk_nvmf_poll_group_update_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem); int spdk_nvmf_poll_group_add_subsystem(struct spdk_nvmf_poll_group *group, struct spdk_nvmf_subsystem *subsystem); int spdk_nvmf_poll_group_remove_subsystem(struct spdk_nvmf_poll_group *group, diff --git a/lib/nvmf/subsystem.c b/lib/nvmf/subsystem.c index 3b70720a6..cead2666b 100644 --- a/lib/nvmf/subsystem.c +++ b/lib/nvmf/subsystem.c @@ -316,8 +316,6 @@ _spdk_nvmf_subsystem_remove_host(struct spdk_nvmf_subsystem *subsystem, struct s free(host); } -static int _spdk_nvmf_subsystem_remove_ns(struct spdk_nvmf_subsystem *subsystem, uint32_t nsid); - void spdk_nvmf_subsystem_destroy(struct spdk_nvmf_subsystem *subsystem) { @@ -351,7 +349,7 @@ spdk_nvmf_subsystem_destroy(struct spdk_nvmf_subsystem *subsystem) while (ns != NULL) { struct spdk_nvmf_ns *next_ns = spdk_nvmf_subsystem_get_next_ns(subsystem, ns); - _spdk_nvmf_subsystem_remove_ns(subsystem, ns->opts.nsid); + spdk_nvmf_subsystem_remove_ns(subsystem, ns->opts.nsid); ns = next_ns; } @@ -821,58 +819,13 @@ spdk_nvmf_subsystem_get_next_listener(struct spdk_nvmf_subsystem *subsystem, return TAILQ_NEXT(prev_listener, link); } + const struct spdk_nvme_transport_id * spdk_nvmf_listener_get_trid(struct spdk_nvmf_listener *listener) { return &listener->trid; } -struct subsystem_update_ns_ctx { - struct spdk_nvmf_subsystem *subsystem; - - spdk_nvmf_subsystem_state_change_done cb_fn; - void *cb_arg; -}; - -static void -subsystem_update_ns_done(struct spdk_io_channel_iter *i, int status) -{ - struct subsystem_update_ns_ctx *ctx = spdk_io_channel_iter_get_ctx(i); - - if (ctx->cb_fn) { - ctx->cb_fn(ctx->subsystem, ctx->cb_arg, status); - } - free(ctx); -} - -static void -subsystem_update_ns_on_pg(struct spdk_io_channel_iter *i) -{ - int rc; - struct subsystem_update_ns_ctx *ctx; - struct spdk_nvmf_poll_group *group; - struct spdk_nvmf_subsystem *subsystem; - - ctx = spdk_io_channel_iter_get_ctx(i); - group = spdk_io_channel_get_ctx(spdk_io_channel_iter_get_channel(i)); - subsystem = ctx->subsystem; - - rc = spdk_nvmf_poll_group_update_subsystem(group, subsystem); - spdk_for_each_channel_continue(i, rc); -} - -static int -spdk_nvmf_subsystem_update_ns(struct spdk_nvmf_subsystem *subsystem, spdk_channel_for_each_cpl cpl, - void *ctx) -{ - spdk_for_each_channel(subsystem->tgt, - subsystem_update_ns_on_pg, - ctx, - cpl); - - return 0; -} - static void spdk_nvmf_subsystem_ns_changed(struct spdk_nvmf_subsystem *subsystem, uint32_t nsid) { @@ -883,8 +836,8 @@ spdk_nvmf_subsystem_ns_changed(struct spdk_nvmf_subsystem *subsystem, uint32_t n } } -static int -_spdk_nvmf_subsystem_remove_ns(struct spdk_nvmf_subsystem *subsystem, uint32_t nsid) +int +spdk_nvmf_subsystem_remove_ns(struct spdk_nvmf_subsystem *subsystem, uint32_t nsid) { struct spdk_nvmf_ns *ns; @@ -916,50 +869,15 @@ _spdk_nvmf_subsystem_remove_ns(struct spdk_nvmf_subsystem *subsystem, uint32_t n return 0; } -int -spdk_nvmf_subsystem_remove_ns(struct spdk_nvmf_subsystem *subsystem, uint32_t nsid, - spdk_nvmf_subsystem_state_change_done cb_fn, void *cb_arg) -{ - int rc; - struct subsystem_update_ns_ctx *ctx; - - rc = _spdk_nvmf_subsystem_remove_ns(subsystem, nsid); - if (rc < 0) { - return rc; - } - - ctx = calloc(1, sizeof(*ctx)); - - if (ctx == NULL) { - return -ENOMEM; - } - - ctx->subsystem = subsystem; - ctx->cb_fn = cb_fn; - ctx->cb_arg = cb_arg; - - spdk_nvmf_subsystem_update_ns(subsystem, subsystem_update_ns_done, ctx); - - return 0; -} - -static void -_spdk_nvmf_ns_hot_remove_done(struct spdk_nvmf_subsystem *subsystem, void *cb_arg, int status) -{ - if (status != 0) { - SPDK_ERRLOG("Failed to make changes to NVMe-oF subsystem with id %u\n", subsystem->id); - } - spdk_nvmf_subsystem_resume(subsystem, NULL, NULL); -} - static void _spdk_nvmf_ns_hot_remove(struct spdk_nvmf_subsystem *subsystem, void *cb_arg, int status) { struct spdk_nvmf_ns *ns = cb_arg; - spdk_nvmf_subsystem_remove_ns(subsystem, ns->opts.nsid, _spdk_nvmf_ns_hot_remove_done, - subsystem); + spdk_nvmf_subsystem_remove_ns(subsystem, ns->opts.nsid); + + spdk_nvmf_subsystem_resume(subsystem, NULL, NULL); } static void diff --git a/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c b/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c index 22faad015..7bc307444 100644 --- a/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c +++ b/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c @@ -163,13 +163,6 @@ spdk_nvmf_ctrlr_destruct(struct spdk_nvmf_ctrlr *ctrlr) { } -int -spdk_nvmf_poll_group_update_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem) -{ - return 0; -} - int spdk_nvmf_poll_group_add_subsystem(struct spdk_nvmf_poll_group *group, struct spdk_nvmf_subsystem *subsystem) diff --git a/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c b/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c index b8395e08f..3b0856329 100644 --- a/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c +++ b/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c @@ -39,17 +39,6 @@ SPDK_LOG_REGISTER_COMPONENT("nvmf", SPDK_LOG_NVMF) -static void -_subsystem_send_msg(spdk_thread_fn fn, void *ctx, void *thread_ctx) -{ - fn(ctx); -} - -static void -subsystem_ns_remove_cb(struct spdk_nvmf_subsystem *subsystem, void *cb_arg, int status) -{ -} - uint32_t spdk_env_get_current_core(void) { @@ -119,13 +108,6 @@ spdk_nvmf_tgt_get_transport(struct spdk_nvmf_tgt *tgt, enum spdk_nvme_transport_ return NULL; } -int -spdk_nvmf_poll_group_update_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem) -{ - return 0; -} - int spdk_nvmf_poll_group_add_subsystem(struct spdk_nvmf_poll_group *group, struct spdk_nvmf_subsystem *subsystem) @@ -233,11 +215,9 @@ spdk_bdev_get_uuid(const struct spdk_bdev *bdev) static void test_spdk_nvmf_subsystem_add_ns(void) { - struct spdk_nvmf_tgt tgt = {}; struct spdk_nvmf_subsystem subsystem = { .max_nsid = 0, .ns = NULL, - .tgt = &tgt }; struct spdk_bdev bdev1 = {}, bdev2 = {}; struct spdk_nvmf_ns_opts ns_opts; @@ -276,8 +256,8 @@ test_spdk_nvmf_subsystem_add_ns(void) CU_ASSERT(nsid == 0); CU_ASSERT(subsystem.max_nsid == 5); - spdk_nvmf_subsystem_remove_ns(&subsystem, 1, subsystem_ns_remove_cb, NULL); - spdk_nvmf_subsystem_remove_ns(&subsystem, 5, subsystem_ns_remove_cb, NULL); + spdk_nvmf_subsystem_remove_ns(&subsystem, 1); + spdk_nvmf_subsystem_remove_ns(&subsystem, 5); free(subsystem.ns); } @@ -447,12 +427,9 @@ int main(int argc, char **argv) return CU_get_error(); } - spdk_allocate_thread(_subsystem_send_msg, NULL, NULL, NULL, "thread0"); CU_basic_set_mode(CU_BRM_VERBOSE); CU_basic_run_tests(); num_failures = CU_get_number_of_failures(); CU_cleanup_registry(); - spdk_free_thread(); - return num_failures; }