Commit ec9e6728 authored by Shuhei Matsumoto's avatar Shuhei Matsumoto Committed by Tomasz Zawadzki
Browse files

bdev/nvme: Fix the case that namespace was removed during reset 



Some NVMe-oF targets add listener and then add namespace to a subsystem.
If such NVMe-oF targets reset, the subsystem can be empty temporarily
when its connection is re-established. Then, namespace can be exposed
again later.

The NVMe-oF initiator of the NVMe bdev module could not handle reset
correctly for such targets because nvme_ns->ns was not changed and
it was not checked if nvme_ns->ns is non-NULL.

This patch adds the following changes to fix the bug.

After adminq is reconnected, check if ns exists or not. If ns does not
exist, clear nvme_ns->ns to NULL.

Async event callback fills nvme_ns->ns if ns is active and nvme_ns->ns
is different from ns.

Add check if nvme_ns->ns is not NULL to all corresponding functions.

Add unit test for verification.

Fixes github issue #3313

Signed-off-by: default avatarShuhei Matsumoto <smatsumoto@nvidia.com>
Change-Id: Iad3c2aace56a21191a7d772981bec8b4e2047af7
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/22485


Reviewed-by: default avatarBen Walker <ben@nvidia.com>
Community-CI: Mellanox Build Bot
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarJim Harris <jim.harris@samsung.com>
parent 3544958e

module/bdev/nvme/bdev_nvme.c

+42 −2
Original line number Diff line number Diff line
@@ -833,6 +833,10 @@ nvme_ns_is_active(struct nvme_ns *nvme_ns) 
		return false;

	}



	if (spdk_unlikely(nvme_ns->ns == NULL)) {

		return false;

	}



	return true;

}
@@ -2220,6 +2224,24 @@ bdev_nvme_reset_create_qpair(struct spdk_io_channel_iter *i) 
	}

}



static void

nvme_ctrlr_check_namespaces(struct nvme_ctrlr *nvme_ctrlr)

{

	struct spdk_nvme_ctrlr *ctrlr = nvme_ctrlr->ctrlr;

	struct nvme_ns *nvme_ns;



	for (nvme_ns = nvme_ctrlr_get_first_active_ns(nvme_ctrlr);

	     nvme_ns != NULL;

	     nvme_ns = nvme_ctrlr_get_next_active_ns(nvme_ctrlr, nvme_ns)) {

		if (!spdk_nvme_ctrlr_is_active_ns(ctrlr, nvme_ns->id)) {

			SPDK_DEBUGLOG(bdev_nvme, "NSID %u was removed during reset.\n", nvme_ns->id);

			/* NS can be added again. Just nullify nvme_ns->ns. */

			nvme_ns->ns = NULL;

		}

	}

}





static int

bdev_nvme_reconnect_ctrlr_poll(void *arg)

