Commit 3c8b6782 authored by Konrad Sztyber's avatar Konrad Sztyber Committed by Tomasz Zawadzki
Browse files

nvme: check hostnqn when attaching controllers 



This ensures that a new controller will be attached when a controller
with the same transport id, but a different hostnqn already exists.  It
can be useful in cases where the target returns different namespace
configuration based on the NQN of the host.

Additionally, because hostnqns are now always checked and the default
hostnqn is built from nvme_driver.default_extended_host_id, unit tests
had to be changed to ensure that this field is initialized.

Suggested-by: default avatarJim Harris <jim.harris@samsung.com>
Signed-off-by: default avatarKonrad Sztyber <konrad.sztyber@intel.com>
Change-Id: Ibb660d7f99ad67ff7c29658a7a7da7d8328b19d6
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/23718


Reviewed-by: default avatarAleksey Marchuk <alexeymar@nvidia.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarJim Harris <jim.harris@samsung.com>
Reviewed-by: default avatarKrzysztof Karas <krzysztof.karas@intel.com>
Community-CI: Mellanox Build Bot
parent c8404680

app/spdk_nvme_identify/identify.c

+3 −1
Original line number Diff line number Diff line
@@ -2926,7 +2926,9 @@ main(int argc, char **argv) 
		struct spdk_nvme_ctrlr_opts opts;



		spdk_nvme_ctrlr_get_default_ctrlr_opts(&opts, sizeof(opts));

		if (g_hostnqn[0] != '\0') {

			memcpy(opts.hostnqn, g_hostnqn, sizeof(opts.hostnqn));

		}

		ctrlr = spdk_nvme_connect(&g_trid, &opts, sizeof(opts));

		if (!ctrlr) {

			fprintf(stderr, "spdk_nvme_connect() failed\n");

lib/nvme/nvme.c

+34 −11
Original line number Diff line number Diff line
@@ -655,7 +655,7 @@ nvme_ctrlr_probe(const struct spdk_nvme_transport_id *trid, 
	spdk_nvme_ctrlr_get_default_ctrlr_opts(&opts, sizeof(opts));



	if (!probe_ctx->probe_cb || probe_ctx->probe_cb(probe_ctx->cb_ctx, trid, &opts)) {

		ctrlr = nvme_get_ctrlr_by_trid_unsafe(trid);

		ctrlr = nvme_get_ctrlr_by_trid_unsafe(trid, opts.hostnqn);

		if (ctrlr) {

			/* This ctrlr already exists. */
@@ -761,12 +761,12 @@ nvme_init_controllers(struct spdk_nvme_probe_ctx *probe_ctx) 


/* This function must not be called while holding g_spdk_nvme_driver->lock */

static struct spdk_nvme_ctrlr *

nvme_get_ctrlr_by_trid(const struct spdk_nvme_transport_id *trid)

nvme_get_ctrlr_by_trid(const struct spdk_nvme_transport_id *trid, const char *hostnqn)

{

	struct spdk_nvme_ctrlr *ctrlr;



	nvme_robust_mutex_lock(&g_spdk_nvme_driver->lock);

	ctrlr = nvme_get_ctrlr_by_trid_unsafe(trid);

	ctrlr = nvme_get_ctrlr_by_trid_unsafe(trid, hostnqn);

	nvme_robust_mutex_unlock(&g_spdk_nvme_driver->lock);



	return ctrlr;
@@ -774,22 +774,30 @@ nvme_get_ctrlr_by_trid(const struct spdk_nvme_transport_id *trid) 


/* This function must be called while holding g_spdk_nvme_driver->lock */

struct spdk_nvme_ctrlr *

nvme_get_ctrlr_by_trid_unsafe(const struct spdk_nvme_transport_id *trid)

nvme_get_ctrlr_by_trid_unsafe(const struct spdk_nvme_transport_id *trid, const char *hostnqn)

{

	struct spdk_nvme_ctrlr *ctrlr;



	/* Search per-process list */

	TAILQ_FOREACH(ctrlr, &g_nvme_attached_ctrlrs, tailq) {

		if (spdk_nvme_transport_id_compare(&ctrlr->trid, trid) == 0) {

			return ctrlr;

		if (spdk_nvme_transport_id_compare(&ctrlr->trid, trid) != 0) {

			continue;

		}

		if (hostnqn && strcmp(ctrlr->opts.hostnqn, hostnqn) != 0) {

			continue;

		}

		return ctrlr;

	}



	/* Search multi-process shared list */

	TAILQ_FOREACH(ctrlr, &g_spdk_nvme_driver->shared_attached_ctrlrs, tailq) {

		if (spdk_nvme_transport_id_compare(&ctrlr->trid, trid) == 0) {

			return ctrlr;

		if (spdk_nvme_transport_id_compare(&ctrlr->trid, trid) != 0) {

			continue;

		}

		if (hostnqn && strcmp(ctrlr->opts.hostnqn, hostnqn) != 0) {

			continue;

		}

		return ctrlr;

	}



	return NULL;
@@ -802,6 +810,7 @@ nvme_probe_internal(struct spdk_nvme_probe_ctx *probe_ctx, 
{

	int rc;

	struct spdk_nvme_ctrlr *ctrlr, *ctrlr_tmp;

	const struct spdk_nvme_ctrlr_opts *opts = probe_ctx->opts;



	if (strlen(probe_ctx->trid.trstring) == 0) {

		/* If user didn't provide trstring, derive it from trtype */
@@ -838,6 +847,10 @@ nvme_probe_internal(struct spdk_nvme_probe_ctx *probe_ctx, 
				continue;

			}



			if (opts && strcmp(opts->hostnqn, ctrlr->opts.hostnqn) != 0) {

				continue;

			}



			/* Do not attach if we failed to initialize it in this process */

			if (nvme_ctrlr_get_current_process(ctrlr) == NULL) {

				continue;
@@ -865,12 +878,14 @@ nvme_probe_internal(struct spdk_nvme_probe_ctx *probe_ctx, 
static void

nvme_probe_ctx_init(struct spdk_nvme_probe_ctx *probe_ctx,

		    const struct spdk_nvme_transport_id *trid,

		    const struct spdk_nvme_ctrlr_opts *opts,

		    void *cb_ctx,

		    spdk_nvme_probe_cb probe_cb,

		    spdk_nvme_attach_cb attach_cb,

		    spdk_nvme_remove_cb remove_cb)

{

	probe_ctx->trid = *trid;

	probe_ctx->opts = opts;

	probe_ctx->cb_ctx = cb_ctx;

	probe_ctx->probe_cb = probe_cb;

	probe_ctx->attach_cb = attach_cb;
@@ -989,15 +1004,23 @@ spdk_nvme_connect(const struct spdk_nvme_transport_id *trid, 
	struct spdk_nvme_probe_ctx *probe_ctx;

	struct spdk_nvme_ctrlr_opts *opts_local_p = NULL;

	struct spdk_nvme_ctrlr_opts opts_local;

	char hostnqn[SPDK_NVMF_NQN_MAX_LEN + 1];



	if (trid == NULL) {

		SPDK_ERRLOG("No transport ID specified\n");

		return NULL;

	}



	rc = nvme_driver_init();

	if (rc != 0) {

		return NULL;

	}



	nvme_get_default_hostnqn(hostnqn, sizeof(hostnqn));

	if (opts) {

		opts_local_p = &opts_local;

		nvme_ctrlr_opts_init(opts_local_p, opts, opts_size);

		memcpy(hostnqn, opts_local.hostnqn, sizeof(hostnqn));

	}



	probe_ctx = spdk_nvme_connect_async(trid, opts_local_p, NULL);
@@ -1011,7 +1034,7 @@ spdk_nvme_connect(const struct spdk_nvme_transport_id *trid, 
		return NULL;

	}



	ctrlr = nvme_get_ctrlr_by_trid(trid);

	ctrlr = nvme_get_ctrlr_by_trid(trid, hostnqn);



	return ctrlr;

}
@@ -1501,7 +1524,7 @@ spdk_nvme_probe_async(const struct spdk_nvme_transport_id *trid, 
		return NULL;

	}



	nvme_probe_ctx_init(probe_ctx, trid, cb_ctx, probe_cb, attach_cb, remove_cb);

	nvme_probe_ctx_init(probe_ctx, trid, NULL, cb_ctx, probe_cb, attach_cb, remove_cb);

	rc = nvme_probe_internal(probe_ctx, false);

	if (rc != 0) {

		free(probe_ctx);
@@ -1559,7 +1582,7 @@ spdk_nvme_connect_async(const struct spdk_nvme_transport_id *trid, 
		probe_cb = nvme_connect_probe_cb;

	}



	nvme_probe_ctx_init(probe_ctx, trid, (void *)opts, probe_cb, attach_cb, NULL);

	nvme_probe_ctx_init(probe_ctx, trid, opts, (void *)opts, probe_cb, attach_cb, NULL);

	rc = nvme_probe_internal(probe_ctx, true);

	if (rc != 0) {

		free(probe_ctx);

lib/nvme/nvme_internal.h

+2 −1
Original line number Diff line number Diff line
@@ -1097,6 +1097,7 @@ struct spdk_nvme_ctrlr { 


struct spdk_nvme_probe_ctx {

	struct spdk_nvme_transport_id		trid;

	const struct spdk_nvme_ctrlr_opts	*opts;

	void					*cb_ctx;

	spdk_nvme_probe_cb			probe_cb;

	spdk_nvme_attach_cb			attach_cb;
@@ -1610,7 +1611,7 @@ int nvme_robust_mutex_init_recursive_shared(pthread_mutex_t *mtx); 
bool	nvme_completion_is_retry(const struct spdk_nvme_cpl *cpl);



struct spdk_nvme_ctrlr *nvme_get_ctrlr_by_trid_unsafe(

	const struct spdk_nvme_transport_id *trid);

	const struct spdk_nvme_transport_id *trid, const char *hostnqn);



const struct spdk_nvme_transport *nvme_get_transport(const char *transport_name);

const struct spdk_nvme_transport *nvme_get_first_transport(void);

lib/nvme/nvme_pcie.c

+2 −2
Original line number Diff line number Diff line
@@ -79,7 +79,7 @@ _nvme_pcie_event_process(struct spdk_pci_event *event, void *cb_ctx) 
			return;

		}



		ctrlr = nvme_get_ctrlr_by_trid_unsafe(&trid);

		ctrlr = nvme_get_ctrlr_by_trid_unsafe(&trid, NULL);

		if (ctrlr == NULL) {

			return;

		}
@@ -833,7 +833,7 @@ pcie_nvme_enum_cb(void *ctx, struct spdk_pci_device *pci_dev) 
	spdk_nvme_trid_populate_transport(&trid, SPDK_NVME_TRANSPORT_PCIE);

	spdk_pci_addr_fmt(trid.traddr, sizeof(trid.traddr), &pci_addr);



	ctrlr = nvme_get_ctrlr_by_trid_unsafe(&trid);

	ctrlr = nvme_get_ctrlr_by_trid_unsafe(&trid, NULL);

	if (!spdk_process_is_primary()) {

		if (!ctrlr) {

			SPDK_ERRLOG("Controller must be constructed in the primary process first.\n");

test/unit/lib/nvme/nvme.c/nvme_ut.c

+16 −12
Original line number Diff line number Diff line
@@ -81,6 +81,7 @@ void 
spdk_nvme_ctrlr_get_default_ctrlr_opts(struct spdk_nvme_ctrlr_opts *opts, size_t opts_size)

{

	memset(opts, 0, opts_size);

	nvme_get_default_hostnqn(opts->hostnqn, sizeof(opts->hostnqn));

	opts->opts_size = opts_size;

}
@@ -147,7 +148,7 @@ nvme_transport_ctrlr_scan(struct spdk_nvme_probe_ctx *probe_ctx, 


	if (direct_connect == true && probe_ctx->probe_cb) {

		nvme_robust_mutex_unlock(&g_spdk_nvme_driver->lock);

		ctrlr = nvme_get_ctrlr_by_trid(&probe_ctx->trid);

		ctrlr = nvme_get_ctrlr_by_trid(&probe_ctx->trid, NULL);

		nvme_robust_mutex_lock(&g_spdk_nvme_driver->lock);

		probe_ctx->probe_cb(probe_ctx->cb_ctx, &probe_ctx->trid, &ctrlr->opts);

	}
@@ -173,7 +174,7 @@ test_spdk_nvme_probe(void) 
	spdk_nvme_remove_cb remove_cb = NULL;

	struct spdk_nvme_ctrlr ctrlr;

	pthread_mutexattr_t attr;

	struct nvme_driver dummy;

	struct nvme_driver dummy = {};

	g_spdk_nvme_driver = &dummy;



	/* driver init fails */
@@ -233,7 +234,7 @@ test_spdk_nvme_connect(void) 
	struct spdk_nvme_ctrlr_opts opts = {};

	struct spdk_nvme_ctrlr ctrlr;

	pthread_mutexattr_t attr;

	struct nvme_driver dummy;

	struct nvme_driver dummy = {};



	/* initialize the variable to prepare the test */

	dummy.initialized = true;
@@ -257,6 +258,7 @@ test_spdk_nvme_connect(void) 


	/* driver init passes, setup one ctrlr on the attached_list */

	memset(&ctrlr, 0, sizeof(struct spdk_nvme_ctrlr));

	spdk_nvme_ctrlr_get_default_ctrlr_opts(&ctrlr.opts, sizeof(ctrlr.opts));

	snprintf(ctrlr.trid.traddr, sizeof(ctrlr.trid.traddr), "0000:01:00.0");

	ctrlr.trid.trtype = SPDK_NVME_TRANSPORT_PCIE;

	TAILQ_INSERT_TAIL(&g_spdk_nvme_driver->shared_attached_ctrlrs, &ctrlr, tailq);
@@ -270,6 +272,7 @@ test_spdk_nvme_connect(void) 
	CU_ASSERT(ret_ctrlr == &ctrlr);

	CU_ASSERT_EQUAL(ret_ctrlr->opts.num_io_queues, DEFAULT_MAX_IO_QUEUES);

	/* get the ctrlr from the attached list with default ctrlr opts and consistent opts_size */

	spdk_nvme_ctrlr_get_default_ctrlr_opts(&opts, sizeof(opts));

	opts.num_io_queues = 1;

	ret_ctrlr = spdk_nvme_connect(&trid, &opts, sizeof(opts));

	CU_ASSERT(ret_ctrlr == &ctrlr);
@@ -297,6 +300,7 @@ test_spdk_nvme_connect(void) 
	memset(&ctrlr, 0, sizeof(struct spdk_nvme_ctrlr));

	snprintf(ctrlr.trid.traddr, sizeof(ctrlr.trid.traddr), "0000:02:00.0");

	ctrlr.trid.trtype = SPDK_NVME_TRANSPORT_PCIE;

	spdk_nvme_ctrlr_get_default_ctrlr_opts(&ctrlr.opts, sizeof(ctrlr.opts));

	TAILQ_INSERT_TAIL(&g_spdk_nvme_driver->shared_attached_ctrlrs, &ctrlr, tailq);

	/* get the ctrlr from the attached list */

	snprintf(trid.traddr, sizeof(trid.traddr), "0000:02:00.0");
@@ -339,7 +343,7 @@ static void 
test_nvme_init_controllers(void)

{

	int rc = 0;

	struct nvme_driver test_driver;

	struct nvme_driver test_driver = {};

	void *cb_ctx = NULL;

	spdk_nvme_attach_cb attach_cb = dummy_attach_cb;

	struct spdk_nvme_probe_ctx *probe_ctx;
@@ -423,7 +427,7 @@ static void 
test_nvme_driver_init(void)

{

	int rc;

	struct nvme_driver dummy;

	struct nvme_driver dummy = {};

	g_spdk_nvme_driver = &dummy;



	/* adjust this so testing doesn't take so long */
@@ -501,7 +505,7 @@ test_spdk_nvme_detach(void) 
	int rc = 1;

	struct spdk_nvme_ctrlr ctrlr;

	struct spdk_nvme_ctrlr *ret_ctrlr;

	struct nvme_driver test_driver;

	struct nvme_driver test_driver = {};



	memset(&ctrlr, 0, sizeof(ctrlr));

	ctrlr.trid.trtype = SPDK_NVME_TRANSPORT_PCIE;
@@ -835,21 +839,21 @@ test_nvme_ctrlr_probe(void) 
	/* test when probe_cb returns false */



	MOCK_SET(dummy_probe_cb, false);

	nvme_probe_ctx_init(&probe_ctx, &trid, cb_ctx, dummy_probe_cb, NULL, NULL);

	nvme_probe_ctx_init(&probe_ctx, &trid, NULL, cb_ctx, dummy_probe_cb, NULL, NULL);

	rc = nvme_ctrlr_probe(&trid, &probe_ctx, devhandle);

	CU_ASSERT(rc == 1);



	/* probe_cb returns true but we can't construct a ctrl */

	MOCK_SET(dummy_probe_cb, true);

	MOCK_SET(nvme_transport_ctrlr_construct, NULL);

	nvme_probe_ctx_init(&probe_ctx, &trid, cb_ctx, dummy_probe_cb, NULL, NULL);

	nvme_probe_ctx_init(&probe_ctx, &trid, NULL, cb_ctx, dummy_probe_cb, NULL, NULL);

	rc = nvme_ctrlr_probe(&trid, &probe_ctx, devhandle);

	CU_ASSERT(rc == -1);



	/* happy path */

	MOCK_SET(dummy_probe_cb, true);

	MOCK_SET(nvme_transport_ctrlr_construct, &ctrlr);

	nvme_probe_ctx_init(&probe_ctx, &trid, cb_ctx, dummy_probe_cb, NULL, NULL);

	nvme_probe_ctx_init(&probe_ctx, &trid, NULL, cb_ctx, dummy_probe_cb, NULL, NULL);

	rc = nvme_ctrlr_probe(&trid, &probe_ctx, devhandle);

	CU_ASSERT(rc == 0);

	dummy = TAILQ_FIRST(&probe_ctx.init_ctrlrs);
@@ -1407,7 +1411,7 @@ test_nvme_ctrlr_probe_internal(void) 
{

	struct spdk_nvme_probe_ctx *probe_ctx;

	struct spdk_nvme_transport_id trid = {};

	struct nvme_driver dummy;

	struct nvme_driver dummy = {};

	int rc;



	probe_ctx = calloc(1, sizeof(*probe_ctx));
@@ -1422,7 +1426,7 @@ test_nvme_ctrlr_probe_internal(void) 
	ut_test_probe_internal = true;

	MOCK_SET(dummy_probe_cb, true);

	trid.trtype = SPDK_NVME_TRANSPORT_PCIE;

	nvme_probe_ctx_init(probe_ctx, &trid, NULL, dummy_probe_cb, NULL, NULL);

	nvme_probe_ctx_init(probe_ctx, &trid, NULL, NULL, dummy_probe_cb, NULL, NULL);

	rc = nvme_probe_internal(probe_ctx, false);

	CU_ASSERT(rc < 0);

	CU_ASSERT(TAILQ_EMPTY(&probe_ctx->init_ctrlrs));
@@ -1495,7 +1499,7 @@ test_spdk_nvme_detach_async(void) 
{

	int rc = 1;

	struct spdk_nvme_ctrlr ctrlr1, ctrlr2;

	struct nvme_driver test_driver;

	struct nvme_driver test_driver = {};

	struct spdk_nvme_detach_ctx *detach_ctx;

	struct nvme_ctrlr_detach_ctx *ctx;