nvmf: Clarify transport API for listen and accept

There are now three simple functions on the transport:

listen(transport, trid)
stop_listen(transport, trid)
accept(transport)

This makes the code quite a bit simpler.

Change-Id: I550343a084b5c095240703952c8c07ae535b5c16
Signed-off-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-on: https://review.gerrithub.io/371774
Reviewed-by: Daniel Verkamp <daniel.verkamp@intel.com>
Tested-by: SPDK Automated Test System <sys_sgsw@intel.com>

lib/nvmf/nvmf.c

@@ -133,7 +133,7 @@ spdk_nvmf_listen_addr_destroy(struct spdk_nvmf_listen_addr *addr)
 		return;
 	}
 
-	spdk_nvmf_transport_listen_addr_remove(transport, addr);
+	spdk_nvmf_transport_stop_listen(transport, &addr->trid);
 	free(addr);
 }
 
@@ -143,7 +143,7 @@ spdk_nvmf_tgt_poll(void)
 	struct spdk_nvmf_transport *transport, *tmp;
 
 	TAILQ_FOREACH_SAFE(transport, &g_nvmf_tgt.transports, link, tmp) {
-		spdk_nvmf_transport_acceptor_poll(transport);
+		spdk_nvmf_transport_accept(transport);
 	}
 }
 

lib/nvmf/rdma.c

@@ -183,7 +183,6 @@ struct spdk_nvmf_rdma_listen_addr {
 	struct ibv_device_attr 			attr;
 	struct ibv_comp_channel			*comp_channel;
 	uint32_t				ref;
-	bool					is_listened;
 	TAILQ_ENTRY(spdk_nvmf_rdma_listen_addr)	link;
 };
 
@@ -954,15 +953,6 @@ spdk_nvmf_rdma_create(struct spdk_nvmf_tgt *tgt)
 	return transport;
 }
 
-static void
-spdk_nvmf_rdma_listen_addr_free(struct spdk_nvmf_rdma_listen_addr *addr)
-{
-	if (!addr) {
-		return;
-	}
-
-	free(addr);
-}
 static int
 spdk_nvmf_rdma_destroy(struct spdk_nvmf_transport *transport)
 {
@@ -980,21 +970,135 @@ spdk_nvmf_rdma_destroy(struct spdk_nvmf_transport *transport)
 }
 
 static int
