nvmf: add subsystem ctrlr management functions

This moves the subsystem->ctrlrs list management fully into the
subsystem code, which will help simplify thread safety considerations
once we start adding locks.

Change-Id: Ibc118923f1bd520f1e524cde5d45ccfcc69aee1e
Signed-off-by: Daniel Verkamp <daniel.verkamp@intel.com>
Reviewed-on: https://review.gerrithub.io/376025
Tested-by: SPDK Automated Test System <sys_sgsw@intel.com>

lib/nvmf/ctrlr.c

@@ -80,15 +80,6 @@ spdk_nvmf_ctrlr_create(struct spdk_nvmf_subsystem *subsystem,
 		return NULL;
 	}
 
-	ctrlr->cntlid = spdk_nvmf_subsystem_gen_cntlid(subsystem);
-	if (ctrlr->cntlid == 0xFFFF) {
-		/* Unable to get a cntlid */
-		SPDK_ERRLOG("Reached max simultaneous ctrlrs\n");
-		spdk_nvmf_poll_group_destroy(ctrlr->group);
-		free(ctrlr);
-		return NULL;
-	}
-
 	TAILQ_INIT(&ctrlr->qpairs);
 	ctrlr->kato = connect_cmd->kato;
 	ctrlr->async_event_config.raw = 0;
@@ -130,13 +121,19 @@ spdk_nvmf_ctrlr_create(struct spdk_nvmf_subsystem *subsystem,
 	SPDK_DEBUGLOG(SPDK_TRACE_NVMF, "cc 0x%x\n", ctrlr->vcprop.cc.raw);
 	SPDK_DEBUGLOG(SPDK_TRACE_NVMF, "csts 0x%x\n", ctrlr->vcprop.csts.raw);
 
-	TAILQ_INSERT_TAIL(&subsystem->ctrlrs, ctrlr, link);
+	if (spdk_nvmf_subsystem_add_ctrlr(subsystem, ctrlr)) {
+		SPDK_ERRLOG("Unable to add controller to subsystem\n");
+		spdk_nvmf_poll_group_destroy(ctrlr->group);
+		free(ctrlr);
+		return NULL;
+	}
+
 	return ctrlr;
 }
 
 static void ctrlr_destruct(struct spdk_nvmf_ctrlr *ctrlr)
 {
-	TAILQ_REMOVE(&ctrlr->subsys->ctrlrs, ctrlr, link);
+	spdk_nvmf_subsystem_remove_ctrlr(ctrlr->subsys, ctrlr);
 	spdk_nvmf_poll_group_destroy(ctrlr->group);
 	free(ctrlr);
 }
