Commit d7b4e337 authored by Joel Cunningham's avatar Joel Cunningham Committed by Tomasz Zawadzki
Browse files

lib/nvmf: move poll group subsystem ns resv update to subsystem.c 



Move the poll group subsystem update of ns reservation state to an
interneal function in subsystem.c.

This better locates the logic with the rest of the ns reservation logic
and makes unit testing the poll group update code possible in
subsystem.c without duplicating the logic into the unit test.

This makes two minor changes to the logic:
  1) memset holder_id if there is no reservation as a safety mechanism
     in case some code reads it instead of rtype/crkey first
  2) Use TAILQ_FOREACH. The _SAFE verion is not needed since we are not
     removing from the list.

Update reservation request unit tests in subsystem_ut.c that fully
exercise the reservation request handling to validate the pg ns updates.

Change-Id: I04136d637850dc81b5f6d9eadcbedbaae290e4ab
Signed-off-by: default avatarJoel Cunningham <joel.cunningham@oracle.com>
Reviewed-on: https://review.spdk.io/c/spdk/spdk/+/26469


Tested-by: default avatarSPDK Automated Test System <spdkbot@gmail.com>
Reviewed-by: default avatarJim Harris <jim.harris@nvidia.com>
Community-CI: Mellanox Build Bot
Reviewed-by: default avatarChangpeng Liu <changpeliu@tencent.com>
parent 84daa90c

lib/nvmf/nvmf.c

+2 −17
Original line number Diff line number Diff line
@@ -1464,9 +1464,8 @@ poll_group_update_subsystem(struct spdk_nvmf_poll_group *group, 
			    struct spdk_nvmf_subsystem *subsystem)

{

	struct spdk_nvmf_subsystem_poll_group *sgroup;

	uint32_t i, j;

	uint32_t i;

	struct spdk_nvmf_ns *ns;

	struct spdk_nvmf_registrant *reg, *tmp;

	struct spdk_io_channel *ch;

	struct spdk_nvmf_subsystem_pg_ns_info *ns_info;

	struct spdk_nvmf_ctrlr *ctrlr;
@@ -1553,21 +1552,7 @@ poll_group_update_subsystem(struct spdk_nvmf_poll_group *group, 
			ns_info->uuid = *spdk_bdev_get_uuid(ns->bdev);

			ns_info->num_blocks = spdk_bdev_get_num_blocks(ns->bdev);

			ns_info->anagrpid = ns->anagrpid;

			ns_info->crkey = ns->crkey;

			ns_info->rtype = ns->rtype;

			if (ns->holder) {

				ns_info->holder_id = ns->holder->hostid;

			}



			memset(&ns_info->reg_hostid, 0, SPDK_NVMF_MAX_NUM_REGISTRANTS * sizeof(struct spdk_uuid));

			j = 0;

			TAILQ_FOREACH_SAFE(reg, &ns->registrants, link, tmp) {

				if (j >= SPDK_NVMF_MAX_NUM_REGISTRANTS) {

					SPDK_ERRLOG("Maximum %u registrants can support.\n", SPDK_NVMF_MAX_NUM_REGISTRANTS);

					return -EINVAL;

				}

				ns_info->reg_hostid[j++] = reg->hostid;

			}

			nvmf_subsystem_poll_group_update_ns_reservation(ns, ns_info);

		}

	}

lib/nvmf/nvmf_internal.h

+2 −0
Original line number Diff line number Diff line
@@ -448,6 +448,8 @@ struct spdk_nvmf_subsystem_listener *nvmf_subsystem_find_listener( 
	struct spdk_nvmf_subsystem *subsystem,

	const struct spdk_nvme_transport_id *trid);

bool nvmf_subsystem_zone_append_supported(struct spdk_nvmf_subsystem *subsystem);

int nvmf_subsystem_poll_group_update_ns_reservation(const struct spdk_nvmf_ns *ns,

		struct spdk_nvmf_subsystem_pg_ns_info *pg_ns);

struct spdk_nvmf_listener *nvmf_transport_find_listener(

	struct spdk_nvmf_transport *transport,

	const struct spdk_nvme_transport_id *trid);

lib/nvmf/subsystem.c