{
@@ -2235,6 +2257,8 @@ bdev_nvme_reconnect_ctrlr_poll(void *arg) 


	spdk_poller_unregister(&nvme_ctrlr->reset_detach_poller);

	if (rc == 0) {

		nvme_ctrlr_check_namespaces(nvme_ctrlr);



		/* Recreate all of the I/O queue pairs */

		spdk_for_each_channel(nvme_ctrlr,

				      bdev_nvme_reset_create_qpair,
@@ -3132,6 +3156,10 @@ bdev_nvme_io_type_supported(void *ctx, enum spdk_bdev_io_type io_type) 
	nvme_ns = TAILQ_FIRST(&nbdev->nvme_ns_list);

	assert(nvme_ns != NULL);

	ns = nvme_ns->ns;

	if (ns == NULL) {

		return false;

	}



	ctrlr = spdk_nvme_ns_get_ctrlr(ns);



	switch (io_type) {
@@ -3599,6 +3627,10 @@ nvme_namespace_info_json(struct spdk_json_write_ctx *w, 
	char buf[128];



	ns = nvme_ns->ns;

	if (ns == NULL) {

		return;

	}



	ctrlr = spdk_nvme_ns_get_ctrlr(ns);



	cdata = spdk_nvme_ctrlr_get_data(ctrlr);
@@ -3967,6 +3999,8 @@ nvme_ns_set_ana_state(const struct spdk_nvme_ana_group_descriptor *desc, void *c 
	struct nvme_ns *nvme_ns = cb_arg;

	uint32_t i;



	assert(nvme_ns->ns != NULL);



	for (i = 0; i < desc->num_of_nsid; i++) {

		if (desc->nsid[i] != spdk_nvme_ns_get_id(nvme_ns->ns)) {

			continue;
@@ -4481,7 +4515,7 @@ nvme_bdev_add_ns(struct nvme_bdev *bdev, struct nvme_ns *nvme_ns) 
	tmp_ns = TAILQ_FIRST(&bdev->nvme_ns_list);

	assert(tmp_ns != NULL);



	if (!bdev_nvme_compare_ns(nvme_ns->ns, tmp_ns->ns)) {

	if (tmp_ns->ns != NULL && !bdev_nvme_compare_ns(nvme_ns->ns, tmp_ns->ns)) {

		pthread_mutex_unlock(&bdev->mutex);

		SPDK_ERRLOG("Namespaces are not identical.\n");

		return -EINVAL;
@@ -4633,8 +4667,14 @@ nvme_ctrlr_populate_namespaces(struct nvme_ctrlr *nvme_ctrlr, 
		next = nvme_ctrlr_get_next_active_ns(nvme_ctrlr, nvme_ns);



		if (spdk_nvme_ctrlr_is_active_ns(ctrlr, nvme_ns->id)) {

			/* NS is still there but attributes may have changed */

			/* NS is still there or added again. Its attributes may have changed. */

			ns = spdk_nvme_ctrlr_get_ns(ctrlr, nvme_ns->id);

			if (nvme_ns->ns != ns) {

				assert(nvme_ns->ns == NULL);

				nvme_ns->ns = ns;

				SPDK_DEBUGLOG(bdev_nvme, "NSID %u was added\n", nvme_ns->id);

			}



			num_sectors = spdk_nvme_ns_get_num_sectors(ns);

			bdev = nvme_ns->bdev;

			assert(bdev != NULL);

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

+100 −3
Original line number Diff line number Diff line
@@ -4265,11 +4265,12 @@ test_find_io_path(void) 
	};

	struct spdk_nvme_qpair qpair1 = {}, qpair2 = {};

	struct spdk_nvme_ctrlr ctrlr1 = {}, ctrlr2 = {};

	struct spdk_nvme_ns ns1 = {}, ns2 = {};

	struct nvme_ctrlr nvme_ctrlr1 = { .ctrlr = &ctrlr1, }, nvme_ctrlr2 = { .ctrlr = &ctrlr2, };

	struct nvme_ctrlr_channel ctrlr_ch1 = {}, ctrlr_ch2 = {};

	struct nvme_qpair nvme_qpair1 = { .ctrlr_ch = &ctrlr_ch1, .ctrlr = &nvme_ctrlr1, };

	struct nvme_qpair nvme_qpair2 = { .ctrlr_ch = &ctrlr_ch2, .ctrlr = &nvme_ctrlr2, };

	struct nvme_ns nvme_ns1 = {}, nvme_ns2 = {};

	struct nvme_ns nvme_ns1 = { .ns = &ns1, }, nvme_ns2 = { .ns = &ns2, };

	struct nvme_io_path io_path1 = { .qpair = &nvme_qpair1, .nvme_ns = &nvme_ns1, };

	struct nvme_io_path io_path2 = { .qpair = &nvme_qpair2, .nvme_ns = &nvme_ns2, };
@@ -5981,6 +5982,7 @@ test_find_next_io_path(void) 
	};

	struct spdk_nvme_qpair qpair1 = {}, qpair2 = {}, qpair3 = {};

	struct spdk_nvme_ctrlr ctrlr1 = {}, ctrlr2 = {}, ctrlr3 = {};

	struct spdk_nvme_ns ns1 = {}, ns2 = {}, ns3 = {};

	struct nvme_ctrlr nvme_ctrlr1 = { .ctrlr = &ctrlr1, };

	struct nvme_ctrlr nvme_ctrlr2 = { .ctrlr = &ctrlr2, };

	struct nvme_ctrlr nvme_ctrlr3 = { .ctrlr = &ctrlr3, };
@@ -5990,7 +5992,7 @@ test_find_next_io_path(void) 
	struct nvme_qpair nvme_qpair1 = { .ctrlr_ch = &ctrlr_ch1, .ctrlr = &nvme_ctrlr1, .qpair = &qpair1, };

	struct nvme_qpair nvme_qpair2 = { .ctrlr_ch = &ctrlr_ch2, .ctrlr = &nvme_ctrlr2, .qpair = &qpair2, };

	struct nvme_qpair nvme_qpair3 = { .ctrlr_ch = &ctrlr_ch3, .ctrlr = &nvme_ctrlr3, .qpair = &qpair3, };

	struct nvme_ns nvme_ns1 = {}, nvme_ns2 = {}, nvme_ns3 = {};

	struct nvme_ns nvme_ns1 = { .ns = &ns1, }, nvme_ns2 = { .ns = &ns2, }, nvme_ns3 = { .ns = &ns3, };

	struct nvme_io_path io_path1 = { .qpair = &nvme_qpair1, .nvme_ns = &nvme_ns1, };

	struct nvme_io_path io_path2 = { .qpair = &nvme_qpair2, .nvme_ns = &nvme_ns2, };

	struct nvme_io_path io_path3 = { .qpair = &nvme_qpair3, .nvme_ns = &nvme_ns3, };
@@ -6052,6 +6054,7 @@ test_find_io_path_min_qd(void) 
	};

	struct spdk_nvme_qpair qpair1 = {}, qpair2 = {}, qpair3 = {};

	struct spdk_nvme_ctrlr ctrlr1 = {}, ctrlr2 = {}, ctrlr3 = {};

	struct spdk_nvme_ns ns1 = {}, ns2 = {}, ns3 = {};

	struct nvme_ctrlr nvme_ctrlr1 = { .ctrlr = &ctrlr1, };

	struct nvme_ctrlr nvme_ctrlr2 = { .ctrlr = &ctrlr2, };

	struct nvme_ctrlr nvme_ctrlr3 = { .ctrlr = &ctrlr3, };
@@ -6061,7 +6064,7 @@ test_find_io_path_min_qd(void) 
	struct nvme_qpair nvme_qpair1 = { .ctrlr_ch = &ctrlr_ch1, .ctrlr = &nvme_ctrlr1, .qpair = &qpair1, };

	struct nvme_qpair nvme_qpair2 = { .ctrlr_ch = &ctrlr_ch2, .ctrlr = &nvme_ctrlr2, .qpair = &qpair2, };

	struct nvme_qpair nvme_qpair3 = { .ctrlr_ch = &ctrlr_ch3, .ctrlr = &nvme_ctrlr3, .qpair = &qpair3, };

	struct nvme_ns nvme_ns1 = {}, nvme_ns2 = {}, nvme_ns3 = {};

	struct nvme_ns nvme_ns1 = { .ns = &ns1, }, nvme_ns2 = { .ns = &ns2, }, nvme_ns3 = { .ns = &ns3, };

	struct nvme_io_path io_path1 = { .qpair = &nvme_qpair1, .nvme_ns = &nvme_ns1, };

	struct nvme_io_path io_path2 = { .qpair = &nvme_qpair2, .nvme_ns = &nvme_ns2, };

	struct nvme_io_path io_path3 = { .qpair = &nvme_qpair3, .nvme_ns = &nvme_ns3, };
@@ -7267,6 +7270,99 @@ test_delete_ctrlr_done(void) 
	CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL);

}



static void

test_ns_remove_during_reset(void)

{

	struct nvme_path_id path = {};

	struct nvme_ctrlr_opts opts = {};

	struct spdk_nvme_ctrlr *ctrlr;

	struct nvme_bdev_ctrlr *nbdev_ctrlr;

	struct nvme_ctrlr *nvme_ctrlr;

	const int STRING_SIZE = 32;

	const char *attached_names[STRING_SIZE];

	struct nvme_bdev *bdev;

	struct nvme_ns *nvme_ns;

	union spdk_nvme_async_event_completion event = {};

	struct spdk_nvme_cpl cpl = {};

	int rc;



	memset(attached_names, 0, sizeof(char *) * STRING_SIZE);

	ut_init_trid(&path.trid);



	set_thread(0);



	ctrlr = ut_attach_ctrlr(&path.trid, 1, false, false);

	SPDK_CU_ASSERT_FATAL(ctrlr != NULL);



	g_ut_attach_ctrlr_status = 0;

	g_ut_attach_bdev_count = 1;



	rc = bdev_nvme_create(&path.trid, "nvme0", attached_names, STRING_SIZE,

			      attach_ctrlr_done, NULL, NULL, &opts, false);

	CU_ASSERT(rc == 0);



	spdk_delay_us(1000);

	poll_threads();



	nbdev_ctrlr = nvme_bdev_ctrlr_get_by_name("nvme0");

	SPDK_CU_ASSERT_FATAL(nbdev_ctrlr != NULL);



	nvme_ctrlr = nvme_bdev_ctrlr_get_ctrlr(nbdev_ctrlr, &path.trid);

	CU_ASSERT(nvme_ctrlr != NULL);



	bdev = nvme_bdev_ctrlr_get_bdev(nbdev_ctrlr, 1);

	CU_ASSERT(bdev != NULL);



	nvme_ns = nvme_ctrlr_get_first_active_ns(nvme_ctrlr);

	CU_ASSERT(nvme_ns != NULL);



	/* If ns is removed during ctrlr reset, nvme_ns and bdev should still exist,

	 * but nvme_ns->ns should be NULL.

	 */



	CU_ASSERT(ctrlr->ns[0].is_active == true);

	ctrlr->ns[0].is_active = false;



	rc = bdev_nvme_reset_ctrlr(nvme_ctrlr);

	CU_ASSERT(rc == 0);



	poll_threads();

	spdk_delay_us(g_opts.nvme_adminq_poll_period_us);

	poll_threads();



	CU_ASSERT(nvme_ctrlr->resetting == false);

	CU_ASSERT(ctrlr->adminq.is_connected == true);



	CU_ASSERT(nvme_ns == nvme_ctrlr_get_first_active_ns(nvme_ctrlr));

	CU_ASSERT(bdev == nvme_bdev_ctrlr_get_bdev(nbdev_ctrlr, 1));

	CU_ASSERT(nvme_ns->bdev == bdev);

	CU_ASSERT(nvme_ns->ns == NULL);



	/* Then, async event should fill nvme_ns->ns again. */



	ctrlr->ns[0].is_active = true;



	event.bits.async_event_type = SPDK_NVME_ASYNC_EVENT_TYPE_NOTICE;

	event.bits.async_event_info = SPDK_NVME_ASYNC_EVENT_NS_ATTR_CHANGED;

	cpl.cdw0 = event.raw;



	aer_cb(nvme_ctrlr, &cpl);



	CU_ASSERT(nvme_ns == nvme_ctrlr_get_first_active_ns(nvme_ctrlr));

	CU_ASSERT(bdev == nvme_bdev_ctrlr_get_bdev(nbdev_ctrlr, 1));

	CU_ASSERT(nvme_ns->bdev == bdev);

	CU_ASSERT(nvme_ns->ns == &ctrlr->ns[0]);



	rc = bdev_nvme_delete("nvme0", &g_any_path, NULL, NULL);

	CU_ASSERT(rc == 0);



	poll_threads();

	spdk_delay_us(1000);

	poll_threads();



	CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL);

}



int

main(int argc, char **argv)

{
@@ -7324,6 +7420,7 @@ main(int argc, char **argv) 
	CU_ADD_TEST(suite, test_bdev_ctrlr_op_rpc);

	CU_ADD_TEST(suite, test_disable_enable_ctrlr);

	CU_ADD_TEST(suite, test_delete_ctrlr_done);

	CU_ADD_TEST(suite, test_ns_remove_during_reset);



	allocate_threads(3);

	set_thread(0);