From 4ca87a01b4f0e0f21eec2ec9d293f9227faa0e0f Mon Sep 17 00:00:00 2001 From: Seth Howell Date: Wed, 18 Apr 2018 09:31:40 -0700 Subject: [PATCH] nvmf: make spdk_nvmf_subsystem_remove_ns asynchronous Update the thread-local caches with new namespace data during each call to spdk_nvmf_subsystem_remove_ns to handle the case where the user requested to remove a namespace and then immediately add a different one at the same namespace id. This makes the call asynchronous. Change-Id: I8fd1968f7da78966386de18506b98d403b82d80e Signed-off-by: Seth Howell Reviewed-on: https://review.gerrithub.io/408220 Tested-by: SPDK Automated Test System Reviewed-by: Daniel Verkamp Reviewed-by: Ben Walker --- 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, 159 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45d16c271..f30ee8600 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,9 @@ 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 9dd6667d9..9f8a7f9a6 100644 --- a/include/spdk/nvmf.h +++ b/include/spdk/nvmf.h @@ -501,10 +501,13 @@ 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); +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); /** * 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 e1d489cde..ecc7c24eb 100644 --- a/lib/event/subsystems/nvmf/nvmf_rpc.c +++ b/lib/event/subsystems/nvmf/nvmf_rpc.c @@ -1228,14 +1228,13 @@ nvmf_rpc_remove_ns_resumed(struct spdk_nvmf_subsystem *subsystem, } static void -nvmf_rpc_remove_ns_paused(struct spdk_nvmf_subsystem *subsystem, - void *cb_arg, int status) +nvmf_rpc_remove_ns_remove_done(struct spdk_nvmf_subsystem *subsystem, void *cb_arg, int status) { - struct nvmf_rpc_remove_ns_ctx *ctx = cb_arg; - int ret; + struct nvmf_rpc_remove_ns_ctx *ctx; - ret = spdk_nvmf_subsystem_remove_ns(subsystem, ctx->nsid); - if (ret < 0) { + ctx = cb_arg; + + if (status != 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"); @@ -1249,6 +1248,23 @@ nvmf_rpc_remove_ns_paused(struct spdk_nvmf_subsystem *subsystem, } } +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 5103defe5..8d87d2c9f 100644 --- a/lib/nvmf/nvmf.c +++ b/lib/nvmf/nvmf.c @@ -531,14 +531,19 @@ 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 196f978ba..2945e205a 100644 --- a/lib/nvmf/nvmf_internal.h +++ b/lib/nvmf/nvmf_internal.h @@ -234,6 +234,8 @@ 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 cead2666b..3b70720a6 100644 --- a/lib/nvmf/subsystem.c +++ b/lib/nvmf/subsystem.c @@ -316,6 +316,8 @@ _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) { @@ -349,7 +351,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; } @@ -819,13 +821,58 @@ 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) { @@ -836,8 +883,8 @@ spdk_nvmf_subsystem_ns_changed(struct spdk_nvmf_subsystem *subsystem, uint32_t n } } -int -spdk_nvmf_subsystem_remove_ns(struct spdk_nvmf_subsystem *subsystem, uint32_t nsid) +static int +_spdk_nvmf_subsystem_remove_ns(struct spdk_nvmf_subsystem *subsystem, uint32_t nsid) { struct spdk_nvmf_ns *ns; @@ -869,15 +916,50 @@ spdk_nvmf_subsystem_remove_ns(struct spdk_nvmf_subsystem *subsystem, uint32_t ns 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_subsystem_resume(subsystem, NULL, NULL); + spdk_nvmf_subsystem_remove_ns(subsystem, ns->opts.nsid, _spdk_nvmf_ns_hot_remove_done, + subsystem); } 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 7bc307444..22faad015 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,6 +163,13 @@ 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 3b0856329..b8395e08f 100644 --- a/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c +++ b/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c @@ -39,6 +39,17 @@ 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) { @@ -108,6 +119,13 @@ 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) @@ -215,9 +233,11 @@ 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; @@ -256,8 +276,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); - spdk_nvmf_subsystem_remove_ns(&subsystem, 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); free(subsystem.ns); } @@ -427,9 +447,12 @@ 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; }