bdev/nvme: Separate attach_cb between creating ctrlr and setting failover

The NVMe bdev module will have two similar features, multipath and
failover when it supports multipath.

Take a case that we add two different trids with the same name by the
bdev_nvme_attach_controller RPC as an example.

The failover adds secondary trid to an existing nvme_ctrlr. The multipath
feature creates another nvme_ctrlr and adds it to the same nvme_bdev_ctrlr
which has an existing nvme_ctrlr.

We want to use bdev_nvme_attach_controller for both failover and multipath.
To do it cleanly, separate callback to spdk_nvme_connect_async() between
creating ctrlr and setting failover.

Signed-off-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Change-Id: Id9bc175af6201cdd74e12d4903fc81afe4f91189
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/9225
Community-CI: Broadcom CI <spdk-ci.pdl@broadcom.com>
Community-CI: Mellanox Build Bot
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Konrad Sztyber <konrad.sztyber@gmail.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>

module/bdev/nvme/bdev_nvme.c

@@ -2599,7 +2599,25 @@ connect_attach_cb(void *cb_ctx, const struct spdk_nvme_transport_id *trid,
 		  struct spdk_nvme_ctrlr *ctrlr, const struct spdk_nvme_ctrlr_opts *opts)
 {
 	struct spdk_nvme_ctrlr_opts *user_opts = cb_ctx;
-	struct nvme_ctrlr	*nvme_ctrlr;
+	struct nvme_async_probe_ctx *ctx;
+	int rc;
+
+	ctx = SPDK_CONTAINEROF(user_opts, struct nvme_async_probe_ctx, opts);
+	ctx->ctrlr_attached = true;
+
+	rc = nvme_ctrlr_create(ctrlr, ctx->base_name, &ctx->trid, ctx->prchk_flags, ctx);
+	if (rc != 0) {
+		populate_namespaces_cb(ctx, 0, rc);
+	}
+}
+
+static void
+connect_set_failover_cb(void *cb_ctx, const struct spdk_nvme_transport_id *trid,
+			struct spdk_nvme_ctrlr *ctrlr,
+			const struct spdk_nvme_ctrlr_opts *opts)
+{
+	struct spdk_nvme_ctrlr_opts *user_opts = cb_ctx;
+	struct nvme_ctrlr *nvme_ctrlr;
 	struct nvme_async_probe_ctx *ctx;
 	int rc;
 
@@ -2610,10 +2628,7 @@ connect_attach_cb(void *cb_ctx, const struct spdk_nvme_transport_id *trid,
 	if (nvme_ctrlr) {
 		rc = bdev_nvme_add_secondary_trid(nvme_ctrlr, ctrlr, &ctx->trid);
 	} else {
-		rc = nvme_ctrlr_create(ctrlr, ctx->base_name, &ctx->trid, ctx->prchk_flags, ctx);
+		rc = -ENODEV;
-		if (rc == 0) {
-			return;
-		}
 	}
 
 	populate_namespaces_cb(ctx, 0, rc);
@@ -2662,6 +2677,7 @@ bdev_nvme_create(struct spdk_nvme_transport_id *trid,
 {
 	struct nvme_probe_skip_entry	*entry, *tmp;
 	struct nvme_async_probe_ctx	*ctx;
+	spdk_nvme_attach_cb attach_cb;
 
 	/* TODO expand this check to include both the host and target TRIDs.
 	 * Only if both are the same should we fail.
@@ -2715,7 +2731,13 @@ bdev_nvme_create(struct spdk_nvme_transport_id *trid,
 		snprintf(ctx->opts.src_svcid, sizeof(ctx->opts.src_svcid), "%s", hostid->hostsvcid);
 	}
 
-	ctx->probe_ctx = spdk_nvme_connect_async(trid, &ctx->opts, connect_attach_cb);
+	if (nvme_ctrlr_get_by_name(base_name) == NULL) {
+		attach_cb = connect_attach_cb;
+	} else {
+		attach_cb = connect_set_failover_cb;
+	}
+
+	ctx->probe_ctx = spdk_nvme_connect_async(trid, &ctx->opts, attach_cb);
 	if (ctx->probe_ctx == NULL) {
 		SPDK_ERRLOG("No controller was found with provided trid (traddr: %s)\n", trid->traddr);
 		free(ctx);

test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c

@@ -2097,29 +2097,60 @@ test_submit_nvme_cmd(void)
 }
 
 static void
-test_remove_trid(void)
+test_add_remove_trid(void)
 {
 	struct spdk_nvme_transport_id trid1 = {}, trid2 = {}, trid3 = {};
-	struct spdk_nvme_ctrlr ctrlr = {};
+	struct spdk_nvme_host_id hostid = {};
+	struct spdk_nvme_ctrlr *ctrlr1, *ctrlr2, *ctrlr3;
 	struct nvme_ctrlr *nvme_ctrlr = NULL;
+	const int STRING_SIZE = 32;
+	const char *attached_names[STRING_SIZE];
 	struct nvme_ctrlr_trid *ctrid;
 	int rc;
 
+	memset(attached_names, 0, sizeof(char *) * STRING_SIZE);
 	ut_init_trid(&trid1);
 	ut_init_trid2(&trid2);
 	ut_init_trid3(&trid3);
 
 	set_thread(0);
 
-	rc = nvme_ctrlr_create(&ctrlr, "nvme0", &trid1, 0, NULL);
+	g_ut_attach_ctrlr_status = 0;
+	g_ut_attach_bdev_count = 0;
+
+	ctrlr1 = ut_attach_ctrlr(&trid1, 0, false);
+	SPDK_CU_ASSERT_FATAL(ctrlr1 != NULL);
+
+	rc = bdev_nvme_create(&trid1, &hostid, "nvme0", attached_names, STRING_SIZE, NULL, 0,
+			      attach_ctrlr_done, NULL, NULL);
 	CU_ASSERT(rc == 0);
 
+	spdk_delay_us(1000);
+	poll_threads();
+
 	nvme_ctrlr = nvme_ctrlr_get_by_name("nvme0");
 	SPDK_CU_ASSERT_FATAL(nvme_ctrlr != NULL);
 
-	rc = bdev_nvme_add_secondary_trid(nvme_ctrlr, &ctrlr, &trid2);
+	CU_ASSERT(spdk_nvme_transport_id_compare(nvme_ctrlr->connected_trid, &trid1) == 0);
+
+	ctrlr2 = ut_attach_ctrlr(&trid2, 0, false);
+	SPDK_CU_ASSERT_FATAL(ctrlr2 != NULL);
+
+	rc = bdev_nvme_create(&trid2, &hostid, "nvme0", attached_names, STRING_SIZE, NULL, 0,
+			      attach_ctrlr_done, NULL, NULL);
 	CU_ASSERT(rc == 0);
 
+	spdk_delay_us(1000);
+	poll_threads();
+
+	CU_ASSERT(spdk_nvme_transport_id_compare(nvme_ctrlr->connected_trid, &trid1) == 0);
+	TAILQ_FOREACH(ctrid, &nvme_ctrlr->trids, link) {
+		if (spdk_nvme_transport_id_compare(&ctrid->trid, &trid2) == 0) {
+			break;
+		}
+	}
+	CU_ASSERT(ctrid != NULL);
+
 	/* trid3 is not in the registered list. */
 	rc = bdev_nvme_delete("nvme0", &trid3);
 	CU_ASSERT(rc == -ENXIO);
@@ -2132,9 +2163,24 @@ test_remove_trid(void)
 		CU_ASSERT(spdk_nvme_transport_id_compare(&ctrid->trid, &trid2) != 0);
 	}
 
-	rc = bdev_nvme_add_secondary_trid(nvme_ctrlr, &ctrlr, &trid3);
+	ctrlr3 = ut_attach_ctrlr(&trid3, 0, false);
+	SPDK_CU_ASSERT_FATAL(ctrlr3 != NULL);
+
+	rc = bdev_nvme_create(&trid3, &hostid, "nvme0", attached_names, STRING_SIZE, NULL, 0,
+			      attach_ctrlr_done, NULL, NULL);
 	CU_ASSERT(rc == 0);
 
+	spdk_delay_us(1000);
+	poll_threads();
+
+	CU_ASSERT(spdk_nvme_transport_id_compare(nvme_ctrlr->connected_trid, &trid1) == 0);
+	TAILQ_FOREACH(ctrid, &nvme_ctrlr->trids, link) {
+		if (spdk_nvme_transport_id_compare(&ctrid->trid, &trid3) == 0) {
+			break;
+		}
+	}
+	CU_ASSERT(ctrid != NULL);
+
 	/* trid1 is currently used and trid3 is an alternative path.
 	 * If we remove trid1, path is changed to trid3.
 	 */
@@ -2162,15 +2208,39 @@ test_remove_trid(void)
 
 	CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL);
 
-	rc = nvme_ctrlr_create(&ctrlr, "nvme0", &trid1, 0, NULL);
+	ctrlr1 = ut_attach_ctrlr(&trid1, 0, false);
+	SPDK_CU_ASSERT_FATAL(ctrlr1 != NULL);
+
+	rc = bdev_nvme_create(&trid1, &hostid, "nvme0", attached_names, STRING_SIZE, NULL, 0,
+			      attach_ctrlr_done, NULL, NULL);
 	CU_ASSERT(rc == 0);
 
+	spdk_delay_us(1000);
+	poll_threads();
+
 	nvme_ctrlr = nvme_ctrlr_get_by_name("nvme0");
 	SPDK_CU_ASSERT_FATAL(nvme_ctrlr != NULL);
 
-	rc = bdev_nvme_add_secondary_trid(nvme_ctrlr, &ctrlr, &trid2);
+	CU_ASSERT(spdk_nvme_transport_id_compare(nvme_ctrlr->connected_trid, &trid1) == 0);
+
+	ctrlr2 = ut_attach_ctrlr(&trid2, 0, false);
+	SPDK_CU_ASSERT_FATAL(ctrlr2 != NULL);
+
+	rc = bdev_nvme_create(&trid2, &hostid, "nvme0", attached_names, STRING_SIZE, NULL, 0,
+			      attach_ctrlr_done, NULL, NULL);
 	CU_ASSERT(rc == 0);
 
+	spdk_delay_us(1000);
+	poll_threads();
+
+	CU_ASSERT(spdk_nvme_transport_id_compare(nvme_ctrlr->connected_trid, &trid1) == 0);
+	TAILQ_FOREACH(ctrid, &nvme_ctrlr->trids, link) {
+		if (spdk_nvme_transport_id_compare(&ctrid->trid, &trid2) == 0) {
+			break;
+		}
+	}
+	CU_ASSERT(ctrid != NULL);
+
 	/* If trid is not specified, nvme_ctrlr itself is removed. */
 	rc = bdev_nvme_delete("nvme0", NULL);
 	CU_ASSERT(rc == 0);
@@ -2645,7 +2715,7 @@ main(int argc, const char **argv)
 	CU_ADD_TEST(suite, test_reconnect_qpair);
 	CU_ADD_TEST(suite, test_aer_cb);
 	CU_ADD_TEST(suite, test_submit_nvme_cmd);
-	CU_ADD_TEST(suite, test_remove_trid);
+	CU_ADD_TEST(suite, test_add_remove_trid);
 	CU_ADD_TEST(suite, test_abort);
 	CU_ADD_TEST(suite, test_get_io_qpair);
 	CU_ADD_TEST(suite, test_bdev_unregister);