Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1702434 > unrolled thread

[[PATCH v1] 00/37] Implement SMBD protocol: Series 1

Back to article view | Back to linux.kernel

Contents

  [[PATCH v1] 00/37] Implement SMBD protocol: Series 1 Long Li <longli@exchange.microsoft.com> - 2017-08-02 22:20 +0200
    [[PATCH v1] 19/37] [CIFS] SMBD: Manage credits on SMBD client and server Long Li <longli@exchange.microsoft.com> - 2017-08-02 22:20 +0200
      RE: [[PATCH v1] 19/37] [CIFS] SMBD: Manage credits on SMBD client and  server Tom Talpey <ttalpey@microsoft.com> - 2017-08-14 22:50 +0200
        RE: [[PATCH v1] 19/37] [CIFS] SMBD: Manage credits on SMBD client and  server Long Li <longli@microsoft.com> - 2017-08-15 01:10 +0200
    [[PATCH v1] 08/37] [CIFS] SMBD: Define packet format for SMBD data transfer message Long Li <longli@exchange.microsoft.com> - 2017-08-02 22:30 +0200
      Re: [[PATCH v1] 08/37] [CIFS] SMBD: Define packet format for SMBD  data transfer message Christoph Hellwig <hch@infradead.org> - 2017-08-13 12:20 +0200
        Re: [[PATCH v1] 08/37] [CIFS] SMBD: Define packet format for SMBD  data transfer message Jeff Layton <jlayton@redhat.com> - 2017-08-14 12:30 +0200
    [[PATCH v1] 11/37] [CIFS] SMBD: Post a receive request Long Li <longli@exchange.microsoft.com> - 2017-08-02 22:30 +0200
      Re: [[PATCH v1] 11/37] [CIFS] SMBD: Post a receive request Christoph Hellwig <hch@infradead.org> - 2017-08-13 12:20 +0200
    [[PATCH v1] 15/37] [CIFS] SMBD: Post a SMBD data transfer message with data payload Long Li <longli@exchange.microsoft.com> - 2017-08-02 22:30 +0200
      Re: [[PATCH v1] 15/37] [CIFS] SMBD: Post a SMBD data transfer  message with data payload Christoph Hellwig <hch@infradead.org> - 2017-08-13 12:30 +0200
      RE: [[PATCH v1] 15/37] [CIFS] SMBD: Post a SMBD data transfer message  with data payload Tom Talpey <ttalpey@microsoft.com> - 2017-08-14 22:30 +0200
    [[PATCH v1] 04/37] [CIFS] SMBD: Define per-channel SMBD transport parameters and default values Long Li <longli@exchange.microsoft.com> - 2017-08-02 22:30 +0200
      Re: [[PATCH v1] 04/37] [CIFS] SMBD: Define per-channel SMBD  transport parameters and default values Christoph Hellwig <hch@infradead.org> - 2017-08-13 12:20 +0200
        RE: [[PATCH v1] 04/37] [CIFS] SMBD: Define per-channel SMBD transport  parameters and default values Tom Talpey <ttalpey@microsoft.com> - 2017-08-14 21:30 +0200
          RE: [[PATCH v1] 04/37] [CIFS] SMBD: Define per-channel SMBD transport  parameters and default values Long Li <longli@microsoft.com> - 2017-08-15 01:00 +0200
    [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD transport Long Li <longli@exchange.microsoft.com> - 2017-08-02 22:30 +0200
      Re: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD transport Stefan Metzmacher <metze@samba.org> - 2017-08-08 09:10 +0200
        RE: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD transport Long Li <longli@microsoft.com> - 2017-08-12 10:40 +0200
          Re: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD transport Christoph Hellwig <hch@infradead.org> - 2017-08-12 21:00 +0200
          Re: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD transport Stefan Metzmacher <metze@samba.org> - 2017-08-14 16:20 +0200
            RE: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD transport Long Li <longli@microsoft.com> - 2017-08-14 20:20 +0200
      Re: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD transport Christoph Hellwig <hch@infradead.org> - 2017-08-13 12:20 +0200
    [[PATCH v1] 18/37] [CIFS] SMBD: Implement API for upper layer to send data Long Li <longli@exchange.microsoft.com> - 2017-08-02 22:30 +0200
      RE: [[PATCH v1] 18/37] [CIFS] SMBD: Implement API for upper layer to  send data Tom Talpey <ttalpey@microsoft.com> - 2017-08-14 22:50 +0200
        RE: [[PATCH v1] 18/37] [CIFS] SMBD: Implement API for upper layer to  send data Long Li <longli@microsoft.com> - 2017-08-20 01:50 +0200
    [[PATCH v1] 12/37] [CIFS] SMBD: Handle send completion from CQ Long Li <longli@exchange.microsoft.com> - 2017-08-02 22:30 +0200
      Re: [[PATCH v1] 12/37] [CIFS] SMBD: Handle send completion from CQ Christoph Hellwig <hch@infradead.org> - 2017-08-13 12:20 +0200
        RE: [[PATCH v1] 12/37] [CIFS] SMBD: Handle send completion from CQ Long Li <longli@microsoft.com> - 2017-08-14 20:20 +0200
    [[PATCH v1] 09/37] [CIFS] SMBD: Add SMBD request and cache Long Li <longli@exchange.microsoft.com> - 2017-08-02 22:30 +0200
    [[PATCH v1] 05/37] [CIFS] SMBD: Implement API for upper layer to create SMBD transport and establish RDMA connection Long Li <longli@exchange.microsoft.com> - 2017-08-02 22:30 +0200
      RE: [[PATCH v1] 05/37] [CIFS] SMBD: Implement API for upper layer to  create SMBD transport and establish RDMA connection Tom Talpey <ttalpey@microsoft.com> - 2017-08-14 22:00 +0200
    [[PATCH v1] 17/37] [CIFS] SMBD: Track status for transport Long Li <longli@exchange.microsoft.com> - 2017-08-02 22:30 +0200
    Re: [[PATCH v1] 00/37] Implement SMBD protocol: Series 1 Christoph Hellwig <hch@infradead.org> - 2017-08-13 12:30 +0200
      Re: [[PATCH v1] 00/37] Implement SMBD protocol: Series 1 Christoph Hellwig <hch@infradead.org> - 2017-08-13 12:40 +0200
      RE: [[PATCH v1] 00/37] Implement SMBD protocol: Series 1 Long Li <longli@microsoft.com> - 2017-08-14 19:10 +0200

Page 1 of 2  [1] 2  Next page →

#1702434 — [[PATCH v1] 00/37] Implement SMBD protocol: Series 1

From: Long Li <longli@microsoft.com>

SMB3 defines a protocol for transfer data over RDMA transport such as Infiniband, RoCE and iWARP. The prococol is published in [MS-SMBD] (https://msdn.microsoft.com/en-us/library/hh536346.aspx).

This is the series 1 of two patch sets. This patch set implements the SMBD transport for doing RDMA send/recv.

This patch set is the foundation of series 2 patch set, which implements sending upper layer RDMA read/write via memory registration.

Long Li (37):
  [CIFS] SMBD: Add parsing for new rdma mount option
  [CIFS] SMBD: Add structure for SMBD transport
  [CIFS] SMBD: Add logging functions for debug
  [CIFS] SMBD: Define per-channel SMBD transport parameters and default
    values
  [CIFS] SMBD: Implement API for upper layer to create SMBD transport
    and establish RDMA connection
  [CIFS] SMBD: Add definition and cache for SMBD response
  [CIFS] SMBD: Implement receive buffer for handling SMBD response
  [CIFS] SMBD: Define packet format for SMBD data transfer message
  [CIFS] SMBD: Add SMBD request and cache
  [CIFS] SMBD: Introduce wait queue when sending SMBD request
  [CIFS] SMBD: Post a receive request
  [CIFS] SMBD: Handle send completion from CQ
  [CIFS] SMBD: Implement SMBD protocol negotiation
  [CIFS] SMBD: Post a SMBD data transfer message with page payload
  [CIFS] SMBD: Post a SMBD data transfer message with data payload
  [CIFS] SMBD: Post a SMBD message with no payload
  [CIFS] SMBD: Track status for transport
  [CIFS] SMBD: Implement API for upper layer to send data
  [CIFS] SMBD: Manage credits on SMBD client and server
  [CIFS] SMBD: Implement reassembly queue for receiving data
  [CIFS] SMBD: Implement API for upper layer to receive data
  [CIFS] SMBD: Implement API for upper layer to receive data to page
  [CIFS] SMBD: Implement API for upper layer to reconnect transport
  [CIFS] SMBD: Support for SMBD keep alive protocol
  [CIFS] SMBD: Support SMBD idle connection timer
  [CIFS] SMBD: Send an immediate packet when it's needed
  [CIFS] SMBD: Destroy transport when RDMA channel is disconnected
  [CIFS] SMBD: Implement API for upper layer to destroy the transport
  [CIFS] SMBD: Disconnect RDMA connection on QP errors
  [CIFS] SMBD: Add SMBDirect transport to Makefile
  [CIFS] Add SMBD transport to SMB session context
  [CIFS] Add SMBD debug couters to CIFS debug exports
  [CIFS] Connect to SMBD transport when specified in mount option
  [CIFS] Reconnect to SMBD transport when it's used
  [CIFS] Destroy SMBD transport on exit
  [CIFS] Read from SMBD transport when it's used
  [CIFS] Write to SMBD transport when it's used

 fs/cifs/Makefile     |    2 +-
 fs/cifs/cifs_debug.c |   25 +
 fs/cifs/cifsfs.c     |    2 +
 fs/cifs/cifsglob.h   |    3 +
 fs/cifs/cifsrdma.c   | 1833 ++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/cifs/cifsrdma.h   |  243 +++++++
 fs/cifs/connect.c    |   56 +-
 fs/cifs/transport.c  |    7 +
 8 files changed, 2164 insertions(+), 7 deletions(-)
 create mode 100644 fs/cifs/cifsrdma.c
 create mode 100644 fs/cifs/cifsrdma.h

-- 
2.7.4

[toc] | [next] | [standalone]

#1702435 — [[PATCH v1] 19/37] [CIFS] SMBD: Manage credits on SMBD client and server

From: Long Li <longli@microsoft.com>

SMB client and server maintain a credit system on the SMBD transport. Credits are used to tell when the client or server can send a packet to the peer, based on current memory or resource usage.

Signed-off-by: Long Li <longli@microsoft.com>
---
 fs/cifs/cifsrdma.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/fs/cifs/cifsrdma.c b/fs/cifs/cifsrdma.c
index eb48651..97cde3f 100644
--- a/fs/cifs/cifsrdma.c
+++ b/fs/cifs/cifsrdma.c
@@ -575,6 +575,46 @@ static int cifs_rdma_post_send_negotiate_req(struct cifs_rdma_info *info)
 }
 
 /*
+ * Extend the credits to remote peer
+ * This implements [MS-SMBD] 3.1.5.9
+ * The idea is that we should extend credits to remote peer as quickly as
+ * it's allowed, to maintain data flow. We allocate as much as receive
+ * buffer as possible, and extend the receive credits to remote peer
+ * return value: the new credtis being granted.
+ */
+static int manage_credits_prior_sending(struct cifs_rdma_info *info)
+{
+	int ret = 0;
+	struct cifs_rdma_response *response;
+	int rc;
+
+	if (atomic_read(&info->receive_credit_target) >
+	    atomic_read(&info->receive_credits)) {
+		while (true) {
+			response = get_receive_buffer(info);
+			if (!response)
+				break;
+
+			response->type = SMBD_TRANSFER_DATA;
+			response->first_segment = false;
+			rc = cifs_rdma_post_recv(info, response);
+			if (rc) {
+				log_rdma_recv("post_recv failed rc=%d\n", rc);
+				put_receive_buffer(info, response);
+				break;
+			}
+
+			ret++;
+		}
+	}
+
+	atomic_add(ret, &info->receive_credits);
+	log_transport_credit(info);
+
+	return ret;
+}
+
+/*
  * Send a page
  * page: the page to send
  * offset: offset in the page to send
@@ -607,6 +647,8 @@ static int cifs_rdma_post_send_page(struct cifs_rdma_info *info, struct page *pa
 
 	packet = (struct smbd_data_transfer *) request->packet;
 	packet->credits_requested = cpu_to_le16(info->send_credit_target);
+	packet->credits_granted =
+		cpu_to_le16(manage_credits_prior_sending(info));
 	packet->flags = cpu_to_le16(0);
 
 	packet->reserved = cpu_to_le16(0);
@@ -718,6 +760,8 @@ static int cifs_rdma_post_send_empty(struct cifs_rdma_info *info)
 	request->info = info;
 	packet = (struct smbd_data_transfer_no_data *) request->packet;
 
+	credits_granted = manage_credits_prior_sending(info);
+
 	/* nothing to do? */
 	if (credits_granted==0 && flags==0) {
 		mempool_free(request, info->request_mempool);
@@ -827,6 +871,7 @@ static int cifs_rdma_post_send_data(
 
 	packet = (struct smbd_data_transfer *) request->packet;
 	packet->credits_requested = cpu_to_le16(info->send_credit_target);
+	packet->credits_granted = cpu_to_le16(manage_credits_prior_sending(info));
 	packet->flags = cpu_to_le16(0);
 	packet->reserved = cpu_to_le16(0);
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1711432 — RE: [[PATCH v1] 19/37] [CIFS] SMBD: Manage credits on SMBD client and server

> -----Original Message-----
> From: linux-cifs-owner@vger.kernel.org [mailto:linux-cifs-
> owner@vger.kernel.org] On Behalf Of Long Li
> Sent: Wednesday, August 2, 2017 4:11 PM
> To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org; samba-
> technical@lists.samba.org; linux-kernel@vger.kernel.org
> Cc: Long Li <longli@microsoft.com>
> Subject: [[PATCH v1] 19/37] [CIFS] SMBD: Manage credits on SMBD client and
> server
> 
>  /*
> + * Extend the credits to remote peer
> + * This implements [MS-SMBD] 3.1.5.9
> + * The idea is that we should extend credits to remote peer as quickly as
> + * it's allowed, to maintain data flow. We allocate as much as receive
> + * buffer as possible, and extend the receive credits to remote peer
> + * return value: the new credtis being granted.
> + */
> +static int manage_credits_prior_sending(struct cifs_rdma_info *info)
> +{
> +       int ret = 0;
> +       struct cifs_rdma_response *response;
> +       int rc;
> +
> +       if (atomic_read(&info->receive_credit_target) >

When does the receive_credit_target value change? It seems wasteful to
perform an atomic_read() on this local value each time.

Tom.

[toc] | [prev] | [next] | [standalone]

#1711552 — RE: [[PATCH v1] 19/37] [CIFS] SMBD: Manage credits on SMBD client and server

> -----Original Message-----
> From: Tom Talpey
> Sent: Monday, August 14, 2017 1:47 PM
> To: Long Li <longli@microsoft.com>; Steve French <sfrench@samba.org>;
> linux-cifs@vger.kernel.org; samba-technical@lists.samba.org; linux-
> kernel@vger.kernel.org
> Subject: RE: [[PATCH v1] 19/37] [CIFS] SMBD: Manage credits on SMBD client
> and server
> 
> > -----Original Message-----
> > From: linux-cifs-owner@vger.kernel.org [mailto:linux-cifs-
> > owner@vger.kernel.org] On Behalf Of Long Li
> > Sent: Wednesday, August 2, 2017 4:11 PM
> > To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org;
> > samba- technical@lists.samba.org; linux-kernel@vger.kernel.org
> > Cc: Long Li <longli@microsoft.com>
> > Subject: [[PATCH v1] 19/37] [CIFS] SMBD: Manage credits on SMBD client
> > and server
> >
> >  /*
> > + * Extend the credits to remote peer
> > + * This implements [MS-SMBD] 3.1.5.9
> > + * The idea is that we should extend credits to remote peer as
> > +quickly as
> > + * it's allowed, to maintain data flow. We allocate as much as
> > +receive
> > + * buffer as possible, and extend the receive credits to remote peer
> > + * return value: the new credtis being granted.
> > + */
> > +static int manage_credits_prior_sending(struct cifs_rdma_info *info)
> > +{
> > +       int ret = 0;
> > +       struct cifs_rdma_response *response;
> > +       int rc;
> > +
> > +       if (atomic_read(&info->receive_credit_target) >
> 
> When does the receive_credit_target value change? It seems wasteful to
> perform an atomic_read() on this local value each time.

It could be potentially changed while receiving a SMBD packet, as specified in MS-SMBD 3.1.5.8.

I agree with you there is no need to use atomic since this value is not increased or decreased, just being set. Will change it.

> 
> Tom.

[toc] | [prev] | [next] | [standalone]

#1702438 — [[PATCH v1] 08/37] [CIFS] SMBD: Define packet format for SMBD data transfer message

From: Long Li <longli@microsoft.com>

Define the packet format for a SMBD data packet with payload

Signed-off-by: Long Li <longli@microsoft.com>
---
 fs/cifs/cifsrdma.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/fs/cifs/cifsrdma.h b/fs/cifs/cifsrdma.h
index 78ce2bf..ed0ff54 100644
--- a/fs/cifs/cifsrdma.h
+++ b/fs/cifs/cifsrdma.h
@@ -78,6 +78,21 @@ enum smbd_message_type {
 	SMBD_TRANSFER_DATA,
 };
 
+#define SMB_DIRECT_RESPONSE_REQUESTED 0x0001
+
+// SMBD data transfer packet with payload [MS-SMBD] 2.2.3
+struct smbd_data_transfer {
+	__le16 credits_requested;
+	__le16 credits_granted;
+	__le16 flags;
+	__le16 reserved;
+	__le32 remaining_data_length;
+	__le32 data_offset;
+	__le32 data_length;
+	__le32 padding;
+	char buffer[0];
+} __packed;
+
 // The context for a SMBD response
 struct cifs_rdma_response {
 	struct cifs_rdma_info *info;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1710467 — Re: [[PATCH v1] 08/37] [CIFS] SMBD: Define packet format for SMBD data transfer message

> +// SMBD data transfer packet with payload [MS-SMBD] 2.2.3
> +struct smbd_data_transfer {
> +	__le16 credits_requested;
> +	__le16 credits_granted;
> +	__le16 flags;
> +	__le16 reserved;
> +	__le32 remaining_data_length;
> +	__le32 data_offset;
> +	__le32 data_length;
> +	__le32 padding;
> +	char buffer[0];

Please use the actually standardized [] syntax for variable sized
arrays.  Also normally this would be a __u8 to fit with the other
types, but I haven't seen the usage yet.

> +} __packed;

The structure is natually packed already, no need to add the
attribute.

[toc] | [prev] | [next] | [standalone]

#1710799 — Re: [[PATCH v1] 08/37] [CIFS] SMBD: Define packet format for SMBD data transfer message

On Sun, 2017-08-13 at 03:15 -0700, Christoph Hellwig wrote:
> > +// SMBD data transfer packet with payload [MS-SMBD] 2.2.3
> > +struct smbd_data_transfer {
> > +	__le16 credits_requested;
> > +	__le16 credits_granted;
> > +	__le16 flags;
> > +	__le16 reserved;
> > +	__le32 remaining_data_length;
> > +	__le32 data_offset;
> > +	__le32 data_length;
> > +	__le32 padding;
> > +	char buffer[0];
> 
> Please use the actually standardized [] syntax for variable sized
> arrays.  Also normally this would be a __u8 to fit with the other
> types, but I haven't seen the usage yet.
> 

Yes, having a single-element array makes it harder to handle the
indexes, etc. Flexible arrays are better.
 
> > +} __packed;
> 
> The structure is natually packed already, no need to add the
> attribute.

I think this should remain on structs that are intended to go across the
wire. Could we ever end up with some exotic arch that stuffs some
padding in there? Maybe I'm just paranoid, but I don't see any harm in
leaving that here.

-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1702439 — [[PATCH v1] 11/37] [CIFS] SMBD: Post a receive request

From: Long Li <longli@microsoft.com>

Add code to post a receive request to RDMA. Before the SMB server can send a packet to SMB client via SMBD, a receive request must be posted to local RDMA layer.

Signed-off-by: Long Li <longli@microsoft.com>
---
 fs/cifs/cifsrdma.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/cifs/cifsrdma.h |   5 +++
 2 files changed, 129 insertions(+)

diff --git a/fs/cifs/cifsrdma.c b/fs/cifs/cifsrdma.c
index 8aa8a47..20237b7 100644
--- a/fs/cifs/cifsrdma.c
+++ b/fs/cifs/cifsrdma.c
@@ -62,6 +62,10 @@ static void put_receive_buffer(
 static int allocate_receive_buffers(struct cifs_rdma_info *info, int num_buf);
 static void destroy_receive_buffers(struct cifs_rdma_info *info);
 
+static int cifs_rdma_post_recv(
+		struct cifs_rdma_info *info,
+		struct cifs_rdma_response *response);
+
 /*
  * Per RDMA transport connection parameters
  * as defined in [MS-SMBD] 3.1.1.1
@@ -193,6 +197,85 @@ cifs_rdma_qp_async_error_upcall(struct ib_event *event, void *context)
 	}
 }
 
+/* Called from softirq, when recv is done */
+static void recv_done(struct ib_cq *cq, struct ib_wc *wc)
+{
+	struct smbd_data_transfer *data_transfer;
+	struct cifs_rdma_response *response =
+		container_of(wc->wr_cqe, struct cifs_rdma_response, cqe);
+	struct cifs_rdma_info *info = response->info;
+
+	log_rdma_recv("response=%p type=%d wc status=%d wc opcode %d "
+		      "byte_len=%d pkey_index=%x\n",
+		response, response->type, wc->status, wc->opcode,
+		wc->byte_len, wc->pkey_index);
+
+	if (wc->status != IB_WC_SUCCESS || wc->opcode != IB_WC_RECV) {
+		log_rdma_recv("wc->status=%d opcode=%d\n",
+			wc->status, wc->opcode);
+		goto error;
+	}
+
+	ib_dma_sync_single_for_cpu(
+		wc->qp->device,
+		response->sge.addr,
+		response->sge.length,
+		DMA_FROM_DEVICE);
+
+	switch(response->type) {
+	case SMBD_TRANSFER_DATA:
+		data_transfer = (struct smbd_data_transfer *) response->packet;
+		atomic_dec(&info->receive_credits);
+		atomic_set(&info->receive_credit_target,
+			le16_to_cpu(data_transfer->credits_requested));
+		atomic_add(le16_to_cpu(data_transfer->credits_granted),
+			&info->send_credits);
+
+		log_incoming("data flags %d data_offset %d data_length %d "
+			     "remaining_data_length %d\n",
+			le16_to_cpu(data_transfer->flags),
+			le32_to_cpu(data_transfer->data_offset),
+			le32_to_cpu(data_transfer->data_length),
+			le32_to_cpu(data_transfer->remaining_data_length));
+
+		log_transport_credit(info);
+
+		// process sending queue on new credits
+		if (atomic_read(&info->send_credits))
+			wake_up(&info->wait_send_queue);
+
+		// process receive queue
+		if (le32_to_cpu(data_transfer->data_length)) {
+			if (info->full_packet_received) {
+				response->first_segment = true;
+			}
+
+			if (le32_to_cpu(data_transfer->remaining_data_length))
+				info->full_packet_received = false;
+			else
+				info->full_packet_received = true;
+
+			goto queue_done;
+		}
+
+		// if we reach here, this is an empty packet, finish it
+		break;
+
+	default:
+		log_rdma_recv("unexpected response type=%d\n", response->type);
+	}
+
+error:
+	put_receive_buffer(info, response);
+
+queue_done:
+	if (atomic_dec_and_test(&info->recv_pending)) {
+		wake_up(&info->wait_recv_pending);
+	}
+
+	return;
+}
+
 static struct rdma_cm_id* cifs_rdma_create_id(
 		struct cifs_rdma_info *info, struct sockaddr *dstaddr)
 {
@@ -289,6 +372,44 @@ static int cifs_rdma_ia_open(
 }
 
 /*
+ * Post a receive request to the transport
+ * The remote peer can only send data when a receive is posted
+ * The interaction is controlled by send/recieve credit system
+ */
+static int cifs_rdma_post_recv(struct cifs_rdma_info *info, struct cifs_rdma_response *response)
+{
+	struct ib_recv_wr recv_wr, *recv_wr_fail=NULL;
+	int rc = -EIO;
+
+	response->sge.addr = ib_dma_map_single(info->id->device, response->packet,
+				info->max_receive_size, DMA_FROM_DEVICE);
+	if (ib_dma_mapping_error(info->id->device, response->sge.addr))
+		return rc;
+
+	response->sge.length = info->max_receive_size;
+	response->sge.lkey = info->pd->local_dma_lkey;
+
+	response->cqe.done = recv_done;
+
+	recv_wr.wr_cqe = &response->cqe;
+	recv_wr.next = NULL;
+	recv_wr.sg_list = &response->sge;
+	recv_wr.num_sge = 1;
+
+	atomic_inc(&info->recv_pending);
+	rc = ib_post_recv(info->id->qp, &recv_wr, &recv_wr_fail);
+	if (rc) {
+		ib_dma_unmap_single(info->id->device, response->sge.addr,
+				    response->sge.length, DMA_FROM_DEVICE);
+
+		log_rdma_recv("ib_post_recv failed rc=%d\n", rc);
+		atomic_dec(&info->recv_pending);
+	}
+
+	return rc;
+}
+
+/*
  * Receive buffer operations.
  * For each remote send, we need to post a receive. The receive buffers are
  * pre-allocated in advance.
@@ -485,6 +606,9 @@ struct cifs_rdma_info* cifs_create_rdma_session(
 
 	allocate_receive_buffers(info, info->receive_credit_max);
 	init_waitqueue_head(&info->wait_send_queue);
+
+	init_waitqueue_head(&info->wait_recv_pending);
+	atomic_set(&info->recv_pending, 0);
 out2:
 	rdma_destroy_id(info->id);
 
diff --git a/fs/cifs/cifsrdma.h b/fs/cifs/cifsrdma.h
index 287b5b1..8702a2b 100644
--- a/fs/cifs/cifsrdma.h
+++ b/fs/cifs/cifsrdma.h
@@ -59,6 +59,9 @@ struct cifs_rdma_info {
 	atomic_t receive_credits;
 	atomic_t receive_credit_target;
 
+	atomic_t recv_pending;
+	wait_queue_head_t wait_recv_pending;
+
 	struct list_head receive_queue;
 	spinlock_t receive_queue_lock;
 
@@ -68,6 +71,8 @@ struct cifs_rdma_info {
 	struct kmem_cache *request_cache;
 	mempool_t *request_mempool;
 
+	bool full_packet_received;
+
 	// response pool for RDMA receive
 	struct kmem_cache *response_cache;
 	mempool_t *response_mempool;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1710471 — Re: [[PATCH v1] 11/37] [CIFS] SMBD: Post a receive request

> +	switch(response->type) {
> +	case SMBD_TRANSFER_DATA:
> +		data_transfer = (struct smbd_data_transfer *) response->packet;

Maybe add a little helper for the packet data to hide these cast, e.g.

static inline void *smbd_payload(struct cifs_rdma_response *resp)
{
	return (void *)response->packet;
}


> +		atomic_dec(&info->receive_credits);
> +		atomic_set(&info->receive_credit_target,
> +			le16_to_cpu(data_transfer->credits_requested));
> +		atomic_add(le16_to_cpu(data_transfer->credits_granted),
> +			&info->send_credits);

That's a lot of atomic ops in the fast path handler.  Also remember
that atomic_set isn't really atomic vs other callers.

[toc] | [prev] | [next] | [standalone]

#1702440 — [[PATCH v1] 15/37] [CIFS] SMBD: Post a SMBD data transfer message with data payload

From: Long Li <longli@microsoft.com>

Similar to sending transfer message with page payload, this function creates a SMBD data packet and send it over to RDMA, from iov passed from upper layer.

Signed-off-by: Long Li <longli@microsoft.com>
---
 fs/cifs/cifsrdma.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 119 insertions(+)

diff --git a/fs/cifs/cifsrdma.c b/fs/cifs/cifsrdma.c
index b3ec109..989cad8 100644
--- a/fs/cifs/cifsrdma.c
+++ b/fs/cifs/cifsrdma.c
@@ -66,6 +66,9 @@ static int cifs_rdma_post_recv(
 		struct cifs_rdma_info *info,
 		struct cifs_rdma_response *response);
 
+static int cifs_rdma_post_send_data(
+		struct cifs_rdma_info *info,
+		struct kvec *iov, int n_vec, int remaining_data_length);
 static int cifs_rdma_post_send_page(struct cifs_rdma_info *info,
 		struct page *page, unsigned long offset,
 		size_t size, int remaining_data_length);
@@ -671,6 +674,122 @@ static int cifs_rdma_post_send_page(struct cifs_rdma_info *info, struct page *pa
 }
 
 /*
+ * Send a data buffer
+ * iov: the iov array describing the data buffers
+ * n_vec: number of iov array
+ * remaining_data_length: remaining data to send in this payload
+ */
+static int cifs_rdma_post_send_data(
+	struct cifs_rdma_info *info, struct kvec *iov, int n_vec,
+	int remaining_data_length)
+{
+	struct cifs_rdma_request *request;
+	struct smbd_data_transfer *packet;
+	struct ib_send_wr send_wr, *send_wr_fail;
+	int rc = -ENOMEM, i;
+	u32 data_length;
+
+	request = mempool_alloc(info->request_mempool, GFP_KERNEL);
+	if (!request)
+		return rc;
+
+	request->info = info;
+
+	wait_event(info->wait_send_queue, atomic_read(&info->send_credits) > 0);
+	atomic_dec(&info->send_credits);
+
+	packet = (struct smbd_data_transfer *) request->packet;
+	packet->credits_requested = cpu_to_le16(info->send_credit_target);
+	packet->flags = cpu_to_le16(0);
+	packet->reserved = cpu_to_le16(0);
+
+	packet->data_offset = cpu_to_le32(24);
+
+	data_length = 0;
+	for (i=0; i<n_vec; i++)
+		data_length += iov[i].iov_len;
+	packet->data_length = cpu_to_le32(data_length);
+
+	packet->remaining_data_length = cpu_to_le32(remaining_data_length);
+	packet->padding = cpu_to_le32(0);
+
+	log_rdma_send("credits_requested=%d credits_granted=%d data_offset=%d "
+		      "data_length=%d remaining_data_length=%d\n",
+		le16_to_cpu(packet->credits_requested),
+		le16_to_cpu(packet->credits_granted),
+		le32_to_cpu(packet->data_offset),
+		le32_to_cpu(packet->data_length),
+		le32_to_cpu(packet->remaining_data_length));
+
+	request->sge = kzalloc(sizeof(struct ib_sge)*(n_vec+1), GFP_KERNEL);
+	if (!request->sge)
+		goto allocate_sge_failed;
+
+	request->num_sge = n_vec+1;
+
+	request->sge[0].addr = ib_dma_map_single(
+				info->id->device, (void *)packet,
+				sizeof(*packet), DMA_BIDIRECTIONAL);
+	if(ib_dma_mapping_error(info->id->device, request->sge[0].addr)) {
+		rc = -EIO;
+		goto dma_mapping_failure;
+	}
+	request->sge[0].length = sizeof(*packet);
+	request->sge[0].lkey = info->pd->local_dma_lkey;
+	ib_dma_sync_single_for_device(info->id->device, request->sge[0].addr,
+		request->sge[0].length, DMA_TO_DEVICE);
+
+	for (i=0; i<n_vec; i++) {
+		request->sge[i+1].addr = ib_dma_map_single(info->id->device, iov[i].iov_base,
+						iov[i].iov_len, DMA_BIDIRECTIONAL);
+		if(ib_dma_mapping_error(info->id->device, request->sge[i+1].addr)) {
+			rc = -EIO;
+			goto dma_mapping_failure;
+		}
+		request->sge[i+1].length = iov[i].iov_len;
+		request->sge[i+1].lkey = info->pd->local_dma_lkey;
+		ib_dma_sync_single_for_device(info->id->device, request->sge[i+i].addr,
+			request->sge[i+i].length, DMA_TO_DEVICE);
+	}
+
+	log_rdma_send("rdma_request sge[0] addr=%llu legnth=%u lkey=%u\n",
+		request->sge[0].addr, request->sge[0].length, request->sge[0].lkey);
+	for (i=0; i<n_vec; i++)
+		log_rdma_send("rdma_request sge[%d] addr=%llu legnth=%u lkey=%u\n",
+			i+1, request->sge[i+1].addr,
+			request->sge[i+1].length, request->sge[i+1].lkey);
+
+	request->cqe.done = send_done;
+
+	send_wr.next = NULL;
+	send_wr.wr_cqe = &request->cqe;
+	send_wr.sg_list = request->sge;
+	send_wr.num_sge = request->num_sge;
+	send_wr.opcode = IB_WR_SEND;
+	send_wr.send_flags = IB_SEND_SIGNALED;
+
+	rc = ib_post_send(info->id->qp, &send_wr, &send_wr_fail);
+	if (!rc)
+		return 0;
+
+	// post send failed
+	log_rdma_send("ib_post_send failed rc=%d\n", rc);
+
+dma_mapping_failure:
+	for (i=0; i<n_vec+1; i++)
+		if (request->sge[i].addr)
+			ib_dma_unmap_single(info->id->device,
+					    request->sge[i].addr,
+					    request->sge[i].length,
+					    DMA_TO_DEVICE);
+	kfree(request->sge);
+
+allocate_sge_failed:
+	mempool_free(request, info->request_mempool);
+	return rc;
+}
+
+/*
  * Post a receive request to the transport
  * The remote peer can only send data when a receive is posted
  * The interaction is controlled by send/recieve credit system
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1710473 — Re: [[PATCH v1] 15/37] [CIFS] SMBD: Post a SMBD data transfer message with data payload

You can always get the struct page for kernel allocations using
virt_to_page (or vmalloc_to_page, but this code would not handle the
vmalloc case either), so I don't think you need this helper and can
always use the one added in the previous patch.

[toc] | [prev] | [next] | [standalone]

#1711415 — RE: [[PATCH v1] 15/37] [CIFS] SMBD: Post a SMBD data transfer message with data payload

> -----Original Message-----
> From: linux-cifs-owner@vger.kernel.org [mailto:linux-cifs-
> owner@vger.kernel.org] On Behalf Of Long Li
> Sent: Wednesday, August 2, 2017 4:10 PM
> To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org; samba-
> technical@lists.samba.org; linux-kernel@vger.kernel.org
> Cc: Long Li <longli@microsoft.com>
> Subject: [[PATCH v1] 15/37] [CIFS] SMBD: Post a SMBD data transfer message
> with data payload
> 
 
> Similar to sending transfer message with page payload, this function creates a
> SMBD data packet and send it over to RDMA, from iov passed from upper layer.

The following routine is heavily redundant with 14/37 cifs_rdma_post_send_page().
Because they share quite a bit of protocol and DMA mapping logic, strongly suggest
they be merged.

Tom.

> +static int cifs_rdma_post_send_data(
> +               struct cifs_rdma_info *info,
> +               struct kvec *iov, int n_vec, int remaining_data_length);
>  static int cifs_rdma_post_send_page(struct cifs_rdma_info *info,
>                 struct page *page, unsigned long offset,
>                 size_t size, int remaining_data_length);
> @@ -671,6 +674,122 @@ static int cifs_rdma_post_send_page(struct
> cifs_rdma_info *info, struct page *pa
>  }

[toc] | [prev] | [next] | [standalone]

#1702441 — [[PATCH v1] 04/37] [CIFS] SMBD: Define per-channel SMBD transport parameters and default values

From: Long Li <longli@microsoft.com>

For each channel, SMBD defines per-channel parameters. Those can be negotiated with the server, and are restricted by RDMA hardware limitations.

Signed-off-by: Long Li <longli@microsoft.com>
---
 fs/cifs/cifsrdma.c | 14 ++++++++++++++
 fs/cifs/cifsrdma.h | 13 +++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/fs/cifs/cifsrdma.c b/fs/cifs/cifsrdma.c
index 3c9f478..7c4c178 100644
--- a/fs/cifs/cifsrdma.c
+++ b/fs/cifs/cifsrdma.c
@@ -54,6 +54,20 @@
 
 #include "cifsrdma.h"
 
+/*
+ * Per RDMA transport connection parameters
+ * as defined in [MS-SMBD] 3.1.1.1
+ */
+static int receive_credit_max = 512;
+static int send_credit_target = 512;
+static int max_send_size = 8192;
+static int max_fragmented_recv_size = 1024*1024;
+static int max_receive_size = 8192;
+
+// maximum number of SGEs in a RDMA I/O
+static int max_send_sge = 16;
+static int max_recv_sge = 16;
+
 /* Logging functions
  * Logging are defined as classes. They can be ORed to define the actual
  * logging level via module parameter rdma_logging_class
diff --git a/fs/cifs/cifsrdma.h b/fs/cifs/cifsrdma.h
index ec6aa61..9979fd4 100644
--- a/fs/cifs/cifsrdma.h
+++ b/fs/cifs/cifsrdma.h
@@ -36,6 +36,19 @@
 struct cifs_rdma_info {
 	struct TCP_Server_Info *server_info;
 
+	//connection paramters
+	int receive_credit_max;
+	int send_credit_target;
+	int max_send_size;
+	int max_fragmented_recv_size;
+	int max_fragmented_send_size;
+	int max_receive_size;
+	int max_readwrite_size;
+	int protocol;
+	atomic_t send_credits;
+	atomic_t receive_credits;
+	atomic_t receive_credit_target;
+
 	// for debug purposes
 	unsigned int count_receive_buffer;
 	unsigned int count_get_receive_buffer;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1710468 — Re: [[PATCH v1] 04/37] [CIFS] SMBD: Define per-channel SMBD transport parameters and default values

> +/*
> + * Per RDMA transport connection parameters
> + * as defined in [MS-SMBD] 3.1.1.1
> + */
> +static int receive_credit_max = 512;
> +static int send_credit_target = 512;
> +static int max_send_size = 8192;
> +static int max_fragmented_recv_size = 1024*1024;
> +static int max_receive_size = 8192;

Are these protocol constants?  If so please use either #defines
or enums with upper case names for them.

> +// maximum number of SGEs in a RDMA I/O

Please always use /* ... */ style comments in the kernel.

[toc] | [prev] | [next] | [standalone]

#1711374 — RE: [[PATCH v1] 04/37] [CIFS] SMBD: Define per-channel SMBD transport parameters and default values

> -----Original Message-----
> From: linux-cifs-owner@vger.kernel.org [mailto:linux-cifs-
> owner@vger.kernel.org] On Behalf Of Christoph Hellwig
> Sent: Sunday, August 13, 2017 6:12 AM
> To: Long Li <longli@microsoft.com>
> Cc: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org; samba-
> technical@lists.samba.org; linux-kernel@vger.kernel.org; Long Li
> <longli@microsoft.com>
> Subject: Re: [[PATCH v1] 04/37] [CIFS] SMBD: Define per-channel SMBD
> transport parameters and default values
> 
> > +/*
> > + * Per RDMA transport connection parameters
> > + * as defined in [MS-SMBD] 3.1.1.1
> > + */
> > +static int receive_credit_max = 512;
> > +static int send_credit_target = 512;
> > +static int max_send_size = 8192;
> > +static int max_fragmented_recv_size = 1024*1024;
> > +static int max_receive_size = 8192;
> 
> Are these protocol constants?  If so please use either #defines
> or enums with upper case names for them.

These are not defined constants, but the values beg for some explanatory text
why they are chosen. Windows uses, and negotiates by default, a 1364-byte
maximum send size, and caps credits to 255. The other values match.

BTW, the parameters are defined in MS-SMBD 3.1.1.1 but the chosen values
are in behavior notes 2 and 7.

Tom.

[toc] | [prev] | [next] | [standalone]

#1711551 — RE: [[PATCH v1] 04/37] [CIFS] SMBD: Define per-channel SMBD transport parameters and default values

> -----Original Message-----
> From: Tom Talpey
> Sent: Monday, August 14, 2017 12:29 PM
> To: Christoph Hellwig <hch@infradead.org>; Long Li <longli@microsoft.com>
> Cc: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org; samba-
> technical@lists.samba.org; linux-kernel@vger.kernel.org
> Subject: RE: [[PATCH v1] 04/37] [CIFS] SMBD: Define per-channel SMBD
> transport parameters and default values
> 
> > -----Original Message-----
> > From: linux-cifs-owner@vger.kernel.org [mailto:linux-cifs-
> > owner@vger.kernel.org] On Behalf Of Christoph Hellwig
> > Sent: Sunday, August 13, 2017 6:12 AM
> > To: Long Li <longli@microsoft.com>
> > Cc: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org;
> > samba- technical@lists.samba.org; linux-kernel@vger.kernel.org; Long
> > Li <longli@microsoft.com>
> > Subject: Re: [[PATCH v1] 04/37] [CIFS] SMBD: Define per-channel SMBD
> > transport parameters and default values
> >
> > > +/*
> > > + * Per RDMA transport connection parameters
> > > + * as defined in [MS-SMBD] 3.1.1.1
> > > + */
> > > +static int receive_credit_max = 512; static int send_credit_target
> > > += 512; static int max_send_size = 8192; static int
> > > +max_fragmented_recv_size = 1024*1024; static int max_receive_size =
> > > +8192;
> >
> > Are these protocol constants?  If so please use either #defines or
> > enums with upper case names for them.
> 
> These are not defined constants, but the values beg for some explanatory
> text why they are chosen. Windows uses, and negotiates by default, a 1364-
> byte maximum send size, and caps credits to 255. The other values match.
> 
> BTW, the parameters are defined in MS-SMBD 3.1.1.1 but the chosen values
> are in behavior notes 2 and 7.

I will change those values to more inline with what Windows choses. The different values don't have a visible impact to performance while RDMA read/write is used.

> 
> Tom.

[toc] | [prev] | [next] | [standalone]

#1702442 — [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD transport

From: Long Li <longli@microsoft.com>

Define a new structure for SMBD transport. This stucture will have all the
information on the transport, and it will be stored in the current SMB session.

Signed-off-by: Long Li <longli@microsoft.com>
---
 fs/cifs/cifsrdma.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/cifs/cifsrdma.h | 45 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+)
 create mode 100644 fs/cifs/cifsrdma.c
 create mode 100644 fs/cifs/cifsrdma.h

diff --git a/fs/cifs/cifsrdma.c b/fs/cifs/cifsrdma.c
new file mode 100644
index 0000000..a2c0478
--- /dev/null
+++ b/fs/cifs/cifsrdma.c
@@ -0,0 +1,56 @@
+/*
+ *   Copyright (C) 2017, Microsoft Corporation.
+ *
+ *   Author(s): Long Li <longli@microsoft.com>
+ *
+ *   This program is free software;  you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
+ *   the GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program;  if not, write to the Free Software
+ *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+#include <linux/fs.h>
+#include <linux/net.h>
+#include <linux/string.h>
+#include <linux/list.h>
+#include <linux/wait.h>
+#include <linux/slab.h>
+#include <linux/pagemap.h>
+#include <linux/ctype.h>
+#include <linux/utsname.h>
+#include <linux/mempool.h>
+#include <linux/delay.h>
+#include <linux/completion.h>
+#include <linux/kthread.h>
+#include <linux/pagevec.h>
+#include <linux/freezer.h>
+#include <linux/namei.h>
+#include <asm/uaccess.h>
+#include <asm/processor.h>
+#include <linux/inet.h>
+#include <linux/module.h>
+#include <keys/user-type.h>
+#include <net/ipv6.h>
+#include <linux/parser.h>
+
+#include "cifspdu.h"
+#include "cifsglob.h"
+#include "cifsproto.h"
+#include "cifs_unicode.h"
+#include "cifs_debug.h"
+#include "cifs_fs_sb.h"
+#include "ntlmssp.h"
+#include "nterr.h"
+#include "rfc1002pdu.h"
+#include "fscache.h"
+
+#include "cifsrdma.h"
+
diff --git a/fs/cifs/cifsrdma.h b/fs/cifs/cifsrdma.h
new file mode 100644
index 0000000..ec6aa61
--- /dev/null
+++ b/fs/cifs/cifsrdma.h
@@ -0,0 +1,45 @@
+/*
+ *   Copyright (C) 2017, Microsoft Corporation.
+ *
+ *   Author(s): Long Li <longli@microsoft.com>
+ *
+ *   This program is free software;  you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
+ *   the GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program;  if not, write to the Free Software
+ *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+#ifndef _CIFS_RDMA_H
+#define _CIFS_RDMA_H
+
+#include "cifsglob.h"
+#include <rdma/ib_verbs.h>
+#include <rdma/rdma_cm.h>
+#include <linux/mempool.h>
+
+/*
+ * The context for the SMBDirect transport
+ * Everything related to the transport is here. It has several logical parts
+ * 1. RDMA related structures
+ * 2. SMBDirect connection parameters
+ * 3. Reassembly queue for data receive path
+ * 4. mempools for allocating packets
+ */
+struct cifs_rdma_info {
+	struct TCP_Server_Info *server_info;
+
+	// for debug purposes
+	unsigned int count_receive_buffer;
+	unsigned int count_get_receive_buffer;
+	unsigned int count_put_receive_buffer;
+	unsigned int count_send_empty;
+};
+#endif
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1706091 — Re: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD transport

Hi Li,

thanks for providing this patchset, I guess it will be a huge win to
have SMBDirect support for the kernel client!

> Define a new structure for SMBD transport. This stucture will have all the
> information on the transport, and it will be stored in the current SMB session.
...
> +/*
> + * The context for the SMBDirect transport
> + * Everything related to the transport is here. It has several
logical parts
> + * 1. RDMA related structures
> + * 2. SMBDirect connection parameters
> + * 3. Reassembly queue for data receive path
> + * 4. mempools for allocating packets
> + */
> +struct cifs_rdma_info {
> +	struct TCP_Server_Info *server_info;
> +
> +	// for debug purposes
> +	unsigned int count_receive_buffer;
> +	unsigned int count_get_receive_buffer;
> +	unsigned int count_put_receive_buffer;
> +	unsigned int count_send_empty;
> +};
> +#endif
>

It seems that the new transport is tied to it's caller
regarding structures and naming conventions.

I think it would be better to strictly separate them,
as I'd like to use the SMBDirect transport also from the
userspace for the client side e.g. in Samba's '[lib]smbclient',
but also in Samba's server side code 'smbd'.

Would it be possible to isolate this in
smb_direct.c and smb_direct.h while using
smb_direct_* prefixes for structures and
functions? Also avoiding the usage of other headers
from fs/cifs/*.h, expect for something generic like
nterr.h.

I guess 'struct cifs_rdma_info' would then be
'struct smb_direct_connection'. And it won't
have a reference to struct TCP_Server_Info.

It the strict layering is too much change,
I'd at least like to have the name changes.

This should relatively easy to do by using somthing like

git format-patch --stdout -37 > before

cat before | sed \
-e 's!struct cifs_rdma_info!struct smb_direct_connection!g' \
-e 's!cifsrdma\.h!smb_direct.h!g' \
-e 's!cifsrdma\.c!smb_direct.c!g' \
> after

git reset --hard HEAD~37
git am after

metze

[toc] | [prev] | [next] | [standalone]

#1710205 — RE: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD transport

> -----Original Message-----
> From: Stefan Metzmacher [mailto:metze@samba.org]
> Sent: Monday, August 7, 2017 11:58 PM
> To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org; samba-
> technical@lists.samba.org; linux-kernel@vger.kernel.org
> Cc: Long Li <longli@microsoft.com>
> Subject: Re: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD
> transport
> 
> Hi Li,
> 
> thanks for providing this patchset, I guess it will be a huge win to have
> SMBDirect support for the kernel client!
> 
> > Define a new structure for SMBD transport. This stucture will have all
> > the information on the transport, and it will be stored in the current SMB
> session.
> ...
> > +/*
> > + * The context for the SMBDirect transport
> > + * Everything related to the transport is here. It has several
> logical parts
> > + * 1. RDMA related structures
> > + * 2. SMBDirect connection parameters
> > + * 3. Reassembly queue for data receive path
> > + * 4. mempools for allocating packets  */ struct cifs_rdma_info {
> > +	struct TCP_Server_Info *server_info;
> > +
> > +	// for debug purposes
> > +	unsigned int count_receive_buffer;
> > +	unsigned int count_get_receive_buffer;
> > +	unsigned int count_put_receive_buffer;
> > +	unsigned int count_send_empty;
> > +};
> > +#endif
> >
> 
> It seems that the new transport is tied to it's caller regarding structures and
> naming conventions.
> 
> I think it would be better to strictly separate them, as I'd like to use the
> SMBDirect transport also from the userspace for the client side e.g. in
> Samba's '[lib]smbclient', but also in Samba's server side code 'smbd'.

Thank you for reviewing the patch set.

I think it is possible to separate the common code that implements the SMBDirect transport. There are some challenges to reuse the same code for both kernel and user spaces.
1. Kernel mode RDMA verbs are similar but different to user-mode ones.
2. Some RDMA features (e.g Fast Registration Work Request) are not available in user-mode.
3. Locking and synchronization mechanism is different
4. Memory management is different.
5. Process creation/scheduling and data sharing between processes are different, and there is no user-mode code running in interrupt/softirq.

Those needs to be abstracted through a layer, the rest of the code can be shared. I can work on this after patch set is reviewed.

> 
> Would it be possible to isolate this in
> smb_direct.c and smb_direct.h while using
> smb_direct_* prefixes for structures and functions? Also avoiding the usage
> of other headers from fs/cifs/*.h, expect for something generic like nterr.h.

Sure I will make naming changes and clean up the header files.
> 
> I guess 'struct cifs_rdma_info' would then be 'struct smb_direct_connection'.
> And it won't have a reference to struct TCP_Server_Info.

I will look for ways to remove reference to struct TCP_Server_Info . The reason why it has a reference to TCP_Server_Info is that: TCP_Server_Info represents a transport connection, although it also has many other TCP related code. SMBD needs to get to this connection TCP_Server_Info and set the transport status on shutdown (and maybe other situations).


Long

> 
> It the strict layering is too much change, I'd at least like to have the name
> changes.
> 
> This should relatively easy to do by using somthing like
> 
> git format-patch --stdout -37 > before
> 
> cat before | sed \
> -e 's!struct cifs_rdma_info!struct smb_direct_connection!g' \ -e
> 's!cifsrdma\.h!smb_direct.h!g' \ -e 's!cifsrdma\.c!smb_direct.c!g' \
> > after
> 
> git reset --hard HEAD~37
> git am after
> 
> metze

[toc] | [prev] | [next] | [standalone]

#1710350 — Re: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD transport

On Sat, Aug 12, 2017 at 08:32:48AM +0000, Long Li via samba-technical wrote:
> I think it is possible to separate the common code that implements the SMBDirect transport. There are some challenges to reuse the same code for both kernel and user spaces.
> 1. Kernel mode RDMA verbs are similar but different to user-mode ones.
> 2. Some RDMA features (e.g Fast Registration Work Request) are not available in user-mode.
> 3. Locking and synchronization mechanism is different
> 4. Memory management is different.
> 5. Process creation/scheduling and data sharing between processes are different, and there is no user-mode code running in interrupt/softirq.
> 
> Those needs to be abstracted through a layer, the rest of the code can be shared. I can work on this after patch set is reviewed.

NAK - code with those sort of obsfucation layer will be rejected
for kernel inclusion - don't add it.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web