+27 −0
Original line number Diff line number Diff line
@@ -1681,6 +1681,33 @@ spdk_nvmf_subsystem_any_listener_allowed(struct spdk_nvmf_subsystem *subsystem) 
	return subsystem->flags.allow_any_listener;

}



int

nvmf_subsystem_poll_group_update_ns_reservation(const struct spdk_nvmf_ns *ns,

		struct spdk_nvmf_subsystem_pg_ns_info *pg_ns)

{

	uint32_t j;

	struct spdk_nvmf_registrant *reg;



	pg_ns->crkey = ns->crkey;

	pg_ns->rtype = ns->rtype;

	if (ns->holder) {

		pg_ns->holder_id = ns->holder->hostid;

	} else {

		memset(&pg_ns->holder_id, 0, sizeof(pg_ns->holder_id));

	}



	memset(&pg_ns->reg_hostid, 0, SPDK_NVMF_MAX_NUM_REGISTRANTS * sizeof(struct spdk_uuid));

	j = 0;

	TAILQ_FOREACH(reg, &ns->registrants, link) {

		if (j >= SPDK_NVMF_MAX_NUM_REGISTRANTS) {

			SPDK_ERRLOG("Maximum %u registrants can support.\n", SPDK_NVMF_MAX_NUM_REGISTRANTS);

			return -EINVAL;

		}

		pg_ns->reg_hostid[j++] = reg->hostid;

	}

	return 0;

}



struct subsystem_update_ns_ctx {