-spdk_nvmf_rdma_listen_remove(struct spdk_nvmf_transport *transport,
+spdk_nvmf_rdma_listen(struct spdk_nvmf_transport *transport,
-			     struct spdk_nvmf_listen_addr *listen_addr)
+		      const struct spdk_nvme_transport_id *trid)
+{
+	struct spdk_nvmf_rdma_listen_addr *addr_tmp, *addr;
+	struct sockaddr_in saddr;
+	int rc;
+
+	addr = calloc(1, sizeof(*addr));
+	if (!addr) {
+		return -ENOMEM;
+	}
+
+	/* Selectively copy the trid. Things like NQN don't matter here - that
+	 * mapping is enforced elsewhere.
+	 */
+	addr->trid.trtype = SPDK_NVME_TRANSPORT_RDMA;
+	addr->trid.adrfam = trid->adrfam;
+	snprintf(addr->trid.traddr, sizeof(addr->trid.traddr), "%s", trid->traddr);
+	snprintf(addr->trid.trsvcid, sizeof(addr->trid.trsvcid), "%s", trid->trsvcid);
+
+	pthread_mutex_lock(&g_rdma.lock);
+	assert(g_rdma.event_channel != NULL);
+	TAILQ_FOREACH(addr_tmp, &g_rdma.listen_addrs, link) {
+		if (spdk_nvme_transport_id_compare(&addr_tmp->trid, &addr->trid) == 0) {
+			addr_tmp->ref++;
+			free(addr);
+			/* Already listening at this address */
+			pthread_mutex_unlock(&g_rdma.lock);
+			return 0;
+		}
+	}
+
+	rc = rdma_create_id(g_rdma.event_channel, &addr->id, addr, RDMA_PS_TCP);
+	if (rc < 0) {
+		SPDK_ERRLOG("rdma_create_id() failed\n");
+		free(addr);
+		pthread_mutex_unlock(&g_rdma.lock);
+		return rc;
+	}
+
+	memset(&saddr, 0, sizeof(saddr));
+	saddr.sin_family = AF_INET;
+	saddr.sin_addr.s_addr = inet_addr(addr->trid.traddr);
+	saddr.sin_port = htons((uint16_t)strtoul(addr->trid.trsvcid, NULL, 10));
+	rc = rdma_bind_addr(addr->id, (struct sockaddr *)&saddr);
+	if (rc < 0) {
+		SPDK_ERRLOG("rdma_bind_addr() failed\n");
+		rdma_destroy_id(addr->id);
+		free(addr);
+		pthread_mutex_unlock(&g_rdma.lock);
+		return rc;
+	}
+
+	rc = ibv_query_device(addr->id->verbs, &addr->attr);
+	if (rc < 0) {
+		SPDK_ERRLOG("Failed to query RDMA device attributes.\n");
+		rdma_destroy_id(addr->id);
+		free(addr);
+		pthread_mutex_unlock(&g_rdma.lock);
+		return rc;
+	}
+
+	addr->comp_channel = ibv_create_comp_channel(addr->id->verbs);
+	if (!addr->comp_channel) {
+		SPDK_ERRLOG("Failed to create completion channel\n");
+		rdma_destroy_id(addr->id);
+		free(addr);
+		pthread_mutex_unlock(&g_rdma.lock);
+		return rc;
+	}
+	SPDK_TRACELOG(SPDK_TRACE_RDMA, "For listen id %p with context %p, created completion channel %p\n",
+		      addr->id, addr->id->verbs, addr->comp_channel);
+
+	rc = fcntl(addr->comp_channel->fd, F_SETFL, O_NONBLOCK);
+	if (rc < 0) {
+		SPDK_ERRLOG("fcntl to set comp channel to non-blocking failed\n");
+		ibv_destroy_comp_channel(addr->comp_channel);
+		rdma_destroy_id(addr->id);
+		free(addr);
+		pthread_mutex_unlock(&g_rdma.lock);
+		return rc;
+	}
+
+	rc = rdma_listen(addr->id, 10); /* 10 = backlog */
+	if (rc < 0) {
+		SPDK_ERRLOG("rdma_listen() failed\n");
+		ibv_destroy_comp_channel(addr->comp_channel);
+		rdma_destroy_id(addr->id);
+		free(addr);
+		pthread_mutex_unlock(&g_rdma.lock);
+		return rc;
+	}
+
+	SPDK_NOTICELOG("*** NVMf Target Listening on %s port %d ***\n",
+		       addr->trid.traddr, ntohs(rdma_get_src_port(addr->id)));
+
+	addr->ref = 1;
+
+	TAILQ_INSERT_TAIL(&g_rdma.listen_addrs, addr, link);
+	pthread_mutex_unlock(&g_rdma.lock);
+
+	return 0;
+}
+
+static int
+spdk_nvmf_rdma_stop_listen(struct spdk_nvmf_transport *transport,
+			   const struct spdk_nvme_transport_id *_trid)
 {
 	struct spdk_nvmf_rdma_listen_addr *addr, *tmp;
+	struct spdk_nvme_transport_id trid = {};
+
+	/* Selectively copy the trid. Things like NQN don't matter here - that
+	 * mapping is enforced elsewhere.
+	 */
+	trid.trtype = SPDK_NVME_TRANSPORT_RDMA;
+	trid.adrfam = _trid->adrfam;
+	snprintf(trid.traddr, sizeof(addr->trid.traddr), "%s", _trid->traddr);
+	snprintf(trid.trsvcid, sizeof(addr->trid.trsvcid), "%s", _trid->trsvcid);
 
 	pthread_mutex_lock(&g_rdma.lock);
 	TAILQ_FOREACH_SAFE(addr, &g_rdma.listen_addrs, link, tmp) {
-		if (spdk_nvme_transport_id_compare(&listen_addr->trid, &addr->trid) == 0) {
+		if (spdk_nvme_transport_id_compare(&addr->trid, &trid) == 0) {
 			assert(addr->ref > 0);
 			addr->ref--;
-			if (!addr->ref) {
+			if (addr->ref == 0) {
 				TAILQ_REMOVE(&g_rdma.listen_addrs, addr, link);
 				ibv_destroy_comp_channel(addr->comp_channel);
 				rdma_destroy_id(addr->id);
-				spdk_nvmf_rdma_listen_addr_free(addr);
+				free(addr);
 			}
 			break;
 		}
@@ -1008,48 +1112,16 @@ static int
 spdk_nvmf_rdma_poll(struct spdk_nvmf_qpair *qpair);
 
 static void
-spdk_nvmf_rdma_addr_listen_init(struct spdk_nvmf_rdma_listen_addr *addr)
+spdk_nvmf_rdma_accept(struct spdk_nvmf_transport *transport)
-{
-	int rc;
-
-	rc = rdma_listen(addr->id, 10); /* 10 = backlog */
-	if (rc < 0) {
-		SPDK_ERRLOG("rdma_listen() failed\n");
-		addr->ref--;
-		assert(addr->ref == 0);
-		TAILQ_REMOVE(&g_rdma.listen_addrs, addr, link);
-		ibv_destroy_comp_channel(addr->comp_channel);
-		rdma_destroy_id(addr->id);
-		spdk_nvmf_rdma_listen_addr_free(addr);
-		return;
-	}
-
-	addr->is_listened = true;
-
-	SPDK_NOTICELOG("*** NVMf Target Listening on %s port %d ***\n",
-		       addr->trid.traddr, ntohs(rdma_get_src_port(addr->id)));
-}
-
-static void
-spdk_nvmf_rdma_acceptor_poll(struct spdk_nvmf_transport *transport)
 {
 	struct rdma_cm_event		*event;
 	int				rc;
 	struct spdk_nvmf_rdma_qpair	*rdma_qpair, *tmp;
-	struct spdk_nvmf_rdma_listen_addr *addr = NULL, *addr_tmp;
 
 	if (g_rdma.event_channel == NULL) {
 		return;
 	}
 
-	pthread_mutex_lock(&g_rdma.lock);
-	TAILQ_FOREACH_SAFE(addr, &g_rdma.listen_addrs, link, addr_tmp) {
-		if (!addr->is_listened) {
-			spdk_nvmf_rdma_addr_listen_init(addr);
-		}
-	}
-	pthread_mutex_unlock(&g_rdma.lock);
-
 	/* Process pending connections for incoming capsules. The only capsule
 	 * this should ever find is a CONNECT request. */
 	TAILQ_FOREACH_SAFE(rdma_qpair, &g_pending_conns, link, tmp) {
@@ -1104,93 +1176,6 @@ spdk_nvmf_rdma_acceptor_poll(struct spdk_nvmf_transport *transport)
 	}
 }
 
-static int
-spdk_nvmf_rdma_listen(struct spdk_nvmf_transport *transport,
-		      struct spdk_nvmf_listen_addr *listen_addr)
-{
-	struct spdk_nvmf_rdma_listen_addr *addr;
-	struct sockaddr_in saddr;
-	int rc;
-
-	pthread_mutex_lock(&g_rdma.lock);
-	assert(g_rdma.event_channel != NULL);
-	TAILQ_FOREACH(addr, &g_rdma.listen_addrs, link) {
-		if (spdk_nvme_transport_id_compare(&listen_addr->trid, &addr->trid) == 0) {
-			addr->ref++;
-			/* Already listening at this address */
-			pthread_mutex_unlock(&g_rdma.lock);
-			return 0;
-		}
-	}
-
-	addr = calloc(1, sizeof(*addr));
-	if (!addr) {
-		pthread_mutex_unlock(&g_rdma.lock);
-		return -1;
-	}
-
-	addr->trid = listen_addr->trid;
-
-	rc = rdma_create_id(g_rdma.event_channel, &addr->id, addr, RDMA_PS_TCP);
-	if (rc < 0) {
-		SPDK_ERRLOG("rdma_create_id() failed\n");
-		spdk_nvmf_rdma_listen_addr_free(addr);
-		pthread_mutex_unlock(&g_rdma.lock);
-		return -1;
-	}
-
-	memset(&saddr, 0, sizeof(saddr));
-	saddr.sin_family = AF_INET;
-	saddr.sin_addr.s_addr = inet_addr(addr->trid.traddr);
-	saddr.sin_port = htons((uint16_t)strtoul(addr->trid.trsvcid, NULL, 10));
-	rc = rdma_bind_addr(addr->id, (struct sockaddr *)&saddr);
-	if (rc < 0) {
-		SPDK_ERRLOG("rdma_bind_addr() failed\n");
-		rdma_destroy_id(addr->id);
-		spdk_nvmf_rdma_listen_addr_free(addr);
-		pthread_mutex_unlock(&g_rdma.lock);
-		return -1;
-	}
-
-	rc = ibv_query_device(addr->id->verbs, &addr->attr);
-	if (rc < 0) {
-		SPDK_ERRLOG("Failed to query RDMA device attributes.\n");
-		rdma_destroy_id(addr->id);
-		spdk_nvmf_rdma_listen_addr_free(addr);
-		pthread_mutex_unlock(&g_rdma.lock);
-		return -1;
-	}
-
-	addr->comp_channel = ibv_create_comp_channel(addr->id->verbs);
-	if (!addr->comp_channel) {
-		SPDK_ERRLOG("Failed to create completion channel\n");
-		rdma_destroy_id(addr->id);
-		spdk_nvmf_rdma_listen_addr_free(addr);
-		pthread_mutex_unlock(&g_rdma.lock);
-		return -1;
-	}
-	SPDK_TRACELOG(SPDK_TRACE_RDMA, "For listen id %p with context %p, created completion channel %p\n",
-		      addr->id, addr->id->verbs, addr->comp_channel);
-
-	rc = fcntl(addr->comp_channel->fd, F_SETFL, O_NONBLOCK);
-	if (rc < 0) {
-		SPDK_ERRLOG("fcntl to set comp channel to non-blocking failed\n");
-		rdma_destroy_id(addr->id);
-		ibv_destroy_comp_channel(addr->comp_channel);
-		spdk_nvmf_rdma_listen_addr_free(addr);
-		pthread_mutex_unlock(&g_rdma.lock);
-		return -1;
-	}
-
-
-	addr->ref = 1;
-	TAILQ_INSERT_TAIL(&g_rdma.listen_addrs, addr, link);
-	pthread_mutex_unlock(&g_rdma.lock);
-
-
-	return 0;
-}
-
 static void
 spdk_nvmf_rdma_discover(struct spdk_nvmf_transport *transport,
 			struct spdk_nvmf_listen_addr *listen_addr,
@@ -1596,10 +1581,10 @@ const struct spdk_nvmf_transport_ops spdk_nvmf_transport_rdma = {
 	.create = spdk_nvmf_rdma_create,
 	.destroy = spdk_nvmf_rdma_destroy,
 
-	.acceptor_poll = spdk_nvmf_rdma_acceptor_poll,
+	.listen = spdk_nvmf_rdma_listen,
+	.stop_listen = spdk_nvmf_rdma_stop_listen,
+	.accept = spdk_nvmf_rdma_accept,
 
-	.listen_addr_add = spdk_nvmf_rdma_listen,
-	.listen_addr_remove = spdk_nvmf_rdma_listen_remove,
 	.listen_addr_discover = spdk_nvmf_rdma_discover,
 
 	.ctrlr_init = spdk_nvmf_rdma_ctrlr_init,

lib/nvmf/subsystem.c

@@ -287,7 +287,7 @@ spdk_nvmf_tgt_listen(struct spdk_nvme_transport_id *trid)
 		return NULL;
 	}
 
-	rc = spdk_nvmf_transport_listen_addr_add(transport, listen_addr);
+	rc = spdk_nvmf_transport_listen(transport, trid);
 	if (rc < 0) {
 		free(listen_addr);
 		SPDK_ERRLOG("Unable to listen on address '%s'\n", trid->traddr);

lib/nvmf/transport.c

@@ -92,24 +92,24 @@ spdk_nvmf_transport_destroy(struct spdk_nvmf_transport *transport)
 	return transport->ops->destroy(transport);
 }
 
+int
+spdk_nvmf_transport_listen(struct spdk_nvmf_transport *transport,
+			   const struct spdk_nvme_transport_id *trid)
+{
+	return transport->ops->listen(transport, trid);
+}
+
+int
+spdk_nvmf_transport_stop_listen(struct spdk_nvmf_transport *transport,
+				const struct spdk_nvme_transport_id *trid)
+{
+	return transport->ops->stop_listen(transport, trid);
+}
+
 void
-spdk_nvmf_transport_acceptor_poll(struct spdk_nvmf_transport *transport)
+spdk_nvmf_transport_accept(struct spdk_nvmf_transport *transport)
 {
-	transport->ops->acceptor_poll(transport);
+	transport->ops->accept(transport);
-}
-
-int
-spdk_nvmf_transport_listen_addr_add(struct spdk_nvmf_transport *transport,
-				    struct spdk_nvmf_listen_addr *listen_addr)
-{
-	return transport->ops->listen_addr_add(transport, listen_addr);
-}
-
-int
-spdk_nvmf_transport_listen_addr_remove(struct spdk_nvmf_transport *transport,
-				       struct spdk_nvmf_listen_addr *listen_addr)
-{
-	return transport->ops->listen_addr_remove(transport, listen_addr);
 }
 
 void

lib/nvmf/transport.h

@@ -64,24 +64,23 @@ struct spdk_nvmf_transport_ops {
 	 */
 	int (*destroy)(struct spdk_nvmf_transport *transport);
 
+	/**
+	  * Instruct the transport to accept new connections at the address
+	  * provided. This may be called multiple times.
+	  */
+	int (*listen)(struct spdk_nvmf_transport *transport,
+		      const struct spdk_nvme_transport_id *trid);
+
+	/**
+	  * Stop accepting new connections at the given address.
+	  */
+	int (*stop_listen)(struct spdk_nvmf_transport *transport,
+			   const struct spdk_nvme_transport_id *trid);
+
 	/**
 	 * Check for new connections on the transport.
 	 */
-	void (*acceptor_poll)(struct spdk_nvmf_transport *transport);
+	void (*accept)(struct spdk_nvmf_transport *transport);
-
-	/**
-	  * Instruct the acceptor to listen on the address provided. This
-	  * may be called multiple times.
-	  */
-	int (*listen_addr_add)(struct spdk_nvmf_transport *transport,
-			       struct spdk_nvmf_listen_addr *listen_addr);
-
-	/**
-	  * Instruct to remove listening on the address provided. This
-	  * may be called multiple times.
-	  */
-	int (*listen_addr_remove)(struct spdk_nvmf_transport *transport,
-				  struct spdk_nvmf_listen_addr *listen_addr);
 
 	/**
 	 * Fill out a discovery log entry for a specific listen address.
@@ -136,13 +135,13 @@ struct spdk_nvmf_transport *spdk_nvmf_transport_create(struct spdk_nvmf_tgt *tgt
 		enum spdk_nvme_transport_type type);
 int spdk_nvmf_transport_destroy(struct spdk_nvmf_transport *transport);
 
-void spdk_nvmf_transport_acceptor_poll(struct spdk_nvmf_transport *transport);
+int spdk_nvmf_transport_listen(struct spdk_nvmf_transport *transport,
+			       const struct spdk_nvme_transport_id *trid);
 
-int spdk_nvmf_transport_listen_addr_add(struct spdk_nvmf_transport *transport,
+int spdk_nvmf_transport_stop_listen(struct spdk_nvmf_transport *transport,
-					struct spdk_nvmf_listen_addr *listen_addr);
+				    const struct spdk_nvme_transport_id *trid);
 
-int spdk_nvmf_transport_listen_addr_remove(struct spdk_nvmf_transport *transport,
+void spdk_nvmf_transport_accept(struct spdk_nvmf_transport *transport);
-		struct spdk_nvmf_listen_addr *listen_addr);
 
 void spdk_nvmf_transport_listen_addr_discover(struct spdk_nvmf_transport *transport,
 		struct spdk_nvmf_listen_addr *listen_addr,

test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c

@@ -80,8 +80,8 @@ spdk_bdev_get_name(const struct spdk_bdev *bdev)
 }
 
 int
-spdk_nvmf_transport_listen_addr_add(struct spdk_nvmf_transport *transport,
+spdk_nvmf_transport_listen(struct spdk_nvmf_transport *transport,
-				    struct spdk_nvmf_listen_addr *listen_addr)
+			   const struct spdk_nvme_transport_id *trid)
 {
 	return 0;
 }

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

@@ -67,8 +67,8 @@ spdk_nvmf_listen_addr_destroy(struct spdk_nvmf_listen_addr *addr)
 }
 
 int
-spdk_nvmf_transport_listen_addr_add(struct spdk_nvmf_transport *transport,
+spdk_nvmf_transport_listen(struct spdk_nvmf_transport *transport,
-				    struct spdk_nvmf_listen_addr *listen_addr)
+			   const struct spdk_nvme_transport_id *trid)
 {
 	return 0;
 }