Commit 0e3de45d authored by Shuhei Matsumoto's avatar Shuhei Matsumoto Committed by Jim Harris
Browse files

bdev/nvme: Refactor add_trid() and rename it by add_secondary_trid() 



Clean up bdev_nvme_add_trid() by factoring out comparison of namespaces
and comparison of trids into helper functions, bdev_nvme_compare_namespaces()
and bdev_nvme_compare_trids(), respectively.

Then rename bdev_nvme_add_trid() by bdev_nvme_add_secondary_trid().

Rename is for clarification and clean-up is for the next patch.
The next patch will merge spdk_nvme_detach() and populate_namespaces_cb()
into the renamed bdev_nvme_secondary_trid(). Clean-up makes the
next patch simpler.

One note is that checking if the type of trid is not PCIe is done
by holding mutex now to prioritize clean up.

Signed-off-by: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Change-Id: Idc8652329dd2c721d101a724ec1a57a66c4174a7
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/7094


Community-CI: Broadcom CI
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
Reviewed-by: default avatarPaul Luse <paul.e.luse@intel.com>
Reviewed-by: default avatarAleksey Marchuk <alexeymar@mellanox.com>
parent 7366e569

module/bdev/nvme/bdev_nvme.c

+70 −38
Original line number Diff line number Diff line
@@ -2094,59 +2094,59 @@ nvme_ctrlr_populate_namespaces_done(struct nvme_bdev_ctrlr *nvme_bdev_ctrlr, 
	populate_namespaces_cb(ctx, j, 0);

}



static bool

bdev_nvme_compare_ns(struct spdk_nvme_ns *ns1, struct spdk_nvme_ns *ns2)

{

	const struct spdk_nvme_ns_data *nsdata1, *nsdata2;



	nsdata1 = spdk_nvme_ns_get_data(ns1);

	nsdata2 = spdk_nvme_ns_get_data(ns2);



	return memcmp(nsdata1->nguid, nsdata2->nguid, sizeof(nsdata1->nguid));

}



static int

