lib/nvmf: Add an API spdk_nvmf_subsystem_add_ns_ext() to pass bdev_name

Add an new API spdk_nvmf_subsystem_add_ns_ext() to pass not bdev but
bdev_name to fix the race condition due to the time gap between
spdk_bdev_get_by_name() and spdk_bdev_open(). A pointer to a bdev is
valid only while the bdev is opened.

spdk_bdev_open() has been replaced by spdk_bdev_open_ext() but the
issue still existed.

Update the corresponding unit tests accordingly.

Then replace the internal of spdk_nvmf_subsystem_add_ns() by
spdk_nvmf_subsystem_add_ns_ext() call.

Signed-off-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Change-Id: Ifcaa2121129ef22d5e61c9a8f7c640ff37a64485
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/4485
Community-CI: Broadcom CI
Community-CI: Mellanox Build Bot
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-by: Aleksey Marchuk <alexeymar@mellanox.com>

include/spdk/nvmf.h

@@ -701,7 +701,7 @@ struct spdk_nvmf_ns_opts {
 void spdk_nvmf_ns_opts_get_defaults(struct spdk_nvmf_ns_opts *opts, size_t opts_size);
 
 /**
- * Add a namespace to a subsytem.
+ * Add a namespace to a subsytem (deprecated, please use spdk_nvmf_subsystem_add_ns_ext).
  *
  * May only be performed on subsystems in the PAUSED or INACTIVE states.
  *
@@ -717,6 +717,24 @@ uint32_t spdk_nvmf_subsystem_add_ns(struct spdk_nvmf_subsystem *subsystem, struc
 				    const struct spdk_nvmf_ns_opts *opts, size_t opts_size,
 				    const char *ptpl_file);
 
+/**
+ * Add a namespace to a subsystems in the PAUSED or INACTIVE states.
+ *
+ * May only be performed on subsystems in the PAUSED or INACTIVE states.
+ *
+ * \param subsystem Subsystem to add namespace to.
+ * \param bdev_name Block device name to add as a namespace.
+ * \param opts Namespace options, or NULL to use defaults.
+ * \param opts_size sizeof(*opts)
+ * \param ptpl_file Persist through power loss file path.
+ *
+ * \return newly added NSID on success, or 0 on failure.
+ */
+uint32_t spdk_nvmf_subsystem_add_ns_ext(struct spdk_nvmf_subsystem *subsystem,
+					const char *bdev_name,
+					const struct spdk_nvmf_ns_opts *opts, size_t opts_size,
+					const char *ptpl_file);
+
 /**
  * Remove a namespace from a subsytem.
  *

lib/nvmf/nvmf_rpc.c

@@ -1262,16 +1262,6 @@ nvmf_rpc_ns_paused(struct spdk_nvmf_subsystem *subsystem,
 {
 	struct nvmf_rpc_ns_ctx *ctx = cb_arg;
 	struct spdk_nvmf_ns_opts ns_opts;
-	struct spdk_bdev *bdev;
-
-	bdev = spdk_bdev_get_by_name(ctx->ns_params.bdev_name);
-	if (!bdev) {
-		SPDK_ERRLOG("No bdev with name %s\n", ctx->ns_params.bdev_name);
-		spdk_jsonrpc_send_error_response(ctx->request, SPDK_JSONRPC_ERROR_INVALID_PARAMS,
-						 "Invalid parameters");
-		ctx->response_sent = true;
-		goto resume;
-	}
 
 	spdk_nvmf_ns_opts_get_defaults(&ns_opts, sizeof(ns_opts));
 	ns_opts.nsid = ctx->ns_params.nsid;
@@ -1286,7 +1276,8 @@ nvmf_rpc_ns_paused(struct spdk_nvmf_subsystem *subsystem,
 		ns_opts.uuid = ctx->ns_params.uuid;
 	}
 
-	ctx->ns_params.nsid = spdk_nvmf_subsystem_add_ns(subsystem, bdev, &ns_opts, sizeof(ns_opts),
+	ctx->ns_params.nsid = spdk_nvmf_subsystem_add_ns_ext(subsystem, ctx->ns_params.bdev_name,
+			      &ns_opts, sizeof(ns_opts),
 			      ctx->ns_params.ptpl_file);
 	if (ctx->ns_params.nsid == 0) {
 		SPDK_ERRLOG("Unable to add namespace\n");

lib/nvmf/spdk_nvmf.map

@@ -47,6 +47,7 @@
 	spdk_nvmf_subsytem_any_listener_allowed;
 	spdk_nvmf_ns_opts_get_defaults;
 	spdk_nvmf_subsystem_add_ns;
+	spdk_nvmf_subsystem_add_ns_ext;
 	spdk_nvmf_subsystem_remove_ns;
 	spdk_nvmf_subsystem_get_first_ns;
 	spdk_nvmf_subsystem_get_next_ns;

lib/nvmf/subsystem.c

@@ -1262,7 +1262,7 @@ static int
 nvmf_ns_reservation_restore(struct spdk_nvmf_ns *ns, struct spdk_nvmf_reservation_info *info);
 
 uint32_t
-spdk_nvmf_subsystem_add_ns(struct spdk_nvmf_subsystem *subsystem, struct spdk_bdev *bdev,
+spdk_nvmf_subsystem_add_ns_ext(struct spdk_nvmf_subsystem *subsystem, const char *bdev_name,
 			       const struct spdk_nvmf_ns_opts *user_opts, size_t opts_size,
 			       const char *ptpl_file)
 {
@@ -1276,20 +1276,11 @@ spdk_nvmf_subsystem_add_ns(struct spdk_nvmf_subsystem *subsystem, struct spdk_bd
 		return 0;
 	}
 
-	if (spdk_bdev_get_md_size(bdev) != 0 && !spdk_bdev_is_md_interleaved(bdev)) {
-		SPDK_ERRLOG("Can't attach bdev with separate metadata.\n");
-		return 0;
-	}
-
 	spdk_nvmf_ns_opts_get_defaults(&opts, sizeof(opts));
 	if (user_opts) {
 		memcpy(&opts, user_opts, spdk_min(sizeof(opts), opts_size));
 	}
 
-	if (spdk_mem_all_zero(&opts.uuid, sizeof(opts.uuid))) {
-		opts.uuid = *spdk_bdev_get_uuid(bdev);
-	}
-
 	if (opts.nsid == SPDK_NVME_GLOBAL_NS_TAG) {
 		SPDK_ERRLOG("Invalid NSID %" PRIu32 "\n", opts.nsid);
 		return 0;
@@ -1347,22 +1338,36 @@ spdk_nvmf_subsystem_add_ns(struct spdk_nvmf_subsystem *subsystem, struct spdk_bd
 		return 0;
 	}
 
-	ns->bdev = bdev;
+	rc = spdk_bdev_open_ext(bdev_name, true, nvmf_ns_event, ns, &ns->desc);
-	ns->opts = opts;
-	ns->subsystem = subsystem;
-	rc = spdk_bdev_open_ext(bdev->name, true, nvmf_ns_event, ns, &ns->desc);
 	if (rc != 0) {
 		SPDK_ERRLOG("Subsystem %s: bdev %s cannot be opened, error=%d\n",
-			    subsystem->subnqn, spdk_bdev_get_name(bdev), rc);
+			    subsystem->subnqn, bdev_name, rc);
 		free(ns);
 		return 0;
 	}
-	rc = spdk_bdev_module_claim_bdev(bdev, ns->desc, &ns_bdev_module);
+
+	ns->bdev = spdk_bdev_desc_get_bdev(ns->desc);
+
+	if (spdk_bdev_get_md_size(ns->bdev) != 0 && !spdk_bdev_is_md_interleaved(ns->bdev)) {
+		SPDK_ERRLOG("Can't attach bdev with separate metadata.\n");
+		spdk_bdev_close(ns->desc);
+		free(ns);
+		return 0;
+	}
+
+	rc = spdk_bdev_module_claim_bdev(ns->bdev, ns->desc, &ns_bdev_module);
 	if (rc != 0) {
 		spdk_bdev_close(ns->desc);
 		free(ns);
 		return 0;
 	}
+
+	if (spdk_mem_all_zero(&opts.uuid, sizeof(opts.uuid))) {
+		opts.uuid = *spdk_bdev_get_uuid(ns->bdev);
+	}
+
+	ns->opts = opts;
+	ns->subsystem = subsystem;
 	subsystem->ns[opts.nsid - 1] = ns;
 	ns->nsid = opts.nsid;
 	TAILQ_INIT(&ns->registrants);
@@ -1374,6 +1379,7 @@ spdk_nvmf_subsystem_add_ns(struct spdk_nvmf_subsystem *subsystem, struct spdk_bd
 			if (rc) {
 				SPDK_ERRLOG("Subsystem restore reservation failed\n");
 				subsystem->ns[opts.nsid - 1] = NULL;
+				spdk_bdev_module_release_bdev(ns->bdev);
 				spdk_bdev_close(ns->desc);
 				free(ns);
 				return 0;
@@ -1384,7 +1390,7 @@ spdk_nvmf_subsystem_add_ns(struct spdk_nvmf_subsystem *subsystem, struct spdk_bd
 
 	SPDK_DEBUGLOG(nvmf, "Subsystem %s: bdev %s assigned nsid %" PRIu32 "\n",
 		      spdk_nvmf_subsystem_get_nqn(subsystem),
-		      spdk_bdev_get_name(bdev),
+		      bdev_name,
 		      opts.nsid);
 
 	nvmf_subsystem_ns_changed(subsystem, opts.nsid);
@@ -1392,6 +1398,15 @@ spdk_nvmf_subsystem_add_ns(struct spdk_nvmf_subsystem *subsystem, struct spdk_bd
 	return opts.nsid;
 }
 
+uint32_t
+spdk_nvmf_subsystem_add_ns(struct spdk_nvmf_subsystem *subsystem, struct spdk_bdev *bdev,
+			   const struct spdk_nvmf_ns_opts *user_opts, size_t opts_size,
+			   const char *ptpl_file)
+{
+	return spdk_nvmf_subsystem_add_ns_ext(subsystem, spdk_bdev_get_name(bdev),
+					      user_opts, opts_size, ptpl_file);
+}
+
 static uint32_t
 nvmf_subsystem_get_next_allocated_nsid(struct spdk_nvmf_subsystem *subsystem,
 				       uint32_t prev_nsid)

module/event/subsystems/nvmf/conf.c

@@ -365,7 +365,6 @@ nvmf_parse_subsystem(struct spdk_conf_section *sp)
 
 	for (i = 0; ; i++) {
 		struct spdk_nvmf_ns_opts ns_opts;
-		struct spdk_bdev *bdev;
 		const char *bdev_name;
 		const char *uuid_str;
 		char *nsid_str;
@@ -375,14 +374,6 @@ nvmf_parse_subsystem(struct spdk_conf_section *sp)
 			break;
 		}
 
-		bdev = spdk_bdev_get_by_name(bdev_name);
-		if (bdev == NULL) {
-			SPDK_ERRLOG("Could not find namespace bdev '%s'\n", bdev_name);
-			spdk_nvmf_subsystem_destroy(subsystem);
-			subsystem = NULL;
-			goto done;
-		}
-
 		spdk_nvmf_ns_opts_get_defaults(&ns_opts, sizeof(ns_opts));
 
 		nsid_str = spdk_conf_section_get_nmval(sp, "Namespace", i, 1);
@@ -410,7 +401,7 @@ nvmf_parse_subsystem(struct spdk_conf_section *sp)
 			}
 		}
 
-		if (spdk_nvmf_subsystem_add_ns(subsystem, bdev, &ns_opts, sizeof(ns_opts), NULL) == 0) {
+		if (spdk_nvmf_subsystem_add_ns_ext(subsystem, bdev_name, &ns_opts, sizeof(ns_opts), NULL) == 0) {
 			SPDK_ERRLOG("Unable to add namespace\n");
 			spdk_nvmf_subsystem_destroy(subsystem);
 			subsystem = NULL;

test/unit/lib/nvmf/subsystem.c/subsystem_ut.c

@@ -200,16 +200,47 @@ nvmf_ctrlr_ns_changed(struct spdk_nvmf_ctrlr *ctrlr, uint32_t nsid)
 	g_ns_changed_nsid = nsid;
 }
 
+static struct spdk_bdev g_bdevs[] = {
+	{ .name = "bdev1" },
+	{ .name = "bdev2" },
+};
+
+struct spdk_bdev_desc {
+	struct spdk_bdev	*bdev;
+};
+
 int
 spdk_bdev_open_ext(const char *bdev_name, bool write, spdk_bdev_event_cb_t event_cb,
 		   void *event_ctx, struct spdk_bdev_desc **_desc)
 {
+	struct spdk_bdev_desc *desc;
+	size_t i;
+
+	for (i = 0; i < sizeof(g_bdevs); i++) {
+		if (strcmp(bdev_name, g_bdevs[i].name) == 0) {
+
+			desc = calloc(1, sizeof(*desc));
+			SPDK_CU_ASSERT_FATAL(desc != NULL);
+
+			desc->bdev = &g_bdevs[i];
+			*_desc = desc;
 			return 0;
 		}
+	}
+
+	return -EINVAL;
+}
 
 void
 spdk_bdev_close(struct spdk_bdev_desc *desc)
 {
+	free(desc);
+}
+
+struct spdk_bdev *
+spdk_bdev_desc_get_bdev(struct spdk_bdev_desc *desc)
+{
+	return desc->bdev;
 }
 
 const char *
@@ -233,7 +264,6 @@ test_spdk_nvmf_subsystem_add_ns(void)
 		.ns = NULL,
 		.tgt = &tgt
 	};
-	struct spdk_bdev bdev1 = {}, bdev2 = {};
 	struct spdk_nvmf_ns_opts ns_opts;
 	uint32_t nsid;
 	int rc;
@@ -244,34 +274,34 @@ test_spdk_nvmf_subsystem_add_ns(void)
 
 	/* Allow NSID to be assigned automatically */
 	spdk_nvmf_ns_opts_get_defaults(&ns_opts, sizeof(ns_opts));
-	nsid = spdk_nvmf_subsystem_add_ns(&subsystem, &bdev1, &ns_opts, sizeof(ns_opts), NULL);
+	nsid = spdk_nvmf_subsystem_add_ns_ext(&subsystem, "bdev1", &ns_opts, sizeof(ns_opts), NULL);
 	/* NSID 1 is the first unused ID */
 	CU_ASSERT(nsid == 1);
 	CU_ASSERT(subsystem.max_nsid == 1);
 	SPDK_CU_ASSERT_FATAL(subsystem.ns != NULL);
 	SPDK_CU_ASSERT_FATAL(subsystem.ns[nsid - 1] != NULL);
-	CU_ASSERT(subsystem.ns[nsid - 1]->bdev == &bdev1);
+	CU_ASSERT(subsystem.ns[nsid - 1]->bdev == &g_bdevs[nsid - 1]);
 
 	/* Request a specific NSID */
 	spdk_nvmf_ns_opts_get_defaults(&ns_opts, sizeof(ns_opts));
 	ns_opts.nsid = 5;
-	nsid = spdk_nvmf_subsystem_add_ns(&subsystem, &bdev2, &ns_opts, sizeof(ns_opts), NULL);
+	nsid = spdk_nvmf_subsystem_add_ns_ext(&subsystem, "bdev2", &ns_opts, sizeof(ns_opts), NULL);
 	CU_ASSERT(nsid == 5);
 	CU_ASSERT(subsystem.max_nsid == 5);
 	SPDK_CU_ASSERT_FATAL(subsystem.ns[nsid - 1] != NULL);
-	CU_ASSERT(subsystem.ns[nsid - 1]->bdev == &bdev2);
+	CU_ASSERT(subsystem.ns[nsid - 1]->bdev == &g_bdevs[1]);
 
 	/* Request an NSID that is already in use */
 	spdk_nvmf_ns_opts_get_defaults(&ns_opts, sizeof(ns_opts));
 	ns_opts.nsid = 5;
-	nsid = spdk_nvmf_subsystem_add_ns(&subsystem, &bdev2, &ns_opts, sizeof(ns_opts), NULL);
+	nsid = spdk_nvmf_subsystem_add_ns_ext(&subsystem, "bdev2", &ns_opts, sizeof(ns_opts), NULL);
 	CU_ASSERT(nsid == 0);
 	CU_ASSERT(subsystem.max_nsid == 5);
 
 	/* Request 0xFFFFFFFF (invalid NSID, reserved for broadcast) */
 	spdk_nvmf_ns_opts_get_defaults(&ns_opts, sizeof(ns_opts));
 	ns_opts.nsid = 0xFFFFFFFF;
-	nsid = spdk_nvmf_subsystem_add_ns(&subsystem, &bdev2, &ns_opts, sizeof(ns_opts), NULL);
+	nsid = spdk_nvmf_subsystem_add_ns_ext(&subsystem, "bdev2", &ns_opts, sizeof(ns_opts), NULL);
 	CU_ASSERT(nsid == 0);
 	CU_ASSERT(subsystem.max_nsid == 5);
 
@@ -1253,9 +1283,9 @@ test_spdk_nvmf_ns_event(void)
 	struct spdk_nvmf_ctrlr ctrlr = {
 		.subsys = &subsystem
 	};
-	struct spdk_bdev bdev1 = {};
 	struct spdk_nvmf_ns_opts ns_opts;
 	uint32_t nsid;
+	struct spdk_bdev *bdev;
 
 	tgt.max_subsystems = 1024;
 	tgt.subsystems = calloc(tgt.max_subsystems, sizeof(struct spdk_nvmf_subsystem *));
@@ -1263,9 +1293,12 @@ test_spdk_nvmf_ns_event(void)
 
 	/* Add one namespace */
 	spdk_nvmf_ns_opts_get_defaults(&ns_opts, sizeof(ns_opts));
-	nsid = spdk_nvmf_subsystem_add_ns(&subsystem, &bdev1, &ns_opts, sizeof(ns_opts), NULL);
+	nsid = spdk_nvmf_subsystem_add_ns_ext(&subsystem, "bdev1", &ns_opts, sizeof(ns_opts), NULL);
 	CU_ASSERT(nsid == 1);
 	CU_ASSERT(NULL != subsystem.ns[0]);
+	CU_ASSERT(subsystem.ns[nsid - 1]->bdev == &g_bdevs[nsid - 1]);
+
+	bdev = subsystem.ns[nsid - 1]->bdev;
 
 	/* Add one controller */
 	TAILQ_INIT(&subsystem.ctrlrs);
@@ -1275,7 +1308,7 @@ test_spdk_nvmf_ns_event(void)
 	subsystem.state = SPDK_NVMF_SUBSYSTEM_ACTIVE;
 	g_ns_changed_nsid = 0xFFFFFFFF;
 	g_ns_changed_ctrlr = NULL;
-	nvmf_ns_event(SPDK_BDEV_EVENT_RESIZE, &bdev1, subsystem.ns[0]);
+	nvmf_ns_event(SPDK_BDEV_EVENT_RESIZE, bdev, subsystem.ns[0]);
 	CU_ASSERT(SPDK_NVMF_SUBSYSTEM_PAUSING == subsystem.state);
 
 	poll_threads();
@@ -1287,7 +1320,7 @@ test_spdk_nvmf_ns_event(void)
 	subsystem.state = SPDK_NVMF_SUBSYSTEM_ACTIVE;
 	g_ns_changed_nsid = 0xFFFFFFFF;
 	g_ns_changed_ctrlr = NULL;
-	nvmf_ns_event(SPDK_BDEV_EVENT_REMOVE, &bdev1, subsystem.ns[0]);
+	nvmf_ns_event(SPDK_BDEV_EVENT_REMOVE, bdev, subsystem.ns[0]);
 	CU_ASSERT(SPDK_NVMF_SUBSYSTEM_PAUSING == subsystem.state);
 	CU_ASSERT(0xFFFFFFFF == g_ns_changed_nsid);
 	CU_ASSERT(NULL == g_ns_changed_ctrlr);