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 <seth.howell@intel.com>
Reviewed-on: https://review.gerrithub.io/408220
Tested-by: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Daniel Verkamp <daniel.verkamp@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>

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:

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.

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,
+nvmf_rpc_remove_ns_remove_done(struct spdk_nvmf_subsystem *subsystem, void *cb_arg, int status)
-			  void *cb_arg, int status)
 {
-	struct nvmf_rpc_remove_ns_ctx *ctx = cb_arg;
+	struct nvmf_rpc_remove_ns_ctx *ctx;
-	int ret;
 
-	ret = spdk_nvmf_subsystem_remove_ns(subsystem, ctx->nsid);
+	ctx = cb_arg;
-	if (ret < 0) {
+
+	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)

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)

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,

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
+static int
-spdk_nvmf_subsystem_remove_ns(struct spdk_nvmf_subsystem *subsystem, uint32_t nsid)
+_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_remove_ns(subsystem, ns->opts.nsid, _spdk_nvmf_ns_hot_remove_done,
-
+				      subsystem);
-	spdk_nvmf_subsystem_resume(subsystem, NULL, NULL);
 }
 
 static void

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)

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, 1, subsystem_ns_remove_cb, NULL);
-	spdk_nvmf_subsystem_remove_ns(&subsystem, 5);
+	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;
 }