@@ -226,18 +223,10 @@ spdk_nvmf_ctrlr_connect(struct spdk_nvmf_qpair *qpair,
 			return;
 		}
 	} else {
-		struct spdk_nvmf_ctrlr *tmp;
-
 		qpair->type = QPAIR_TYPE_IOQ;
 		SPDK_DEBUGLOG(SPDK_TRACE_NVMF, "Connect I/O Queue for controller id 0x%x\n", data->cntlid);
 
-		ctrlr = NULL;
+		ctrlr = spdk_nvmf_subsystem_get_ctrlr(subsystem, data->cntlid);
-		TAILQ_FOREACH(tmp, &subsystem->ctrlrs, link) {
-			if (tmp->cntlid == data->cntlid) {
-				ctrlr = tmp;
-				break;
-			}
-		}
 		if (ctrlr == NULL) {
 			SPDK_ERRLOG("Unknown controller ID 0x%x\n", data->cntlid);
 			SPDK_NVMF_INVALID_CONNECT_DATA(rsp, cntlid);

lib/nvmf/nvmf_internal.h

@@ -240,7 +240,12 @@ int spdk_nvmf_bdev_ctrlr_identify_ns(struct spdk_bdev *bdev, struct spdk_nvme_ns
 
 int spdk_nvmf_subsystem_bdev_attach(struct spdk_nvmf_subsystem *subsystem);
 void spdk_nvmf_subsystem_bdev_detach(struct spdk_nvmf_subsystem *subsystem);
-uint16_t spdk_nvmf_subsystem_gen_cntlid(struct spdk_nvmf_subsystem *subsystem);
+int spdk_nvmf_subsystem_add_ctrlr(struct spdk_nvmf_subsystem *subsystem,
+				  struct spdk_nvmf_ctrlr *ctrlr);
+void spdk_nvmf_subsystem_remove_ctrlr(struct spdk_nvmf_subsystem *subsystem,
+				      struct spdk_nvmf_ctrlr *ctrlr);
+struct spdk_nvmf_ctrlr *spdk_nvmf_subsystem_get_ctrlr(struct spdk_nvmf_subsystem *subsystem,
+		uint16_t cntlid);
 
 static inline struct spdk_nvmf_ns *
 _spdk_nvmf_subsystem_get_ns(struct spdk_nvmf_subsystem *subsystem, uint32_t nsid)

lib/nvmf/subsystem.c

@@ -497,19 +497,16 @@ spdk_nvmf_subsystem_get_type(struct spdk_nvmf_subsystem *subsystem)
 	return subsystem->subtype;
 }
 
-uint16_t
+static uint16_t
 spdk_nvmf_subsystem_gen_cntlid(struct spdk_nvmf_subsystem *subsystem)
 {
-	struct spdk_nvmf_ctrlr *ctrlr;
+	int count;
-	uint16_t count;
-	bool in_use = true;
 
 	/*
 	 * In the worst case, we might have to try all CNTLID values between 1 and 0xFFF0 - 1
 	 * before we find one that is unused (or find that all values are in use).
 	 */
-	count = 0xFFF0 - 1;
+	for (count = 0; count < 0xFFF0 - 1; count++) {
-	do {
 		subsystem->next_cntlid++;
 		if (subsystem->next_cntlid >= 0xFFF0) {
 			/* The spec reserves cntlid values in the range FFF0h to FFFFh. */
@@ -517,21 +514,49 @@ spdk_nvmf_subsystem_gen_cntlid(struct spdk_nvmf_subsystem *subsystem)
 		}
 
 		/* Check if a controller with this cntlid currently exists. */
-		in_use = false;
+		if (spdk_nvmf_subsystem_get_ctrlr(subsystem, subsystem->next_cntlid) == NULL) {
-		TAILQ_FOREACH(ctrlr, &subsystem->ctrlrs, link) {
+			/* Found unused cntlid */
-			if (ctrlr->cntlid == subsystem->next_cntlid) {
+			return subsystem->next_cntlid;
-				in_use = true;
-				break;
-			}
 		}
-
-		count--;
-	} while (in_use && count > 0);
-
-	if (count == 0) {
-		/* All valid cntlid values are in use. */
-		return 0xFFFF;
 	}
 
-	return subsystem->next_cntlid;
+	/* All valid cntlid values are in use. */
+	return 0xFFFF;
+}
+
+int
+spdk_nvmf_subsystem_add_ctrlr(struct spdk_nvmf_subsystem *subsystem, struct spdk_nvmf_ctrlr *ctrlr)
+{
+	ctrlr->cntlid = spdk_nvmf_subsystem_gen_cntlid(subsystem);
+	if (ctrlr->cntlid == 0xFFFF) {
+		/* Unable to get a cntlid */
+		SPDK_ERRLOG("Reached max simultaneous ctrlrs\n");
+		return -EBUSY;
+	}
+
+	TAILQ_INSERT_TAIL(&subsystem->ctrlrs, ctrlr, link);
+
+	return 0;
+}
+
+void
+spdk_nvmf_subsystem_remove_ctrlr(struct spdk_nvmf_subsystem *subsystem,
+				 struct spdk_nvmf_ctrlr *ctrlr)
+{
+	assert(subsystem == ctrlr->subsys);
+	TAILQ_REMOVE(&subsystem->ctrlrs, ctrlr, link);
+}
+
+struct spdk_nvmf_ctrlr *
+spdk_nvmf_subsystem_get_ctrlr(struct spdk_nvmf_subsystem *subsystem, uint16_t cntlid)
+{
+	struct spdk_nvmf_ctrlr *ctrlr;
+
+	TAILQ_FOREACH(ctrlr, &subsystem->ctrlrs, link) {
+		if (ctrlr->cntlid == cntlid) {
+			return ctrlr;
+		}
+	}
+
+	return NULL;
 }

test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c

@@ -115,12 +115,24 @@ spdk_nvmf_subsystem_get_next_ns(struct spdk_nvmf_subsystem *subsystem, struct sp
 	return NULL;
 }
 
-uint16_t
+int
-spdk_nvmf_subsystem_gen_cntlid(struct spdk_nvmf_subsystem *subsystem)
+spdk_nvmf_subsystem_add_ctrlr(struct spdk_nvmf_subsystem *subsystem, struct spdk_nvmf_ctrlr *ctrlr)
 {
 	return 0;
 }
 
+void
+spdk_nvmf_subsystem_remove_ctrlr(struct spdk_nvmf_subsystem *subsystem,
+				 struct spdk_nvmf_ctrlr *ctrlr)
+{
+}
+
+struct spdk_nvmf_ctrlr *
+spdk_nvmf_subsystem_get_ctrlr(struct spdk_nvmf_subsystem *subsystem, uint16_t cntlid)
+{
+	return NULL;
+}
+
 bool
 spdk_nvmf_ctrlr_dsm_supported(struct spdk_nvmf_ctrlr *ctrlr)
 {