Skip to contentSkip to conversation

TCP transport binding for NVMe over Fabrics

PATCH In review Sagi Grimberg 4 participants 28 messages Nov 15, 2018 – Nov 20, 2018

Subsystems (9)
Networking Network drivers Block layer Library code +5 more subsystems
lore
Share & tools
lore

Sagi Grimberg

author
#0 lore raw

cover TCP transport binding for NVMe over Fabrics

This patch set implements the NVMe over Fabrics TCP host and the target drivers. Now NVMe over Fabrics can run on every Ethernet port in the world. The implementation conforms to NVMe over Fabrics 1.1 specification (which will include already publicly available NVMe/TCP transport binding, TP 8000).

The host driver hooks into the NVMe host stack and implements the TCP transport binding for NVMe over Fabrics. The NVMe over Fabrics TCP host driver is responsible for establishing a NVMe/TCP connection, TCP event and error handling and data-plane messaging and stream processing.

The target driver hooks into the NVMe target core stack and implements the TCP transport binding. The NVMe over Fabrics target driver is responsible for accepting and establishing NVMe/TCP connections, TCP event and error handling, and data-plane messaging and stream processing.

The implementation of both the host and target are fairly simple and straight-forward. Every NVMe queue is backed by a TCP socket that provides us reliable, in-order delivery of fabrics capsules and/or data.

All NVMe queues are sharded over a private bound workqueue such that we always have a single context handling the byte stream and we don't need to worry about any locking/serialization. In addition, close attention was paid to a completely non-blocking data plane to minimize context switching and/or unforced scheduling.

I piggybacked nvme-cli patches to the set for completeness.

Also, @netdev mailing list is cc'd as this patch set contains generic helpers for online digest calculation (patches 1-3).

The patchset structure:
- patches 1-3 adds a helper for digest calculation online with data placement
- patches 4-8 are preparatory patches for NVMe/TCP
- patches 9-11 implements NVMe/TCP
- patches 12-14 are nvme-cli additions for NVMe/TCP

Thanks to the members of the Fabrics Linux Driver team that helped development, testing and benchmarking this work.

Gitweb code is available at:

git://git.infradead.org/nvme.git nvme-tcp

Sagi Grimberg (11):
ath6kl: add ath6kl_ prefix to crypto_type
iov_iter: introduce hash_and_copy iter helpers
datagram: introduce skb_copy_and_hash_datagram_iter helper
nvme-core: add work elements to struct nvme_ctrl
nvmet: Add install_queue callout
nvmet: allow configfs tcp trtype configuration
nvme-fabrics: allow user passing header digest
nvme-fabrics: allow user passing data digest
nvme-tcp: Add protocol header
nvmet-tcp: add NVMe over TCP target driver
nvme-tcp: add NVMe over TCP host driver

 drivers/net/wireless/ath/ath6kl/cfg80211.c |    2 +-
 drivers/net/wireless/ath/ath6kl/common.h   |    2 +-
 drivers/net/wireless/ath/ath6kl/wmi.c      |    6 +-
 drivers/net/wireless/ath/ath6kl/wmi.h      |    6 +-
 drivers/nvme/host/Kconfig                  |   15 +
 drivers/nvme/host/Makefile                 |    3 +
 drivers/nvme/host/fabrics.c                |   10 +
 drivers/nvme/host/fabrics.h                |    4 +
 drivers/nvme/host/fc.c                     |   18 +-
 drivers/nvme/host/nvme.h                   |    2 +
 drivers/nvme/host/rdma.c                   |   19 +-
 drivers/nvme/host/tcp.c                    | 2305 ++++++++++++++++++++
 drivers/nvme/target/Kconfig                |   10 +
 drivers/nvme/target/Makefile               |    2 +
 drivers/nvme/target/configfs.c             |    1 +
 drivers/nvme/target/fabrics-cmd.c          |    9 +
 drivers/nvme/target/nvmet.h                |    1 +
 drivers/nvme/target/tcp.c                  | 1746 +++++++++++++++
 include/linux/nvme-tcp.h                   |  189 ++
 include/linux/nvme.h                       |    1 +
 include/linux/skbuff.h                     |    3 +
 include/linux/uio.h                        |    5 +
 lib/iov_iter.c                             |   31 +
 net/core/datagram.c                        |   90 +
 24 files changed, 4451 insertions(+), 29 deletions(-)
 create mode 100644 drivers/nvme/host/tcp.c
 create mode 100644 drivers/nvme/target/tcp.c
 create mode 100644 include/linux/nvme-tcp.h
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH 1/11 ath6kl: add ath6kl_ prefix to crypto_type

Prevent a namespace conflict as in following patches as skbuff.h will include the crypto API.

Cc Kalle Valo <kvalo at codeaurora.org>
Signed-off-by Sagi Grimberg <sagi at lightbitslabs.com>
---
 drivers/net/wireless/ath/ath6kl/cfg80211.c | 2 +-
 drivers/net/wireless/ath/ath6kl/common.h   | 2 +-
 drivers/net/wireless/ath/ath6kl/wmi.c      | 6 +++---
 drivers/net/wireless/ath/ath6kl/wmi.h      | 6 +++---
 4 files changed, 8 insertions(+), 8 deletions(-)