	struct spdk_nvmf_subsystem *subsystem;

test/unit/lib/nvmf/nvmf.c/nvmf_ut.c

+2 −4
Original line number Diff line number Diff line
@@ -30,6 +30,8 @@ DEFINE_STUB(nvmf_transport_req_free, int, (struct spdk_nvmf_request *req), 0); 
DEFINE_STUB(nvmf_transport_poll_group_poll, int, (struct spdk_nvmf_transport_poll_group *group), 0);

DEFINE_STUB_V(nvmf_subsystem_remove_all_listeners, (struct spdk_nvmf_subsystem *subsystem,

		bool stop));

DEFINE_STUB(nvmf_subsystem_poll_group_update_ns_reservation, int, (const struct spdk_nvmf_ns *ns,

		struct spdk_nvmf_subsystem_pg_ns_info *pg_ns), 0);

DEFINE_STUB(spdk_nvmf_subsystem_destroy, int, (struct spdk_nvmf_subsystem *subsystem,

		nvmf_subsystem_destroy_cb cpl_cb, void *cpl_cb_arg), 0);

DEFINE_STUB(spdk_nvmf_subsystem_get_first_listener, struct spdk_nvmf_subsystem_listener *,
@@ -179,8 +181,6 @@ test_nvmf_tgt_create_poll_group(void) 
	MOCK_SET(spdk_nvmf_subsystem_get_first, &subsystem);



	subsystem.ns[0] = &ns;

	ns.crkey = 0xaa;

	ns.rtype = 0xbb;

	TAILQ_INIT(&ns.registrants);

	ns.bdev = &bdev;

	spdk_uuid_generate(&bdev.uuid);
@@ -203,8 +203,6 @@ test_nvmf_tgt_create_poll_group(void) 
	CU_ASSERT(group.sgroups[0].ns_info[0].channel == &ch);

	CU_ASSERT(!memcmp(&group.sgroups[0].ns_info[0].uuid, &bdev.uuid, 16));

	CU_ASSERT(group.sgroups[0].ns_info[0].num_blocks == 512);

	CU_ASSERT(group.sgroups[0].ns_info[0].crkey == 0xaa);

	CU_ASSERT(group.sgroups[0].ns_info[0].rtype == 0xbb);

	CU_ASSERT(TAILQ_FIRST(&tgt.poll_groups) == &group);

	CU_ASSERT(tgt.num_poll_groups == 1);

	CU_ASSERT(group.thread == thread);

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

+77 −8
Original line number Diff line number Diff line
@@ -101,7 +101,21 @@ int 
nvmf_poll_group_update_subsystem(struct spdk_nvmf_poll_group *group,

				 struct spdk_nvmf_subsystem *subsystem)

{

	struct spdk_nvmf_subsystem_poll_group *sgroup;

	struct spdk_nvmf_subsystem_pg_ns_info *pg_ns;

	struct spdk_nvmf_ns *ns;



	g_ns_pg_update++;

	/* If test setup pg subsystem, update it */

	if (group->sgroups) {

		SPDK_CU_ASSERT_FATAL(group->num_sgroups);

		sgroup = &group->sgroups[subsystem->id];

		SPDK_CU_ASSERT_FATAL(subsystem->max_nsid >= 1);

		SPDK_CU_ASSERT_FATAL(subsystem->ns[0]);

		ns = subsystem->ns[0];

		pg_ns = &sgroup->ns_info[ns->nsid - 1];

		return nvmf_subsystem_poll_group_update_ns_reservation(ns, pg_ns);

	}

	return 0;

}
@@ -1649,6 +1663,33 @@ nvmf_tgt_destroy_poll_group(void *io_device, void *ctx_buf) 
{

}



static int

nvmf_tgt_reservation_create_poll_group(void *io_device, void *ctx_buf)

{

	struct spdk_nvmf_poll_group *pg = ctx_buf;

	struct spdk_nvmf_subsystem_poll_group *sgroup;

	pg->thread = spdk_get_thread();

	pg->num_sgroups = 1;

	pg->sgroups = calloc(pg->num_sgroups, sizeof(struct spdk_nvmf_subsystem_poll_group));

	SPDK_CU_ASSERT_FATAL(pg->sgroups);

	SPDK_CU_ASSERT_FATAL(g_subsystem.id < pg->num_sgroups);

	sgroup = &pg->sgroups[g_subsystem.id];

	sgroup->ns_info = calloc(g_subsystem.max_nsid, sizeof(struct spdk_nvmf_subsystem_pg_ns_info));

	SPDK_CU_ASSERT_FATAL(sgroup->ns_info);

	sgroup->num_ns = g_subsystem.max_nsid;

	return 0;

}



static void

nvmf_tgt_reservation_destroy_poll_group(void *io_device, void *ctx_buf)

{

	struct spdk_nvmf_poll_group *pg = ctx_buf;

	struct spdk_nvmf_subsystem_poll_group *sgroup;

	sgroup = &pg->sgroups[g_subsystem.id];

	free(sgroup->ns_info);

	free(pg->sgroups);

}



static size_t

registrants_get_count(const struct spdk_nvmf_ns *ns)

{
@@ -1685,6 +1726,8 @@ test_reservation_request(void) 
	struct spdk_nvmf_qpair qpair = {};

	struct spdk_nvmf_tgt tgt = {};

	struct spdk_nvmf_poll_group *pg;

	struct spdk_nvmf_subsystem_poll_group *sgroup;

	struct spdk_nvmf_subsystem_pg_ns_info *pg_ns;

	struct spdk_io_channel *ch;

	struct spdk_thread *thread;

	const uint64_t rkey = 0xa1;
@@ -1698,17 +1741,18 @@ test_reservation_request(void) 
	g_subsystem.tgt = &tgt;



	spdk_io_device_register(&tgt,

				nvmf_tgt_create_poll_group,

				nvmf_tgt_destroy_poll_group,

				nvmf_tgt_reservation_create_poll_group,

				nvmf_tgt_reservation_destroy_poll_group,

				sizeof(struct spdk_nvmf_poll_group),

				NULL);



	/* Create a pg */

	/* Get the poll group */

	ch = spdk_get_io_channel(&tgt);

	SPDK_CU_ASSERT_FATAL(ch);

	pg = spdk_io_channel_get_ctx(ch);

	pg->thread = thread;

	qpair.group = pg;

	sgroup = &pg->sgroups[g_subsystem.id];

	pg_ns = &sgroup->ns_info[g_ns.nsid - 1];



	req = ut_reservation_build_req(16);

	SPDK_CU_ASSERT_FATAL(req != NULL);
@@ -1731,6 +1775,12 @@ test_reservation_request(void) 
	poll_threads(); /* drive poll group update */

	SPDK_CU_ASSERT_FATAL(g_ns_pg_update == 1);

	SPDK_CU_ASSERT_FATAL(reservations_get_count(&g_ns) == 0);

	/* Validate poll group ns state */

	SPDK_CU_ASSERT_FATAL(pg_ns->crkey == 0);

	SPDK_CU_ASSERT_FATAL(pg_ns->rtype == 0);

	SPDK_CU_ASSERT_FATAL(spdk_uuid_is_null(&pg_ns->holder_id));

	SPDK_CU_ASSERT_FATAL(spdk_uuid_compare(&pg_ns->reg_hostid[0],

					       &g_ctrlr1_A.hostid) == 0);



	/* Acquire */

	ut_reservation_build_acquire_request(req, SPDK_NVME_RESERVE_ACQUIRE, 0,
@@ -1747,6 +1797,11 @@ test_reservation_request(void) 
	poll_threads(); /* drive poll group update */

	SPDK_CU_ASSERT_FATAL(g_ns_pg_update == 2);

	SPDK_CU_ASSERT_FATAL(reservations_get_count(&g_ns) == 0);

	/* Validate poll group ns state */

	SPDK_CU_ASSERT_FATAL(pg_ns->crkey == rkey);

	SPDK_CU_ASSERT_FATAL(pg_ns->rtype == SPDK_NVME_RESERVE_WRITE_EXCLUSIVE);

	SPDK_CU_ASSERT_FATAL(spdk_uuid_compare(&pg_ns->holder_id,

					       &g_ctrlr1_A.hostid) == 0);



	/* Report */

	report_req = ut_reservation_build_req(
@@ -1795,6 +1850,12 @@ test_reservation_request(void) 
	poll_threads(); /* drive poll group update */

	SPDK_CU_ASSERT_FATAL(g_ns_pg_update == 3);

	SPDK_CU_ASSERT_FATAL(reservations_get_count(&g_ns) == 0);

	/* Validate poll group ns state */

	SPDK_CU_ASSERT_FATAL(pg_ns->crkey == 0);

	SPDK_CU_ASSERT_FATAL(pg_ns->rtype == 0);

	SPDK_CU_ASSERT_FATAL(spdk_uuid_is_null(&pg_ns->holder_id));

	SPDK_CU_ASSERT_FATAL(spdk_uuid_compare(&pg_ns->reg_hostid[0],

					       &g_ctrlr1_A.hostid) == 0);



	spdk_put_io_channel(ch);

	spdk_io_device_unregister(&tgt, NULL);
@@ -1818,6 +1879,8 @@ test_reservation_request_multi_pending(void) 
	struct spdk_nvme_cpl *rsp;

	struct spdk_nvmf_tgt tgt = {};

	struct spdk_nvmf_poll_group *pg;

	struct spdk_nvmf_subsystem_poll_group *sgroup;

	struct spdk_nvmf_subsystem_pg_ns_info *pg_ns;

	struct spdk_io_channel *ch;

	struct spdk_thread *thread;

	const uint64_t key_base = 0xDEADBEEF;
@@ -1832,16 +1895,17 @@ test_reservation_request_multi_pending(void) 
	g_ns_pg_update = 0;



	spdk_io_device_register(&tgt,

				nvmf_tgt_create_poll_group,

				nvmf_tgt_destroy_poll_group,

				nvmf_tgt_reservation_create_poll_group,

				nvmf_tgt_reservation_destroy_poll_group,

				sizeof(struct spdk_nvmf_poll_group),

				NULL);



	/* Create a pg */

	/* Get the poll group */

	ch = spdk_get_io_channel(&tgt);

	SPDK_CU_ASSERT_FATAL(ch);

	pg = spdk_io_channel_get_ctx(ch);

	pg->thread = thread;

	sgroup = &pg->sgroups[g_subsystem.id];

	pg_ns = &sgroup->ns_info[g_ns.nsid - 1];



	memset(&qpairs[0], 0, sizeof(qpairs));

	/* Build registrations for each controller on the same nsid */
@@ -1884,6 +1948,11 @@ test_reservation_request_multi_pending(void) 
		}

		poll_thread_times(0, 4); /* enough polls to complete a req and process the next */

		SPDK_CU_ASSERT_FATAL(g_ns_pg_update == i + 1);

		SPDK_CU_ASSERT_FATAL(pg_ns->crkey == 0);

		SPDK_CU_ASSERT_FATAL(pg_ns->rtype == 0);

		SPDK_CU_ASSERT_FATAL(spdk_uuid_is_null(&pg_ns->holder_id));

		SPDK_CU_ASSERT_FATAL(spdk_uuid_compare(&pg_ns->reg_hostid[i],

						       &ctrlrs[i]->hostid) == 0);

	}



	/* All requests should be complete */