bdev_nvme_add_trid(struct nvme_bdev_ctrlr *nvme_bdev_ctrlr, struct spdk_nvme_ctrlr *new_ctrlr,

bdev_nvme_compare_trids(struct nvme_bdev_ctrlr *nvme_bdev_ctrlr,

			struct spdk_nvme_ctrlr *new_ctrlr,

			struct spdk_nvme_transport_id *trid)

{

	uint32_t			i, nsid;

	struct nvme_bdev_ns		*nvme_ns;

	struct spdk_nvme_ns		*new_ns;

	struct nvme_bdev_ctrlr_trid	*new_trid, *tmp_trid;

	int				rc = 0;



	assert(nvme_bdev_ctrlr != NULL);

	struct nvme_bdev_ctrlr_trid *tmp_trid;



	if (trid->trtype == SPDK_NVME_TRANSPORT_PCIE) {

		SPDK_ERRLOG("PCIe failover is not supported.\n");

		return -ENOTSUP;

	}



	pthread_mutex_lock(&nvme_bdev_ctrlr->mutex);



	/* Currently we only support failover to the same transport type. */

	if (nvme_bdev_ctrlr->connected_trid->trtype != trid->trtype) {

		rc = -EINVAL;

		goto exit;

		return -EINVAL;

	}



	/* Currently we only support failover to the same NQN. */

	if (strncmp(trid->subnqn, nvme_bdev_ctrlr->connected_trid->subnqn, SPDK_NVMF_NQN_MAX_LEN)) {

		rc = -EINVAL;

		goto exit;

		return -EINVAL;

	}



	/* Skip all the other checks if we've already registered this path. */

	TAILQ_FOREACH(new_trid, &nvme_bdev_ctrlr->trids, link) {

		if (!spdk_nvme_transport_id_compare(&new_trid->trid, trid)) {

			rc = -EEXIST;

			goto exit;

	TAILQ_FOREACH(tmp_trid, &nvme_bdev_ctrlr->trids, link) {

		if (!spdk_nvme_transport_id_compare(&tmp_trid->trid, trid)) {

			return -EEXIST;

		}

	}



	return 0;

}



static bool

bdev_nvme_compare_ns(struct spdk_nvme_ns *ns1, struct spdk_nvme_ns *ns2)

{

	const struct spdk_nvme_ns_data *nsdata1, *nsdata2;



	nsdata1 = spdk_nvme_ns_get_data(ns1);

	nsdata2 = spdk_nvme_ns_get_data(ns2);



	return memcmp(nsdata1->nguid, nsdata2->nguid, sizeof(nsdata1->nguid));

}



static int

bdev_nvme_compare_namespaces(struct nvme_bdev_ctrlr *nvme_bdev_ctrlr,

			     struct spdk_nvme_ctrlr *new_ctrlr)

{

	uint32_t i, nsid;

	struct nvme_bdev_ns *nvme_ns;

	struct spdk_nvme_ns *new_ns;



	if (spdk_nvme_ctrlr_get_num_ns(new_ctrlr) != nvme_bdev_ctrlr->num_ns) {

		rc = -EINVAL;

		goto exit;

		return -EINVAL;

	}



	for (i = 0; i < nvme_bdev_ctrlr->num_ns; i++) {
@@ -2161,15 +2161,22 @@ bdev_nvme_add_trid(struct nvme_bdev_ctrlr *nvme_bdev_ctrlr, struct spdk_nvme_ctr 
		assert(new_ns != NULL);



		if (bdev_nvme_compare_ns(nvme_ns->ns, new_ns) != 0) {

			rc = -EINVAL;

			goto exit;

			return -EINVAL;

		}

	}



	return 0;

}



static int

_bdev_nvme_add_secondary_trid(struct nvme_bdev_ctrlr *nvme_bdev_ctrlr,

			      struct spdk_nvme_transport_id *trid)

{

	struct nvme_bdev_ctrlr_trid *new_trid, *tmp_trid;



	new_trid = calloc(1, sizeof(*new_trid));

	if (new_trid == NULL) {

		rc = -ENOMEM;

		goto exit;

		return -ENOMEM;

	}

	new_trid->trid = *trid;

	new_trid->is_failed = false;
@@ -2177,11 +2184,36 @@ bdev_nvme_add_trid(struct nvme_bdev_ctrlr *nvme_bdev_ctrlr, struct spdk_nvme_ctr 
	TAILQ_FOREACH(tmp_trid, &nvme_bdev_ctrlr->trids, link) {

		if (tmp_trid->is_failed) {

			TAILQ_INSERT_BEFORE(tmp_trid, new_trid, link);

			goto exit;

			return 0;

		}

	}



	TAILQ_INSERT_TAIL(&nvme_bdev_ctrlr->trids, new_trid, link);

	return 0;

}



static int

bdev_nvme_add_secondary_trid(struct nvme_bdev_ctrlr *nvme_bdev_ctrlr,

			     struct spdk_nvme_ctrlr *new_ctrlr,

			     struct spdk_nvme_transport_id *trid)

{

	int rc;



	assert(nvme_bdev_ctrlr != NULL);



	pthread_mutex_lock(&nvme_bdev_ctrlr->mutex);



	rc = bdev_nvme_compare_trids(nvme_bdev_ctrlr, new_ctrlr, trid);

	if (rc != 0) {

		goto exit;

	}



	rc = bdev_nvme_compare_namespaces(nvme_bdev_ctrlr, new_ctrlr);

	if (rc != 0) {

		goto exit;

	}



	rc = _bdev_nvme_add_secondary_trid(nvme_bdev_ctrlr, trid);



exit:

	pthread_mutex_unlock(&nvme_bdev_ctrlr->mutex);
@@ -2206,7 +2238,7 @@ connect_attach_cb(void *cb_ctx, const struct spdk_nvme_transport_id *trid, 
		 * nvme_bdev_ctrlr for failover. After checking if it can access the same

		 * namespaces as the primary path, it is disconnected until failover occurs.

		 */

		rc = bdev_nvme_add_trid(nvme_bdev_ctrlr, ctrlr, &ctx->trid);

		rc = bdev_nvme_add_secondary_trid(nvme_bdev_ctrlr, ctrlr, &ctx->trid);



		spdk_nvme_detach(ctrlr);

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

+4 −4
Original line number Diff line number Diff line
@@ -1235,7 +1235,7 @@ test_failover_ctrlr(void) 
	set_thread(0);



	/* Second, test two trids case. */

	rc = bdev_nvme_add_trid(nvme_bdev_ctrlr, &ctrlr, &trid2);

	rc = bdev_nvme_add_secondary_trid(nvme_bdev_ctrlr, &ctrlr, &trid2);

	CU_ASSERT(rc == 0);



	curr_trid = TAILQ_FIRST(&nvme_bdev_ctrlr->trids);
@@ -1879,7 +1879,7 @@ test_remove_trid(void) 
	nvme_bdev_ctrlr = nvme_bdev_ctrlr_get_by_name("nvme0");

	SPDK_CU_ASSERT_FATAL(nvme_bdev_ctrlr != NULL);



	rc = bdev_nvme_add_trid(nvme_bdev_ctrlr, &ctrlr, &trid2);

	rc = bdev_nvme_add_secondary_trid(nvme_bdev_ctrlr, &ctrlr, &trid2);

	CU_ASSERT(rc == 0);



	/* trid3 is not in the registered list. */
@@ -1894,7 +1894,7 @@ test_remove_trid(void) 
		CU_ASSERT(spdk_nvme_transport_id_compare(&ctrid->trid, &trid2) != 0);

	}



	rc = bdev_nvme_add_trid(nvme_bdev_ctrlr, &ctrlr, &trid3);

	rc = bdev_nvme_add_secondary_trid(nvme_bdev_ctrlr, &ctrlr, &trid3);

	CU_ASSERT(rc == 0);



	/* trid1 is currently used and trid3 is an alternative path.
@@ -1929,7 +1929,7 @@ test_remove_trid(void) 
	nvme_bdev_ctrlr = nvme_bdev_ctrlr_get_by_name("nvme0");

	SPDK_CU_ASSERT_FATAL(nvme_bdev_ctrlr != NULL);



	rc = bdev_nvme_add_trid(nvme_bdev_ctrlr, &ctrlr, &trid2);

	rc = bdev_nvme_add_secondary_trid(nvme_bdev_ctrlr, &ctrlr, &trid2);

	CU_ASSERT(rc == 0);



	/* If trid is not specified, nvme_bdev_ctrlr itself is removed. */