drivers/net/wireless/ath/ath6kl/cfg80211.c +1 -1
index e121187f371f..fa049c4ae315 100644
@@ -1322,7 +1322,7 @@ static int ath6kl_cfg80211_set_default_key(struct wiphy *wiphy,
 	struct ath6kl_vif *vif = netdev_priv(ndev);
 	struct ath6kl_key *key = NULL;
 	u8 key_usage;
-	enum crypto_type key_type = NONE_CRYPT;
+	enum ath6kl_crypto_type key_type = NONE_CRYPT;
 
 	ath6kl_dbg(ATH6KL_DBG_WLAN_CFG, "%s: index %d\n", __func__, key_index);
drivers/net/wireless/ath/ath6kl/common.h +1 -1
index 4f82e8632d37..d6e5234f67a1 100644
@@ -67,7 +67,7 @@ struct ath6kl_llc_snap_hdr {
 	__be16 eth_type;
 } __packed;
 
-enum crypto_type {
+enum ath6kl_crypto_type {
 	NONE_CRYPT          = 0x01,
 	WEP_CRYPT           = 0x02,
 	TKIP_CRYPT          = 0x04,
drivers/net/wireless/ath/ath6kl/wmi.c +3 -3
index 777acc564ac9..9d7ac1ab2d02 100644
@@ -1849,9 +1849,9 @@ int ath6kl_wmi_connect_cmd(struct wmi *wmi, u8 if_idx,
 			   enum network_type nw_type,
 			   enum dot11_auth_mode dot11_auth_mode,
 			   enum auth_mode auth_mode,
-			   enum crypto_type pairwise_crypto,
+			   enum ath6kl_crypto_type pairwise_crypto,
 			   u8 pairwise_crypto_len,
-			   enum crypto_type group_crypto,
+			   enum ath6kl_crypto_type group_crypto,
 			   u8 group_crypto_len, int ssid_len, u8 *ssid,
 			   u8 *bssid, u16 channel, u32 ctrl_flags,
 			   u8 nw_subtype)

@@ -2301,7 +2301,7 @@ int ath6kl_wmi_disctimeout_cmd(struct wmi *wmi, u8 if_idx, u8 timeout)
 }
 
 int ath6kl_wmi_addkey_cmd(struct wmi *wmi, u8 if_idx, u8 key_index,
-			  enum crypto_type key_type,
+			  enum ath6kl_crypto_type key_type,
 			  u8 key_usage, u8 key_len,
 			  u8 *key_rsc, unsigned int key_rsc_len,
 			  u8 *key_material,
drivers/net/wireless/ath/ath6kl/wmi.h +3 -3
index a60bb49fe920..784940ba4c90 100644
@@ -2556,9 +2556,9 @@ int ath6kl_wmi_connect_cmd(struct wmi *wmi, u8 if_idx,
 			   enum network_type nw_type,
 			   enum dot11_auth_mode dot11_auth_mode,
 			   enum auth_mode auth_mode,
-			   enum crypto_type pairwise_crypto,
+			   enum ath6kl_crypto_type pairwise_crypto,
 			   u8 pairwise_crypto_len,
-			   enum crypto_type group_crypto,
+			   enum ath6kl_crypto_type group_crypto,
 			   u8 group_crypto_len, int ssid_len, u8 *ssid,
 			   u8 *bssid, u16 channel, u32 ctrl_flags,
 			   u8 nw_subtype);

@@ -2610,7 +2610,7 @@ int ath6kl_wmi_config_debug_module_cmd(struct wmi *wmi, u32 valid, u32 config);
 
 int ath6kl_wmi_get_stats_cmd(struct wmi *wmi, u8 if_idx);
 int ath6kl_wmi_addkey_cmd(struct wmi *wmi, u8 if_idx, u8 key_index,
-			  enum crypto_type key_type,
+			  enum ath6kl_crypto_type key_type,
 			  u8 key_usage, u8 key_len,
 			  u8 *key_rsc, unsigned int key_rsc_len,
 			  u8 *key_material,
Signature · 1 line
2.17.1
author
#2 lore raw

PATCH 2/11 iov_iter: introduce hash_and_copy iter helpers

In reply to Sagi Grimberg

Allow consumers that want to use iov iterator helpers and also update a predefined hash calculation online when copying data. This is useful when copying incoming network buffers to a local iterator and calculate a digest on the incoming stream. nvme-tcp host driver that will be introduced in following patches is the first consumer via skb_copy_and_hash_datagram_iter.

Signed-off-by Sagi Grimberg <sagi at lightbitslabs.com>
---
 include/linux/uio.h |  5 +++++
 lib/iov_iter.c      | 31 +++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)
include/linux/uio.h +5 -0
index 55ce99ddb912..c299ea1037a7 100644
@@ -11,6 +11,7 @@
 
 #include <linux/kernel.h>
 #include <linux/thread_info.h>
+#include <crypto/hash.h>
 #include <uapi/linux/uio.h>
 
 struct page;

@@ -127,6 +128,8 @@ size_t copy_page_to_iter(struct page *page, size_t offset, size_t bytes,
 			 struct iov_iter *i);
 size_t copy_page_from_iter(struct page *page, size_t offset, size_t bytes,
 			 struct iov_iter *i);
+size_t hash_and_copy_page_to_iter(struct page *page, size_t offset, size_t bytes,
+			 struct iov_iter *i, struct ahash_request *hash);
 
 size_t _copy_to_iter(const void *addr, size_t bytes, struct iov_iter *i);
 size_t _copy_from_iter(void *addr, size_t bytes, struct iov_iter *i);

@@ -269,6 +272,8 @@ static inline void iov_iter_reexpand(struct iov_iter *i, size_t count)
 size_t csum_and_copy_to_iter(const void *addr, size_t bytes, __wsum *csum, struct iov_iter *i);
 size_t csum_and_copy_from_iter(void *addr, size_t bytes, __wsum *csum, struct iov_iter *i);
 bool csum_and_copy_from_iter_full(void *addr, size_t bytes, __wsum *csum, struct iov_iter *i);
+size_t hash_and_copy_to_iter(const void *addr, size_t bytes,
+		struct iov_iter *i, struct ahash_request *hash);
 
 int import_iovec(int type, const struct iovec __user * uvector,
 		 unsigned nr_segs, unsigned fast_segs,
lib/iov_iter.c +31 -0
index 7ebccb5c1637..f15cc4f93ecc 100644
@@ -6,6 +6,7 @@
 #include <linux/vmalloc.h>
 #include <linux/splice.h>
 #include <net/checksum.h>
+#include <linux/scatterlist.h>
 
 #define PIPE_PARANOIA /* for now */
 

@@ -850,6 +851,22 @@ size_t copy_page_to_iter(struct page *page, size_t offset, size_t bytes,
 }
 EXPORT_SYMBOL(copy_page_to_iter);
 
+size_t hash_and_copy_page_to_iter(struct page *page, size_t offset, size_t bytes,
+			 struct iov_iter *i, struct ahash_request *hash)
+{
+	struct scatterlist sg;
+	size_t copied;
+
+	copied = copy_page_to_iter(page, offset, bytes, i);
+	sg_init_marker(&sg, 1);
+	sg_set_page(&sg, page, bytes, offset);
+	ahash_request_set_crypt(hash, &sg, NULL, copied);
+	crypto_ahash_update(hash);
+
+	return copied;
+}
+EXPORT_SYMBOL(hash_and_copy_page_to_iter);
+
 size_t copy_page_from_iter(struct page *page, size_t offset, size_t bytes,
 			 struct iov_iter *i)
 {

@@ -1474,6 +1491,20 @@ size_t csum_and_copy_to_iter(const void *addr, size_t bytes, __wsum *csum,
 }
 EXPORT_SYMBOL(csum_and_copy_to_iter);
 
+size_t hash_and_copy_to_iter(const void *addr, size_t bytes,
+		struct iov_iter *i, struct ahash_request *hash)
+{
+	struct scatterlist sg;
+	size_t copied;
+
+	copied = copy_to_iter(addr, bytes, i);
+	sg_init_one(&sg, addr, copied);
+	ahash_request_set_crypt(hash, &sg, NULL, copied);
+	crypto_ahash_update(hash);
+	return copied;
+}
+EXPORT_SYMBOL(hash_and_copy_to_iter);
+
 int iov_iter_npages(const struct iov_iter *i, int maxpages)
 {
 	size_t size = i->count;
Signature · 1 line
2.17.1
author
#3 lore raw

PATCH 3/11 datagram: introduce skb_copy_and_hash_datagram_iter helper

In reply to Sagi Grimberg

Introduce a helper to copy datagram into an iovec iterator but also update a predefined hash. This is useful for consumers of skb_copy_datagram_iter to also support inflight data digest without having to finish to copy and only then traverse the iovec and calculate the digest hash.

Signed-off-by Sagi Grimberg <sagi at lightbitslabs.com>
---
 include/linux/skbuff.h |  3 ++
 net/core/datagram.c    | 90 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+)
include/linux/skbuff.h +3 -0
index 0ba687454267..4d89bb14c2fc 100644
@@ -3311,6 +3311,9 @@ int skb_copy_and_csum_datagram_msg(struct sk_buff *skb, int hlen,
 				   struct msghdr *msg);
 int skb_copy_datagram_from_iter(struct sk_buff *skb, int offset,
 				 struct iov_iter *from, int len);
+int skb_copy_and_hash_datagram_iter(const struct sk_buff *skb, int offset,
+			   struct iov_iter *to, int len,
+			   struct ahash_request *hash);
 int zerocopy_sg_from_iter(struct sk_buff *skb, struct iov_iter *frm);
 void skb_free_datagram(struct sock *sk, struct sk_buff *skb);
 void __skb_free_datagram_locked(struct sock *sk, struct sk_buff *skb, int len);
net/core/datagram.c +90 -0

96 lines not on this page · raw message

Signature · 1 line
2.17.1
author
#4 lore raw

PATCH 4/11 nvme-core: add work elements to struct nvme_ctrl

In reply to Sagi Grimberg

connect_work and err_work will be reused by nvme-tcp so share those in nvme_ctrl for rdma and fc to share.

Signed-off-by Sagi Grimberg <sagi at lightbitslabs.com>
---
 drivers/nvme/host/fc.c   | 18 ++++++++----------
 drivers/nvme/host/nvme.h |  2 ++
 drivers/nvme/host/rdma.c | 19 ++++++++-----------
 3 files changed, 18 insertions(+), 21 deletions(-)
drivers/nvme/host/fc.c +8 -10
index 83131e42b336..16812e427e17 100644
@@ -159,8 +159,6 @@ struct nvme_fc_ctrl {
 	struct blk_mq_tag_set	admin_tag_set;
 	struct blk_mq_tag_set	tag_set;
 
-	struct delayed_work	connect_work;
-
 	struct kref		ref;
 	u32			flags;
 	u32			iocnt;

@@ -547,7 +545,7 @@ nvme_fc_resume_controller(struct nvme_fc_ctrl *ctrl)
 			"NVME-FC{%d}: connectivity re-established. "
 			"Attempting reconnect\n", ctrl->cnum);
 
-		queue_delayed_work(nvme_wq, &ctrl->connect_work, 0);
+		queue_delayed_work(nvme_wq, &ctrl->ctrl.connect_work, 0);
 		break;
 
 	case NVME_CTRL_RESETTING:

@@ -2815,7 +2813,7 @@ nvme_fc_delete_ctrl(struct nvme_ctrl *nctrl)
 {
 	struct nvme_fc_ctrl *ctrl = to_fc_ctrl(nctrl);
 
-	cancel_delayed_work_sync(&ctrl->connect_work);
+	cancel_delayed_work_sync(&ctrl->ctrl.connect_work);
 	/*
 	 * kill the association on the link side.  this will block
 	 * waiting for io to terminate

@@ -2850,7 +2848,7 @@ nvme_fc_reconnect_or_delete(struct nvme_fc_ctrl *ctrl, int status)
 		else if (time_after(jiffies + recon_delay, rport->dev_loss_end))
 			recon_delay = rport->dev_loss_end - jiffies;
 
-		queue_delayed_work(nvme_wq, &ctrl->connect_work, recon_delay);
+		queue_delayed_work(nvme_wq, &ctrl->ctrl.connect_work, recon_delay);
 	} else {
 		if (portptr->port_state == FC_OBJSTATE_ONLINE)
 			dev_warn(ctrl->ctrl.device,

@@ -2918,7 +2916,7 @@ nvme_fc_connect_ctrl_work(struct work_struct *work)
 
 	struct nvme_fc_ctrl *ctrl =
 			container_of(to_delayed_work(work),
-				struct nvme_fc_ctrl, connect_work);
+				struct nvme_fc_ctrl, ctrl.connect_work);
 
 	ret = nvme_fc_create_association(ctrl);
 	if (ret)

@@ -3015,7 +3013,7 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 	kref_init(&ctrl->ref);
 
 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_fc_reset_ctrl_work);
-	INIT_DELAYED_WORK(&ctrl->connect_work, nvme_fc_connect_ctrl_work);
+	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work, nvme_fc_connect_ctrl_work);
 	spin_lock_init(&ctrl->lock);
 
 	/* io queue count */

@@ -3086,7 +3084,7 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 
 	nvme_get_ctrl(&ctrl->ctrl);
 
-	if (!queue_delayed_work(nvme_wq, &ctrl->connect_work, 0)) {
+	if (!queue_delayed_work(nvme_wq, &ctrl->ctrl.connect_work, 0)) {
 		nvme_put_ctrl(&ctrl->ctrl);
 		dev_err(ctrl->ctrl.device,
 			"NVME-FC{%d}: failed to schedule initial connect\n",

@@ -3094,7 +3092,7 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 		goto fail_ctrl;
 	}
 
-	flush_delayed_work(&ctrl->connect_work);
+	flush_delayed_work(&ctrl->ctrl.connect_work);
 
 	dev_info(ctrl->ctrl.device,
 		"NVME-FC{%d}: new ctrl: NQN \"%s\"\n",

@@ -3105,7 +3103,7 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 fail_ctrl:
 	nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING);
 	cancel_work_sync(&ctrl->ctrl.reset_work);
-	cancel_delayed_work_sync(&ctrl->connect_work);
+	cancel_delayed_work_sync(&ctrl->ctrl.connect_work);
 
 	ctrl->ctrl.opts = NULL;
drivers/nvme/host/nvme.h +2 -0
index 27663ce3044e..031195e5d7d3 100644
@@ -240,6 +240,8 @@ struct nvme_ctrl {
 	u16 maxcmd;
 	int nr_reconnects;
 	struct nvmf_ctrl_options *opts;
+	struct delayed_work connect_work;
+	struct work_struct err_work;
 };
 
 struct nvme_subsystem {
drivers/nvme/host/rdma.c +8 -11
index 4468d672ced9..779c2c043242 100644
@@ -101,12 +101,9 @@ struct nvme_rdma_ctrl {
 
 	/* other member variables */
 	struct blk_mq_tag_set	tag_set;
-	struct work_struct	err_work;
 
 	struct nvme_rdma_qe	async_event_sqe;
 
-	struct delayed_work	reconnect_work;
-
 	struct list_head	list;
 
 	struct blk_mq_tag_set	admin_tag_set;

@@ -910,8 +907,8 @@ static void nvme_rdma_stop_ctrl(struct nvme_ctrl *nctrl)
 {
 	struct nvme_rdma_ctrl *ctrl = to_rdma_ctrl(nctrl);
 
-	cancel_work_sync(&ctrl->err_work);
-	cancel_delayed_work_sync(&ctrl->reconnect_work);
+	cancel_work_sync(&ctrl->ctrl.err_work);
+	cancel_delayed_work_sync(&ctrl->ctrl.connect_work);
 }
 
 static void nvme_rdma_free_ctrl(struct nvme_ctrl *nctrl)

@@ -943,7 +940,7 @@ static void nvme_rdma_reconnect_or_remove(struct nvme_rdma_ctrl *ctrl)
 	if (nvmf_should_reconnect(&ctrl->ctrl)) {
 		dev_info(ctrl->ctrl.device, "Reconnecting in %d seconds...\n",
 			ctrl->ctrl.opts->reconnect_delay);
-		queue_delayed_work(nvme_wq, &ctrl->reconnect_work,
+		queue_delayed_work(nvme_wq, &ctrl->ctrl.connect_work,
 				ctrl->ctrl.opts->reconnect_delay * HZ);
 	} else {
 		nvme_delete_ctrl(&ctrl->ctrl);

@@ -1015,7 +1012,7 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
 static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
 {
 	struct nvme_rdma_ctrl *ctrl = container_of(to_delayed_work(work),
-			struct nvme_rdma_ctrl, reconnect_work);
+			struct nvme_rdma_ctrl, ctrl.connect_work);
 
 	++ctrl->ctrl.nr_reconnects;
 

@@ -1038,7 +1035,7 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
 static void nvme_rdma_error_recovery_work(struct work_struct *work)
 {
 	struct nvme_rdma_ctrl *ctrl = container_of(work,
-			struct nvme_rdma_ctrl, err_work);
+			struct nvme_rdma_ctrl, ctrl.err_work);
 
 	nvme_stop_keep_alive(&ctrl->ctrl);
 	nvme_rdma_teardown_io_queues(ctrl, false);

@@ -1059,7 +1056,7 @@ static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl)
 	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING))
 		return;
 
-	queue_work(nvme_wq, &ctrl->err_work);
+	queue_work(nvme_wq, &ctrl->ctrl.err_work);
 }
 
 static void nvme_rdma_wr_error(struct ib_cq *cq, struct ib_wc *wc,

@@ -1932,9 +1929,9 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 		goto out_free_ctrl;
 	}
 
-	INIT_DELAYED_WORK(&ctrl->reconnect_work,
+	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work,
 			nvme_rdma_reconnect_ctrl_work);
-	INIT_WORK(&ctrl->err_work, nvme_rdma_error_recovery_work);
+	INIT_WORK(&ctrl->ctrl.err_work, nvme_rdma_error_recovery_work);
 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_rdma_reset_ctrl_work);
 
 	ctrl->ctrl.queue_count = opts->nr_io_queues + 1; /* +1 for admin queue */
Signature · 1 line
2.17.1
Reviewed-by on v1 4/11
#5 lore raw

Re: [PATCH 04/11] nvme-core: add work elements to struct nvme_ctrl

Quoted 4 lines · Sagi Grimberg

connect_work and err_work will be reused by nvme-tcp so share those in nvme_ctrl for rdma and fc to share.

Signed-off-by Sagi Grimberg <sagi at lightbitslabs.com>

looks good (and I see that more sharing should be made in the future as well :) )

Reviewed-by Max Gurtovoy <maxg at mellanox.com>
author
#6 lore raw

PATCH 5/11 nvmet: Add install_queue callout

In reply to Sagi Grimberg

nvmet-tcp will implement it to allocate queue commands which are only known at nvmf connect time (sq size).

Signed-off-by Sagi Grimberg <sagi at lightbitslabs.com>
---
 drivers/nvme/target/fabrics-cmd.c | 9 +++++++++
 drivers/nvme/target/nvmet.h       | 1 +
 2 files changed, 10 insertions(+)
drivers/nvme/target/fabrics-cmd.c +9 -0
index 1f05d8507e35..a84668e8939c 100644
@@ -117,6 +117,15 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
 	nvmet_sq_setup(ctrl, req->sq, qid, sqsize + 1,
 			!!(c->cattr & NVME_CONNECT_DISABLE_SQFLOW));
 
+	if (ctrl->ops->install_queue) {
+		int ret = ctrl->ops->install_queue(req->sq);
+		if (ret) {
+			pr_err("failed to install queue %d cntlid %d ret %d\n",
+				qid, ret, ctrl->cntlid);
+			return NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
+		}
+	}
+
 	return 0;
 }
drivers/nvme/target/nvmet.h +1 -0
index abc603fa725d..1362afe6e0b3 100644
@@ -281,6 +281,7 @@ struct nvmet_fabrics_ops {
 	void (*delete_ctrl)(struct nvmet_ctrl *ctrl);
 	void (*disc_traddr)(struct nvmet_req *req,
 			struct nvmet_port *port, char *traddr);
+	int (*install_queue)(struct nvmet_sq *nvme_sq);
 };
 
 #define NVMET_MAX_INLINE_BIOVEC	8
Signature · 1 line
2.17.1
Reviewed-by on v1 5/11
#7 lore raw

Re: [PATCH 05/11] nvmet: Add install_queue callout

Quoted 14 lines · Sagi Grimberg

nvmet-tcp will implement it to allocate queue commands which are only known at nvmf connect time (sq size).

Signed-off-by Sagi Grimberg <sagi at lightbitslabs.com>
---
  drivers/nvme/target/fabrics-cmd.c | 9 +++++++++
  drivers/nvme/target/nvmet.h       | 1 +
  2 files changed, 10 insertions(+)
drivers/nvme/target/fabrics-cmd.c +0 -0
index 1f05d8507e35..a84668e8939c 100644

drivers/nvme/target/fabrics-cmd.c static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req) · line 118

Quoted 2 lines
drivers/nvme/target/fabrics-cmd.c +0 -0
@@ -117,6 +117,15 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
  	nvmet_sq_setup(ctrl, req->sq, qid, sqsize + 1,
  			!!(c->cattr & NVME_CONNECT_DISABLE_SQFLOW));

what is your code base ? does SQFLOW must for TCP ?

drivers/nvme/target/fabrics-cmd.c static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req) · line 125

Quoted 7 lines
drivers/nvme/target/fabrics-cmd.c +6 -0
+	if (ctrl->ops->install_queue) {
+		int ret = ctrl->ops->install_queue(req->sq);
+		if (ret) {
+			pr_err("failed to install queue %d cntlid %d ret %d\n",
+				qid, ret, ctrl->cntlid);
+			return NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;

I'm not sure regarding the INVALID_PARAM rc.

maybe use NVME_SC_INTERNAL ?

otherwise,

look fine,

Reviewed-by Max Gurtovoy <maxg at mellanox.com>
maintainer
#8 lore raw

Re: [PATCH 05/11] nvmet: Add install_queue callout

Quoted 12 lines

drivers/nvme/target/fabrics-cmd.c static u16 nvmet_install_queue(struct nvmet_ctrl · line 119

Quoted 10 lines
drivers/nvme/target/fabrics-cmd.c +0 -0
index 1f05d8507e35..a84668e8939c 100644
@@ -117,6 +117,15 @@ static u16 nvmet_install_queue(struct nvmet_ctrl 
 *ctrl, struct nvmet_req *req)
 ????? nvmet_sq_setup(ctrl, req->sq, qid, sqsize + 1,
 ????????????? !!(c->cattr & NVME_CONNECT_DISABLE_SQFLOW));

what is your code base ? does SQFLOW must for TCP ?

Not really, just happens to sit on top..

Quoted 10 lines

drivers/nvme/target/fabrics-cmd.c static u16 nvmet_install_queue(struct nvmet_ctrl · line 125

Quoted 6 lines
drivers/nvme/target/fabrics-cmd.c +6 -0
+??? if (ctrl->ops->install_queue) {
+??????? int ret = ctrl->ops->install_queue(req->sq);
+??????? if (ret) {
+??????????? pr_err("failed to install queue %d cntlid %d ret %d\n",
+??????????????? qid, ret, ctrl->cntlid);
+??????????? return NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;

I'm not sure regarding the INVALID_PARAM rc.

maybe use NVME_SC_INTERNAL ?

You're right, perhaps we should propagate ->install_queue() status.

author
#9 lore raw

PATCH 6/11 nvmet: allow configfs tcp trtype configuration

In reply to Sagi Grimberg
Signed-off-by Sagi Grimberg <sagi at lightbitslabs.com>
---
 drivers/nvme/target/configfs.c | 1 +
 1 file changed, 1 insertion(+)
drivers/nvme/target/configfs.c +1 -0
index c2df205096ec..4d28c4d78850 100644
@@ -34,6 +34,7 @@ static const struct nvmet_transport_name {
 } nvmet_transport_names[] = {
 	{ NVMF_TRTYPE_RDMA,	"rdma" },
 	{ NVMF_TRTYPE_FC,	"fc" },
+	{ NVMF_TRTYPE_TCP,	"tcp" },
 	{ NVMF_TRTYPE_LOOP,	"loop" },
 };
Signature · 1 line
2.17.1
Reviewed-by on v1 6/11
#10 lore raw

Re: [PATCH 06/11] nvmet: allow configfs tcp trtype configuration

Quoted 10 lines · Sagi Grimberg
Signed-off-by Sagi Grimberg <sagi at lightbitslabs.com>
---
  drivers/nvme/target/configfs.c | 1 +
  1 file changed, 1 insertion(+)
drivers/nvme/target/configfs.c +0 -0
index c2df205096ec..4d28c4d78850 100644

drivers/nvme/target/configfs.c static const struct nvmet_transport_name { · line 40

Quoted 7 lines
drivers/nvme/target/configfs.c +1 -0
@@ -34,6 +34,7 @@ static const struct nvmet_transport_name {
  } nvmet_transport_names[] = {
  	{ NVMF_TRTYPE_RDMA,	"rdma" },
  	{ NVMF_TRTYPE_FC,	"fc" },
+	{ NVMF_TRTYPE_TCP,	"tcp" },
  	{ NVMF_TRTYPE_LOOP,	"loop" },
  };

looks good,

Reviewed-by Max Gurtovoy <maxg at mellanox.com>
author
#11 lore raw

PATCH 7/11 nvme-fabrics: allow user passing header digest

In reply to Sagi Grimberg

Header digest is a nvme-tcp specific feature, but nothing prevents other transports reusing the concept so do not associate with tcp transport solely.

Signed-off-by Sagi Grimberg <sagi at lightbitslabs.com>
---
 drivers/nvme/host/fabrics.c | 5 +++++
 drivers/nvme/host/fabrics.h | 2 ++
 2 files changed, 7 insertions(+)
drivers/nvme/host/fabrics.c +5 -0
index b78d1a674abc..9a8b57b3b2c1 100644
@@ -614,6 +614,7 @@ static const match_table_t opt_tokens = {
 	{ NVMF_OPT_HOST_ID,		"hostid=%s"		},
 	{ NVMF_OPT_DUP_CONNECT,		"duplicate_connect"	},
 	{ NVMF_OPT_DISABLE_SQFLOW,	"disable_sqflow"	},
+	{ NVMF_OPT_HDR_DIGEST,		"hdr_digest"		},
 	{ NVMF_OPT_ERR,			NULL			}
 };
 

@@ -633,6 +634,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 	opts->reconnect_delay = NVMF_DEF_RECONNECT_DELAY;
 	opts->kato = NVME_DEFAULT_KATO;
 	opts->duplicate_connect = false;
+	opts->hdr_digest = false;
 
 	options = o = kstrdup(buf, GFP_KERNEL);
 	if (!options)

@@ -827,6 +829,9 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 		case NVMF_OPT_DISABLE_SQFLOW:
 			opts->disable_sqflow = true;
 			break;
+		case NVMF_OPT_HDR_DIGEST:
+			opts->hdr_digest = true;
+			break;
 		default:
 			pr_warn("unknown parameter or missing value '%s' in ctrl creation request\n",
 				p);
drivers/nvme/host/fabrics.h +2 -0
index ecd9a006a091..a6127f1a9e8e 100644
@@ -59,6 +59,7 @@ enum {
 	NVMF_OPT_HOST_ID	= 1 << 12,
 	NVMF_OPT_DUP_CONNECT	= 1 << 13,
 	NVMF_OPT_DISABLE_SQFLOW = 1 << 14,
+	NVMF_OPT_HDR_DIGEST	= 1 << 15,
 };
 
 /**

@@ -103,6 +104,7 @@ struct nvmf_ctrl_options {
 	struct nvmf_host	*host;
 	int			max_reconnects;
 	bool			disable_sqflow;
+	bool			hdr_digest;
 };
 
 /*
Signature · 1 line
2.17.1
author
#12 lore raw

PATCH 8/11 nvme-fabrics: allow user passing data digest

In reply to Sagi Grimberg

Data digest is a nvme-tcp specific feature, but nothing prevents other transports reusing the concept so do not associate with tcp transport solely.

Signed-off-by Sagi Grimberg <sagi at lightbitslabs.com>
---
 drivers/nvme/host/fabrics.c | 5 +++++
 drivers/nvme/host/fabrics.h | 2 ++
 2 files changed, 7 insertions(+)
drivers/nvme/host/fabrics.c +5 -0
index 9a8b57b3b2c1..97855a12364e 100644
@@ -615,6 +615,7 @@ static const match_table_t opt_tokens = {
 	{ NVMF_OPT_DUP_CONNECT,		"duplicate_connect"	},
 	{ NVMF_OPT_DISABLE_SQFLOW,	"disable_sqflow"	},
 	{ NVMF_OPT_HDR_DIGEST,		"hdr_digest"		},
+	{ NVMF_OPT_DATA_DIGEST,		"data_digest"		},
 	{ NVMF_OPT_ERR,			NULL			}
 };
 

@@ -635,6 +636,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 	opts->kato = NVME_DEFAULT_KATO;
 	opts->duplicate_connect = false;
 	opts->hdr_digest = false;
+	opts->data_digest = false;
 
 	options = o = kstrdup(buf, GFP_KERNEL);
 	if (!options)

@@ -832,6 +834,9 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 		case NVMF_OPT_HDR_DIGEST:
 			opts->hdr_digest = true;
 			break;
+		case NVMF_OPT_DATA_DIGEST:
+			opts->data_digest = true;
+			break;
 		default:
 			pr_warn("unknown parameter or missing value '%s' in ctrl creation request\n",
 				p);
drivers/nvme/host/fabrics.h +2 -0
index a6127f1a9e8e..524a02a67817 100644
@@ -60,6 +60,7 @@ enum {
 	NVMF_OPT_DUP_CONNECT	= 1 << 13,
 	NVMF_OPT_DISABLE_SQFLOW = 1 << 14,
 	NVMF_OPT_HDR_DIGEST	= 1 << 15,
+	NVMF_OPT_DATA_DIGEST	= 1 << 16,
 };
 
 /**

@@ -105,6 +106,7 @@ struct nvmf_ctrl_options {
 	int			max_reconnects;
 	bool			disable_sqflow;
 	bool			hdr_digest;
+	bool			data_digest;
 };
 
 /*
Signature · 1 line
2.17.1
author
#13 lore raw

PATCH 9/11 nvme-tcp: Add protocol header

In reply to Sagi Grimberg
Signed-off-by Sagi Grimberg <sagi at lightbitslabs.com>
---
 include/linux/nvme-tcp.h | 188 +++++++++++++++++++++++++++++++++++++++
 include/linux/nvme.h     |   1 +
 2 files changed, 189 insertions(+)
 create mode 100644 include/linux/nvme-tcp.h
include/linux/nvme-tcp.h new file +188 -0
new file mode 100644
index 000000000000..33c8afaf63bd
@@ -0,0 +1,188 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * NVMe over Fabrics TCP protocol header.
+ * Copyright (c) 2018 LightBits Labs. All rights reserved.
+ */
+
+#ifndef _LINUX_NVME_TCP_H
+#define _LINUX_NVME_TCP_H
+
+#include <linux/nvme.h>
+
+#define NVME_TCP_DISC_PORT	8009
+#define NVME_TCP_ADMIN_CCSZ	SZ_8K
+
+enum nvme_tcp_pfv {
+	NVME_TCP_PFV_1_0 = 0x0,
+};
+
+enum nvme_tcp_fatal_error_status {
+	NVME_TCP_FES_INVALID_PDU_HDR		= 0x01,
+	NVME_TCP_FES_PDU_SEQ_ERR		= 0x02,
+	NVME_TCP_FES_HDR_DIGEST_ERR		= 0x03,
+	NVME_TCP_FES_DATA_OUT_OF_RANGE		= 0x04,
+	NVME_TCP_FES_R2T_LIMIT_EXCEEDED		= 0x05,
+	NVME_TCP_FES_DATA_LIMIT_EXCEEDED	= 0x05,
+	NVME_TCP_FES_UNSUPPORTED_PARAM		= 0x06,
+};
+
+enum nvme_tcp_digest_option {
+	NVME_TCP_HDR_DIGEST_ENABLE	= (1 << 0),
+	NVME_TCP_DATA_DIGEST_ENABLE	= (1 << 1),
+};
+
+enum nvme_tcp_pdu_type {
+	nvme_tcp_icreq		= 0x0,
+	nvme_tcp_icresp		= 0x1,
+	nvme_tcp_h2c_term	= 0x2,
+	nvme_tcp_c2h_term	= 0x3,
+	nvme_tcp_cmd		= 0x4,
+	nvme_tcp_rsp		= 0x5,
+	nvme_tcp_h2c_data	= 0x6,
+	nvme_tcp_c2h_data	= 0x7,
+	nvme_tcp_r2t		= 0x9,
+};
+
+enum nvme_tcp_pdu_flags {
+	NVME_TCP_F_HDGST		= (1 << 0),
+	NVME_TCP_F_DDGST		= (1 << 1),
+	NVME_TCP_F_DATA_LAST		= (1 << 2),
+	NVME_TCP_F_DATA_SUCCESS		= (1 << 3),
+};
+
+/**
+ * struct nvme_tcp_hdr - nvme tcp pdu common header
+ *
+ * @type:          pdu type
+ * @flags:         pdu specific flags
+ * @hlen:          pdu header length
+ * @pdo:           pdu data offset
+ * @plen:          pdu wire byte length
+ */
+struct nvme_tcp_hdr {
+	__u8	type;
+	__u8	flags;
+	__u8	hlen;
+	__u8	pdo;
+	__le32	plen;
+};
+
+/**
+ * struct nvme_tcp_icreq_pdu - nvme tcp initialize connection request pdu
+ *
+ * @hdr:           pdu generic header
+ * @pfv:           pdu version format
+ * @hpda:          host pdu data alignment (dwords, 0's based)
+ * @digest:        digest types enabled
+ * @maxr2t:        maximum r2ts per request supported
+ */
+struct nvme_tcp_icreq_pdu {
+	struct nvme_tcp_hdr	hdr;
+	__le16			pfv;
+	__u8			hpda;
+	__u8			digest;
+	__le32			maxr2t;
+	__u8			rsvd2[112];
+};
+
+/**
+ * struct nvme_tcp_icresp_pdu - nvme tcp initialize connection response pdu
+ *
+ * @hdr:           pdu common header
+ * @pfv:           pdu version format
+ * @cpda:          controller pdu data alignment (dowrds, 0's based)
+ * @digest:        digest types enabled
+ * @maxdata:       maximum data capsules per r2t supported
+ */
+struct nvme_tcp_icresp_pdu {
+	struct nvme_tcp_hdr	hdr;
+	__le16			pfv;
+	__u8			cpda;
+	__u8			digest;
+	__le32			maxdata;
+	__u8			rsvd[112];
+};
+
+/**
+ * struct nvme_tcp_term_pdu - nvme tcp terminate connection pdu
+ *
+ * @hdr:           pdu common header
+ * @fes:           fatal error status
+ * @fei:           fatal error information
+ */
+struct nvme_tcp_term_pdu {
+	struct nvme_tcp_hdr	hdr;
+	__le16			fes;
+	__le32			fei;
+	__u8			rsvd[8];
+};
+
+/**
+ * struct nvme_tcp_cmd_pdu - nvme tcp command capsule pdu
+ *
+ * @hdr:           pdu common header
+ * @cmd:           nvme command
+ */
+struct nvme_tcp_cmd_pdu {
+	struct nvme_tcp_hdr	hdr;
+	struct nvme_command	cmd;
+};
+
+/**
+ * struct nvme_tcp_rsp_pdu - nvme tcp response capsule pdu
+ *
+ * @hdr:           pdu common header
+ * @hdr:           nvme-tcp generic header
+ * @cqe:           nvme completion queue entry
+ */
+struct nvme_tcp_rsp_pdu {
+	struct nvme_tcp_hdr	hdr;
+	struct nvme_completion	cqe;
+};
+
+/**
+ * struct nvme_tcp_r2t_pdu - nvme tcp ready-to-transfer pdu
+ *
+ * @hdr:           pdu common header
+ * @command_id:    nvme command identifier which this relates to
+ * @ttag:          transfer tag (controller generated)
+ * @r2t_offset:    offset from the start of the command data
+ * @r2t_length:    length the host is allowed to send
+ */
+struct nvme_tcp_r2t_pdu {
+	struct nvme_tcp_hdr	hdr;
+	__u16			command_id;
+	__u16			ttag;
+	__le32			r2t_offset;
+	__le32			r2t_length;
+	__u8			rsvd[4];
+};
+
+/**
+ * struct nvme_tcp_data_pdu - nvme tcp data pdu
+ *
+ * @hdr:           pdu common header
+ * @command_id:    nvme command identifier which this relates to
+ * @ttag:          transfer tag (controller generated)
+ * @data_offset:   offset from the start of the command data
+ * @data_length:   length of the data stream
+ */
+struct nvme_tcp_data_pdu {
+	struct nvme_tcp_hdr	hdr;
+	__u16			command_id;
+	__u16			ttag;
+	__le32			data_offset;
+	__le32			data_length;
+	__u8			rsvd[4];
+};
+
+union nvme_tcp_pdu {
+	struct nvme_tcp_icreq_pdu	icreq;
+	struct nvme_tcp_icresp_pdu	icresp;
+	struct nvme_tcp_cmd_pdu		cmd;
+	struct nvme_tcp_rsp_pdu		rsp;
+	struct nvme_tcp_r2t_pdu		r2t;
+	struct nvme_tcp_data_pdu	data;
+};
+
+#endif /* _LINUX_NVME_TCP_H */
include/linux/nvme.h +1 -0
index 4a560ea04d7c..c651bbf9c7e3 100644
@@ -52,6 +52,7 @@ enum {
 enum {
 	NVMF_TRTYPE_RDMA	= 1,	/* RDMA */
 	NVMF_TRTYPE_FC		= 2,	/* Fibre Channel */
+	NVMF_TRTYPE_TCP		= 3,	/* TCP/IP */
 	NVMF_TRTYPE_LOOP	= 254,	/* Reserved for host usage */
 	NVMF_TRTYPE_MAX,
 };
Signature · 1 line
2.17.1
author
#14 lore raw

PATCH 10/11 nvmet-tcp: add NVMe over TCP target driver

In reply to Sagi Grimberg

This patch implements the TCP transport driver for the NVMe over Fabrics target stack. This allows exporting NVMe over Fabrics functionality over good old TCP/IP.

The driver implements the TP 8000 of how nvme over fabrics capsules and data are encapsulated in nvme-tcp pdus and exchaged on top of a TCP byte stream. nvme-tcp header and data digest are supported as well.

Signed-off-by Sagi Grimberg <sagi at lightbitslabs.com>
Signed-off-by Roy Shterman <roys at lightbitslabs.com>
Signed-off-by Solganik Alexander <sashas at lightbitslabs.com>
---
 drivers/nvme/target/Kconfig  |   10 +
 drivers/nvme/target/Makefile |    2 +
 drivers/nvme/target/tcp.c    | 1746 ++++++++++++++++++++++++++++++++++
 include/linux/nvme-tcp.h     |    1 +
 4 files changed, 1759 insertions(+)
 create mode 100644 drivers/nvme/target/tcp.c
drivers/nvme/target/Kconfig +10 -0
index 3c7b61ddb0d1..d94f25cde019 100644
@@ -60,3 +60,13 @@ config NVME_TARGET_FCLOOP
 	  to test NVMe-FC transport interfaces.
 
 	  If unsure, say N.
+
+config NVME_TARGET_TCP
+	tristate "NVMe over Fabrics TCP target support"
+	depends on INET
+	depends on NVME_TARGET
+	help
+	  This enables the NVMe TCP target support, which allows exporting NVMe
+	  devices over TCP.
+
+	  If unsure, say N.
drivers/nvme/target/Makefile +2 -0
index 8118c93391c6..8c3ad0fb6860 100644
@@ -5,6 +5,7 @@ obj-$(CONFIG_NVME_TARGET_LOOP)		+= nvme-loop.o
 obj-$(CONFIG_NVME_TARGET_RDMA)		+= nvmet-rdma.o
 obj-$(CONFIG_NVME_TARGET_FC)		+= nvmet-fc.o
 obj-$(CONFIG_NVME_TARGET_FCLOOP)	+= nvme-fcloop.o
+obj-$(CONFIG_NVME_TARGET_TCP)		+= nvmet-tcp.o
 
 nvmet-y		+= core.o configfs.o admin-cmd.o fabrics-cmd.o \
 			discovery.o io-cmd-file.o io-cmd-bdev.o

@@ -12,3 +13,4 @@ nvme-loop-y	+= loop.o
 nvmet-rdma-y	+= rdma.o
 nvmet-fc-y	+= fc.o
 nvme-fcloop-y	+= fcloop.o
+nvmet-tcp-y	+= tcp.o
drivers/nvme/target/tcp.c new file +1746 -0

1,746 lines not on this page · raw message

include/linux/nvme-tcp.h +1 -0
index 33c8afaf63bd..685780d1ed04 100644
@@ -11,6 +11,7 @@
 
 #define NVME_TCP_DISC_PORT	8009
 #define NVME_TCP_ADMIN_CCSZ	SZ_8K
+#define NVME_TCP_DIGEST_LENGTH	4
 
 enum nvme_tcp_pfv {
 	NVME_TCP_PFV_1_0 = 0x0,
Signature · 1 line
2.17.1
maintainer
#15 lore raw

Re: [PATCH 10/11] nvmet-tcp: add NVMe over TCP target driver

From: Sagi Grimberg <[email protected]>
Date: Thu, 15 Nov 2018 09:16:22 -0800

drivers/nvme/target/tcp.c line 32

Quoted 11 lines
drivers/nvme/target/tcp.c new file +11 -0
+static unsigned nvmet_tcp_recv_budget = 8;
+module_param_named(recv_budget, nvmet_tcp_recv_budget, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(recv_budget, "recvs budget");
+
+static unsigned nvmet_tcp_send_budget = 8;
+module_param_named(send_budget, nvmet_tcp_send_budget, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(send_budget, "sends budget");
+
+static unsigned nvmet_tcp_io_work_budget = 64;
+module_param_named(io_work_budget, nvmet_tcp_io_work_budget, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(io_work_budget, "io work budget");

I strongly suggest moving away from module parameters for this stuff.

Create a genetlink socket family and allow run time configuration of these knobs by the user.

Thanks.

#16 lore raw

Re: [PATCH 10/11] nvmet-tcp: add NVMe over TCP target driver

Quoted 15 lines · David Miller

From: Sagi Grimberg <sagi at lightbitslabs.com>
Date: Thu, 15 Nov 2018 09:16:22 -0800

drivers/nvme/target/tcp.c line 32

Quoted 11 lines
drivers/nvme/target/tcp.c new file +11 -0
+static unsigned nvmet_tcp_recv_budget = 8;
+module_param_named(recv_budget, nvmet_tcp_recv_budget, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(recv_budget, "recvs budget");
+
+static unsigned nvmet_tcp_send_budget = 8;
+module_param_named(send_budget, nvmet_tcp_send_budget, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(send_budget, "sends budget");
+
+static unsigned nvmet_tcp_io_work_budget = 64;
+module_param_named(io_work_budget, nvmet_tcp_io_work_budget, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(io_work_budget, "io work budget");

I strongly suggest moving away from module parameters for this stuff.

agree here.

also, Sagi, can you explain about the performance trade-offs seen during your development for these values ?

are they HCA/NIC dependent ?

should send/recv ratio be 1:1 ?

should total/send/recv ratio be 8:1:1 ?

Quoted 5 lines

Create a genetlink socket family and allow run time configuration of these knobs by the user.

Thanks.

maintainer
#17 lore raw

Re: [PATCH 10/11] nvmet-tcp: add NVMe over TCP target driver

Quoted 19 lines
Quoted 16 lines
Quoted 13 lines
(quoted diff) +2 -0
+static unsigned nvmet_tcp_recv_budget = 8;
+module_param_named(recv_budget, nvmet_tcp_recv_budget, int, S_IRUGO

| S_IWUSR);

drivers/nvme/target/tcp.c new file +4 -0
+MODULE_PARM_DESC(recv_budget, "recvs budget");
+
+static unsigned nvmet_tcp_send_budget = 8;
+module_param_named(send_budget, nvmet_tcp_send_budget, int, S_IRUGO

| S_IWUSR);

drivers/nvme/target/tcp.c new file +4 -0
+MODULE_PARM_DESC(send_budget, "sends budget");
+
+static unsigned nvmet_tcp_io_work_budget = 64;
+module_param_named(io_work_budget, nvmet_tcp_io_work_budget, int,
Quoted 2 lines

S_IRUGO | S_IWUSR); +MODULE_PARM_DESC(io_work_budget, "io work budget");

I strongly suggest moving away from module parameters for this stuff.

agree here.

Quoted 8 lines

also, Sagi, can you explain about the performance trade-offs seen during your development for these values ?

are they HCA/NIC dependent ?

should send/recv ratio be 1:1 ?

should total/send/recv ratio be 8:1:1 ?

These are not really HW dependent at all, its more about the tradeoff between aggregation vs. fairness multiplexing. The budgets are designed to control how much a specific workload (e.g. nvme queue) can hog the cpu/wire when nvmet is servicing a large number of hosts.

There no constraints about the ratios of the budgets. Its advised though that the io_work_budget would be able to catch at least a few sends/recvs for reasonable aggregation.

I commented to Dave that I prefer not to expose them at this point given that they are not trivial and would require an additional interface to the driver (and its corresponding tool chain).

maintainer
#18 lore raw

Re: [PATCH 10/11] nvmet-tcp: add NVMe over TCP target driver

In reply to David Miller
Quoted 15 lines

drivers/nvme/target/tcp.c line 32

Quoted 12 lines
drivers/nvme/target/tcp.c new file +11 -0
+static unsigned nvmet_tcp_recv_budget = 8;
+module_param_named(recv_budget, nvmet_tcp_recv_budget, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(recv_budget, "recvs budget");
+
+static unsigned nvmet_tcp_send_budget = 8;
+module_param_named(send_budget, nvmet_tcp_send_budget, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(send_budget, "sends budget");
+
+static unsigned nvmet_tcp_io_work_budget = 64;
+module_param_named(io_work_budget, nvmet_tcp_io_work_budget, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(io_work_budget, "io work budget");

I strongly suggest moving away from module parameters for this stuff.

Quoted 2 lines

Create a genetlink socket family and allow run time configuration of these knobs by the user.

Thanks for the feedback Dave.

Given that these are non-trivial knobs to configure at runtime, I think that I'll degenerate them to sane defaults for the initial submission and figure out what interface makes sense afterwards.

Now that I have your attention ;)
I would love you to look at skb_copy_and_hash_datagram_iter as these changes will require an ack from you.

maintainer
#19 lore raw

Re: [PATCH 10/11] nvmet-tcp: add NVMe over TCP target driver

From: Sagi Grimberg <[email protected]>
Date: Mon, 19 Nov 2018 13:26:12 -0800

Quoted 2 lines

I would love you to look at skb_copy_and_hash_datagram_iter as these changes will require an ack from you.

My first impression is that we now have this kind of code pattern in at least two main places and now this will be a third.

I know that nobody likes callbacks because of spectre, but all of these cases could be done with something like:

int __skb_datagram_iter(const struct sk_buff *skb, int offset,

			struct iov_iter *to, int len,
			int (*cb)(void *, int, struct iov_iter *, void *),
			void *data)
{
	...
		n = cb(skb->data + offset, copy, to, data);
	...
}

You get the idea. Then we have one version of all the loops and the different (copy, copy+csum, copy+hash) cases all can be handled by __skb_datagram_iter() but just with a different 'cb' and private 'data'.

maintainer
#20 lore raw

Re: [PATCH 10/11] nvmet-tcp: add NVMe over TCP target driver

Quoted 20 lines
Quoted 3 lines

I would love you to look at skb_copy_and_hash_datagram_iter as these changes will require an ack from you.

My first impression is that we now have this kind of code pattern in at least two main places and now this will be a third.

I know that nobody likes callbacks because of spectre, but all of these cases could be done with something like:

int __skb_datagram_iter(const struct sk_buff *skb, int offset,

			struct iov_iter *to, int len,
			int (*cb)(void *, int, struct iov_iter *, void *),
			void *data)
{
	...
		n = cb(skb->data + offset, copy, to, data);
	...
}
Quoted 4 lines

You get the idea. Then we have one version of all the loops and the different (copy, copy+csum, copy+hash) cases all can be handled by __skb_datagram_iter() but just with a different 'cb' and private 'data'.

I already thought about that, but the fact that we copy both a buffer and a page to the iter (in the most general case) we'd have to carry two callbacks for indirection.. That wasn't something I thought as acceptable...

I guess we could rework skb_copy_datagram_iter to not call copy_page_to_iter and open-code kmapping so we can get away with a single code path? Unless I'm missing something?

Also, looking a bit closer there is a slight difference between the copy vs. the copy_and_csum variants. copy allows for a short_copy if we copy less than we expect while the csum faults it. I'm thinking that the copy_and_hash variant should also fault? Although I'm not sure I understand the fault entirely as csum is supposed to be cumulative, any insight?

maintainer
#21 lore raw

Re: [PATCH 10/11] nvmet-tcp: add NVMe over TCP target driver

From: Sagi Grimberg <[email protected]>
Date: Mon, 19 Nov 2018 15:14:44 -0800

Quoted 6 lines

Also, looking a bit closer there is a slight difference between the copy vs. the copy_and_csum variants. copy allows for a short_copy if we copy less than we expect while the csum faults it. I'm thinking that the copy_and_hash variant should also fault? Although I'm not sure I understand the fault entirely as csum is supposed to be cumulative, any insight?

When we are writing and signal an error, sockets have this recurring pattern where we return immediately the amount of bytes successfully transferred. Then on the next sendmsg() call we give the error.

I don't know if that is what is influencing the behavior here or not but it could be.

maintainer
#22 lore raw

Re: [PATCH 10/11] nvmet-tcp: add NVMe over TCP target driver

Quoted 8 lines
Quoted 7 lines

Also, looking a bit closer there is a slight difference between the copy vs. the copy_and_csum variants. copy allows for a short_copy if we copy less than we expect while the csum faults it. I'm thinking that the copy_and_hash variant should also fault? Although I'm not sure I understand the fault entirely as csum is supposed to be cumulative, any insight?

Quoted 6 lines

When we are writing and signal an error, sockets have this recurring pattern where we return immediately the amount of bytes successfully transferred. Then on the next sendmsg() call we give the error.

I don't know if that is what is influencing the behavior here or not but it could be.

That makes sense... Does recvmsg() have the same semantics? this is the rx path where we copy fragments to an iter..

If so, I guess that it makes sense that we fault and not allow short copies as we have an incomplete csum in this case...

I'm wandering how we can consolidate the code paths together with this subtle difference? Perhaps a short_copy flag?

maintainer
#23 lore raw

Re: [PATCH 10/11] nvmet-tcp: add NVMe over TCP target driver

From: Sagi Grimberg <[email protected]>
Date: Mon, 19 Nov 2018 15:24:13 -0800

Quoted 13 lines
Quoted 7 lines
Quoted 7 lines

Also, looking a bit closer there is a slight difference between the copy vs. the copy_and_csum variants. copy allows for a short_copy if we copy less than we expect while the csum faults it. I'm thinking that the copy_and_hash variant should also fault? Although I'm not sure I understand the fault entirely as csum is supposed to be cumulative, any insight?

Quoted 5 lines

When we are writing and signal an error, sockets have this recurring pattern where we return immediately the amount of bytes successfully transferred. Then on the next sendmsg() call we give the error. I don't know if that is what is influencing the behavior here or not but it could be.

Quoted 3 lines

That makes sense... Does recvmsg() have the same semantics? this is the
rx path where we copy fragments to an iter..

I just checked and TCP at the very least behaves this way on recvmsg() (report short length, then -EFAULT on next recvmsg() call).

It maintains a local variable "copied" which is increased every time skb_copy_datagram_msg() is successful. If in a subsequent iteration of the loop skb_copy_datagram_msg() returns -EFAULT, it will instead return 'copied' from recvmsg() if non-zero else the -EFAULT.

author
#24 lore raw

PATCH 11/11 nvme-tcp: add NVMe over TCP host driver

In reply to Sagi Grimberg

This patch implements the NVMe over TCP host driver. It can be used to connect to remote NVMe over Fabrics subsystems over good old TCP/IP.

The driver implements the TP 8000 of how nvme over fabrics capsules and data are encapsulated in nvme-tcp pdus and exchaged on top of a TCP byte stream. nvme-tcp header and data digest are supported as well.

To connect to all NVMe over Fabrics controllers reachable on a given taget port over TCP use the following command:

nvme connect-all -t tcp -a $IPADDR

This requires the latest version of nvme-cli with TCP support.

Signed-off-by Sagi Grimberg <sagi at lightbitslabs.com>
Signed-off-by Roy Shterman <roys at lightbitslabs.com>
Signed-off-by Solganik Alexander <sashas at lightbitslabs.com>
---
 drivers/nvme/host/Kconfig  |   15 +
 drivers/nvme/host/Makefile |    3 +
 drivers/nvme/host/tcp.c    | 2305 ++++++++++++++++++++++++++++++++++++
 3 files changed, 2323 insertions(+)
 create mode 100644 drivers/nvme/host/tcp.c
drivers/nvme/host/Kconfig +15 -0
index 88a8b5916624..0f345e207675 100644
@@ -57,3 +57,18 @@ config NVME_FC
 	  from https://github.com/linux-nvme/nvme-cli.
 
 	  If unsure, say N.
+
+config NVME_TCP
+	tristate "NVM Express over Fabrics TCP host driver"
+	depends on INET
+	depends on BLK_DEV_NVME
+	select NVME_FABRICS
+	help
+	  This provides support for the NVMe over Fabrics protocol using
+	  the TCP transport.  This allows you to use remote block devices
+	  exported using the NVMe protocol set.
+
+	  To configure a NVMe over Fabrics controller use the nvme-cli tool
+	  from https://github.com/linux-nvme/nvme-cli.
+
+	  If unsure, say N.
drivers/nvme/host/Makefile +3 -0
index aea459c65ae1..8a4b671c5f0c 100644
@@ -7,6 +7,7 @@ obj-$(CONFIG_BLK_DEV_NVME)		+= nvme.o
 obj-$(CONFIG_NVME_FABRICS)		+= nvme-fabrics.o
 obj-$(CONFIG_NVME_RDMA)			+= nvme-rdma.o
 obj-$(CONFIG_NVME_FC)			+= nvme-fc.o
+obj-$(CONFIG_NVME_TCP)			+= nvme-tcp.o
 
 nvme-core-y				:= core.o
 nvme-core-$(CONFIG_TRACING)		+= trace.o

@@ -21,3 +22,5 @@ nvme-fabrics-y				+= fabrics.o
 nvme-rdma-y				+= rdma.o
 
 nvme-fc-y				+= fc.o
+
+nvme-tcp-y				+= tcp.o
drivers/nvme/host/tcp.c new file +2305 -0

2,305 lines not on this page · raw message

Signature · 1 line
2.17.1
author
#25 lore raw

[PATCH nvme-cli 12/11] nvme: Add TCP transport

In reply to Sagi Grimberg
Signed-off-by Sagi Grimberg <sagi at lightbitslabs.com>
---
 fabrics.c    | 5 ++++-
 linux/nvme.h | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)
fabrics.c +4 -1
index aee566d5f68d..6fd9dfaa5d79 100644
@@ -96,6 +96,7 @@ static const char *arg_str(const char * const *strings,
 static const char * const trtypes[] = {
 	[NVMF_TRTYPE_RDMA]	= "rdma",
 	[NVMF_TRTYPE_FC]	= "fibre-channel",
+	[NVMF_TRTYPE_TCP]	= "tcp",
 	[NVMF_TRTYPE_LOOP]	= "loop",
 };
 

@@ -699,11 +700,13 @@ static int connect_ctrl(struct nvmf_disc_rsp_page_entry *e)
 		/* we can safely ignore the rest of the entries */
 		break;
 	case NVMF_TRTYPE_RDMA:
+	case NVMF_TRTYPE_TCP:
 		switch (e->adrfam) {
 		case NVMF_ADDR_FAMILY_IP4:
 		case NVMF_ADDR_FAMILY_IP6:
 			/* FALLTHRU */
-			len = sprintf(p, ",transport=rdma");
+			len = sprintf(p, ",transport=%s",
+				e->trtype == NVMF_TRTYPE_RDMA ? "rdma" : "tcp");
 			if (len < 0)
 				return -EINVAL;
 			p += len;
linux/nvme.h +1 -0
index a6a44b066267..7a600c791877 100644
@@ -52,6 +52,7 @@ enum {
 enum {
 	NVMF_TRTYPE_RDMA	= 1,	/* RDMA */
 	NVMF_TRTYPE_FC		= 2,	/* Fibre Channel */
+	NVMF_TRTYPE_TCP		= 3,	/* TCP */
 	NVMF_TRTYPE_LOOP	= 254,	/* Reserved for host usage */
 	NVMF_TRTYPE_MAX,
 };
Signature · 1 line
2.17.1
author
#26 lore raw

[PATCH nvme-cli 13/11] fabrics: add tcp port tsas decoding

In reply to Sagi Grimberg

tcp tsas include sectype indication for unsecured/tls ports.

Signed-off-by Sagi Grimberg <sagi at lightbitslabs.com>
---
 fabrics.c    | 14 ++++++++++++++
 linux/nvme.h | 10 ++++++++++
 2 files changed, 24 insertions(+)
fabrics.c +14 -0
index 6fd9dfaa5d79..774ea94dc0db 100644
@@ -141,6 +141,16 @@ static inline const char *treq_str(__u8 treq)
 	return arg_str(treqs, ARRAY_SIZE(treqs), treq);
 }
 
+static const char * const sectypes[] = {
+	[NVMF_TCP_SECTYPE_NONE]		= "none",
+	[NVMF_TCP_SECTYPE_TLS]		= "tls",
+};
+
+static inline const char *sectype_str(__u8 sectype)
+{
+	return arg_str(sectypes, ARRAY_SIZE(sectypes), sectype);
+}
+
 static const char * const prtypes[] = {
 	[NVMF_RDMA_PRTYPE_NOT_SPECIFIED]	= "not specified",
 	[NVMF_RDMA_PRTYPE_IB]			= "infiniband",

@@ -450,6 +460,10 @@ static void print_discovery_log(struct nvmf_disc_rsp_page_hdr *log, int numrec)
 			printf("rdma_pkey: 0x%04x\n",
 				e->tsas.rdma.pkey);
 			break;
+		case NVMF_TRTYPE_TCP:
+			printf("sectype: %s\n",
+				sectype_str(e->tsas.tcp.sectype));
+			break;
 		}
 	}
 }
linux/nvme.h +10 -0
index 7a600c791877..68000eb8c1dc 100644
@@ -91,6 +91,13 @@ enum {
 	NVMF_RDMA_CMS_RDMA_CM	= 1, /* Sockets based endpoint addressing */
 };
 
+/* TCP port security type for  Discovery Log Page entry TSAS
+ */
+enum {
+	NVMF_TCP_SECTYPE_NONE	= 0, /* No Security */
+	NVMF_TCP_SECTYPE_TLS	= 1, /* Transport Layer Security */
+};
+
 #define NVME_AQ_DEPTH		32
 #define NVME_NR_AEN_COMMANDS	1
 #define NVME_AQ_BLK_MQ_DEPTH	(NVME_AQ_DEPTH - NVME_NR_AEN_COMMANDS)

@@ -1184,6 +1191,9 @@ struct nvmf_disc_rsp_page_entry {
 			__u16	pkey;
 			__u8	resv10[246];
 		} rdma;
+		struct tcp {
+			__u8	sectype;
+		} tcp;
 	} tsas;
 };
Signature · 1 line
2.17.1
author
#27 lore raw

[PATCH nvme-cli 14/11] fabrics: add transport header and data digest

In reply to Sagi Grimberg

This setting is enabling header and data digest over NVMe/TCP controllers.

Signed-off-by Sagi Grimberg <sagi at lightbitslabs.com>
---
 fabrics.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)
fabrics.c +23 -1
index 774ea94dc0db..2890e4ef36c8 100644
@@ -62,6 +62,8 @@ static struct config {
 	int  duplicate_connect;
 	int  disable_sqflow;
 	bool persistent;
+	int  hdr_digest;
+	int  data_digest;
 } cfg = { NULL };
 
 #define BUF_SIZE		4096

@@ -629,7 +631,9 @@ static int build_options(char *argstr, int max_len)
 	    add_bool_argument(&argstr, &max_len, "duplicate_connect",
 				cfg.duplicate_connect) ||
 	    add_bool_argument(&argstr, &max_len, "disable_sqflow",
-				cfg.disable_sqflow))
+				cfg.disable_sqflow) ||
+	    add_bool_argument(&argstr, &max_len, "hdr_digest", cfg.hdr_digest) ||
+	    add_bool_argument(&argstr, &max_len, "data_digest", cfg.data_digest))
 		return -EINVAL;
 
 	return 0;

@@ -706,6 +710,20 @@ static int connect_ctrl(struct nvmf_disc_rsp_page_entry *e)
 		p += len;
 	}
 
+	if (cfg.hdr_digest) {
+		len = sprintf(p, ",hdr_digest");
+		if (len < 0)
+			return -EINVAL;
+		p += len;
+	}
+
+	if (cfg.data_digest) {
+		len = sprintf(p, ",data_digest");
+		if (len < 0)
+			return -EINVAL;
+		p += len;
+	}
+
 	switch (e->trtype) {
 	case NVMF_TRTYPE_LOOP: /* loop */
 		len = sprintf(p, ",transport=loop");

@@ -947,6 +965,8 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		{"reconnect-delay", 'c', "LIST", CFG_INT, &cfg.reconnect_delay, required_argument, "reconnect timeout period in seconds" },
 		{"ctrl-loss-tmo",   'l', "LIST", CFG_INT, &cfg.ctrl_loss_tmo,   required_argument, "controller loss timeout period in seconds" },
 		{"persistent",  'p', "LIST", CFG_NONE, &cfg.persistent,  no_argument, "persistent discovery connection" },
+		{"hdr_digest", 'g', "", CFG_NONE, &cfg.hdr_digest, no_argument, "enable transport protocol header digest (TCP transport)" },
+		{"data_digest", 'G', "", CFG_NONE, &cfg.data_digest, no_argument, "enable transport protocol data digest (TCP transport)" },
 		{NULL},
 	};
 

@@ -990,6 +1010,8 @@ int connect(const char *desc, int argc, char **argv)
 		{"ctrl-loss-tmo",   'l', "LIST", CFG_INT, &cfg.ctrl_loss_tmo,   required_argument, "controller loss timeout period in seconds" },
 		{"duplicate_connect", 'D', "", CFG_NONE, &cfg.duplicate_connect, no_argument, "allow duplicate connections between same transport host and subsystem port" },
 		{"disable_sqflow", 'd', "", CFG_NONE, &cfg.disable_sqflow, no_argument, "disable controller sq flow control (default false)" },
+		{"hdr_digest", 'g', "", CFG_NONE, &cfg.hdr_digest, no_argument, "enable transport protocol header digest (TCP transport)" },
+		{"data_digest", 'G', "", CFG_NONE, &cfg.data_digest, no_argument, "enable transport protocol data digest (TCP transport)" },
 		{NULL},
 	};
Signature · 1 line
2.17.1