From 88da8a91f956d131e3bdaf5b9c845a66868cd664 Mon Sep 17 00:00:00 2001 From: Ben Walker Date: Thu, 27 Jun 2019 04:46:42 -0700 Subject: [PATCH] nvmf: spdk_nvmf_subsystem_remove_ns is no longer asynchronous Now that the resume path can correctly handle the case where a namespace was removed and a new one added with the same nsid, this no longer needs to be asynchronous. Change-Id: I693045e66a7d4e75255b526d8f5ca5ef8695533e Signed-off-by: Ben Walker Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/459606 Reviewed-by: Changpeng Liu Reviewed-by: Shuhei Matsumoto Reviewed-by: Seth Howell Tested-by: SPDK CI Jenkins --- include/spdk/nvmf.h | 5 +- lib/event/subsystems/nvmf/nvmf_rpc.c | 32 ++++------- lib/nvmf/subsystem.c | 53 ++++--------------- test/unit/lib/nvmf/subsystem.c/subsystem_ut.c | 14 ++--- 4 files changed, 25 insertions(+), 79 deletions(-) diff --git a/include/spdk/nvmf.h b/include/spdk/nvmf.h index d1ca9d917..6fa3aebcc 100644 --- a/include/spdk/nvmf.h +++ b/include/spdk/nvmf.h @@ -588,13 +588,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 893c7e0c2..e00f60516 100644 --- a/lib/event/subsystems/nvmf/nvmf_rpc.c +++ b/lib/event/subsystems/nvmf/nvmf_rpc.c @@ -1002,13 +1002,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"); @@ -1016,29 +1017,14 @@ nvmf_rpc_remove_ns_remove_done(struct spdk_nvmf_subsystem *subsystem, void *cb_a } if (spdk_nvmf_subsystem_resume(subsystem, nvmf_rpc_remove_ns_resumed, ctx)) { - spdk_jsonrpc_send_error_response(ctx->request, SPDK_JSONRPC_ERROR_INTERNAL_ERROR, "Internal error"); + if (!ctx->response_sent) { + spdk_jsonrpc_send_error_response(ctx->request, SPDK_JSONRPC_ERROR_INTERNAL_ERROR, "Internal error"); + } nvmf_rpc_remove_ns_ctx_free(ctx); return; } } -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 spdk_rpc_nvmf_subsystem_remove_ns(struct spdk_jsonrpc_request *request, const struct spdk_json_val *params) diff --git a/lib/nvmf/subsystem.c b/lib/nvmf/subsystem.c index f5b6c7cc4..2c939d126 100644 --- a/lib/nvmf/subsystem.c +++ b/lib/nvmf/subsystem.c @@ -312,8 +312,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); - static void _nvmf_subsystem_remove_listener(struct spdk_nvmf_subsystem *subsystem, struct spdk_nvmf_listener *listener) @@ -361,7 +359,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; } @@ -895,8 +893,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; struct spdk_nvmf_registrant *reg, *reg_tmp; @@ -936,50 +934,19 @@ _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; + int rc; - spdk_nvmf_subsystem_remove_ns(subsystem, ns->opts.nsid, _spdk_nvmf_ns_hot_remove_done, - subsystem); + rc = spdk_nvmf_subsystem_remove_ns(subsystem, ns->opts.nsid); + if (rc != 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 diff --git a/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c b/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c index c5cb7f9b3..1fb20bb97 100644 --- a/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c +++ b/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c @@ -64,11 +64,6 @@ DEFINE_STUB(spdk_nvmf_transport_stop_listen, (struct spdk_nvmf_transport *transport, const struct spdk_nvme_transport_id *trid), 0); -static void -subsystem_ns_remove_cb(struct spdk_nvmf_subsystem *subsystem, void *cb_arg, int status) -{ -} - uint32_t spdk_env_get_current_core(void) { @@ -255,6 +250,7 @@ test_spdk_nvmf_subsystem_add_ns(void) struct spdk_bdev bdev1 = {}, bdev2 = {}; struct spdk_nvmf_ns_opts ns_opts; uint32_t nsid; + int rc; tgt.max_subsystems = 1024; tgt.subsystems = calloc(tgt.max_subsystems, sizeof(struct spdk_nvmf_subsystem *)); @@ -293,10 +289,10 @@ 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); - poll_threads(); - spdk_nvmf_subsystem_remove_ns(&subsystem, 5, subsystem_ns_remove_cb, NULL); - poll_threads(); + rc = spdk_nvmf_subsystem_remove_ns(&subsystem, 1); + CU_ASSERT(rc == 0); + rc = spdk_nvmf_subsystem_remove_ns(&subsystem, 5); + CU_ASSERT(rc == 0); free(subsystem.ns); free(tgt.subsystems);