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 2 of 2 — ← Prev page 1 [2]

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

[Multipart message — attachments visible in raw view] — view raw

Hi Long,

>> 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.

I guess this is a misunderstanding...

I don't want to use that code and run it in userspace,
I have a userspace prototype more or less working here, see
https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master3-rdma
and
https://git.samba.org/?p=metze/samba/wip.git;a=blob;f=libcli/smb/smb_direct.c;h=9cc0d861ccfcbb4df9ef6ad85a7fe3d262e549c0;hb=85d46de6fdbba041d3e8004af46865a72d2b8405

I goal is that we'll have an api that allows userspace
code to use the kernel code SMBDirect code. This
userspace code would get a file descriptor from the kernel
and would be able to use it similar to a tcp socket.
If the kernel would simulate the 4 byte length header,
it's trivial to get to a stage were smbclient and smbd
are able to support SMBDirect without much changes.
We only need to replace connect(), listen(), accept() and a few more
by SMBDirect versions.

For the real data transfer we might be able to use memfd_create()
or something similar to share the buffers between userspace and kernel.

I guess this is a long way, but having the basic SMBDirect code in
dependently in the kernel would help a lot.

>> 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.

Thanks!

>> 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).
> 

Wouldn't it be better to provide a way to ask for the connection state
and let the caller ask for the state instead of changing the callers
structure?

metze

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

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

> -----Original Message-----
> From: Stefan Metzmacher [mailto:metze@samba.org]
> Sent: Monday, August 14, 2017 6:41 AM
> 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; linux-rdma@vger.kernel.org; Christoph Hellwig
> <hch@infradead.org>
> Subject: Re: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD
> transport
> 
> Hi Long,
> 
> >> 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.
> 
> I guess this is a misunderstanding...
> 
> I don't want to use that code and run it in userspace, I have a userspace
> prototype more or less working here, see
> https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/m
> aster3-rdma
> and
> https://git.samba.org/?p=metze/samba/wip.git;a=blob;f=libcli/smb/smb_dir
> ect.c;h=9cc0d861ccfcbb4df9ef6ad85a7fe3d262e549c0;hb=85d46de6fdbba041
> d3e8004af46865a72d2b8405
> 
> I goal is that we'll have an api that allows userspace code to use the kernel
> code SMBDirect code. This userspace code would get a file descriptor from
> the kernel and would be able to use it similar to a tcp socket.
> If the kernel would simulate the 4 byte length header, it's trivial to get to a
> stage were smbclient and smbd are able to support SMBDirect without much
> changes.
> We only need to replace connect(), listen(), accept() and a few more by
> SMBDirect versions.

This is possible.  SMBDirect code can handle the first 4 bytes for upper layer.
> 
> For the real data transfer we might be able to use memfd_create() or
> something similar to share the buffers between userspace and kernel.

You'll need to post RDMA read/write through the same QP created by SMBDirect in the kernel. I think this needs some more work but it's doable.

> 
> I guess this is a long way, but having the basic SMBDirect code in dependently
> in the kernel would help a lot.
> 
> >> 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.
> 
> Thanks!
> 
> >> 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).
> >
> 
> Wouldn't it be better to provide a way to ask for the connection state and let
> the caller ask for the state instead of changing the callers structure?
> 
> metze

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

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

On Wed, Aug 02, 2017 at 01:10:13PM -0700, Long Li wrote:
> 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>

Where do all these includes come from?  It seems like most of them
are not actually used in the code.

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

#1702445 — [[PATCH v1] 18/37] [CIFS] SMBD: Implement API for upper layer to send data

From: Long Li <longli@microsoft.com>

Implement cifs_rdma_write for send an upper layer data. Upper layer uses this function to do a RDMA send. This function is also used to pass SMB packets for doing a RDMA read/write via memory registration.

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

diff --git a/fs/cifs/cifsrdma.c b/fs/cifs/cifsrdma.c
index ef21f1c..eb48651 100644
--- a/fs/cifs/cifsrdma.c
+++ b/fs/cifs/cifsrdma.c
@@ -229,6 +229,10 @@ static void send_done(struct ib_cq *cq, struct ib_wc *wc)
 			request->sge[i].length,
 			DMA_TO_DEVICE);
 
+	if (atomic_dec_and_test(&request->info->send_pending)) {
+		wake_up(&request->info->wait_send_pending);
+	}
+
 	kfree(request->sge);
 	mempool_free(request, request->info->request_mempool);
 }
@@ -551,12 +555,14 @@ static int cifs_rdma_post_send_negotiate_req(struct cifs_rdma_info *info)
 		request->sge[0].addr,
 		request->sge[0].length, request->sge[0].lkey);
 
+	atomic_inc(&info->send_pending);
 	rc = ib_post_send(info->id->qp, &send_wr, &send_wr_fail);
 	if (!rc)
 		return 0;
 
 	// if we reach here, post send failed
 	log_rdma_send("ib_post_send failed rc=%d\n", rc);
+	atomic_dec(&info->send_pending);
 	ib_dma_unmap_single(info->id->device, request->sge[0].addr,
 		request->sge[0].length, DMA_TO_DEVICE);
 
@@ -662,12 +668,14 @@ static int cifs_rdma_post_send_page(struct cifs_rdma_info *info, struct page *pa
 	send_wr.opcode = IB_WR_SEND;
 	send_wr.send_flags = IB_SEND_SIGNALED;
 
+	atomic_inc(&info->send_pending);
 	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);
+	atomic_dec(&info->send_pending);
 
 dma_mapping_failed:
 	for (i=0; i<2; i++)
@@ -768,11 +776,13 @@ static int cifs_rdma_post_send_empty(struct cifs_rdma_info *info)
 	send_wr.opcode = IB_WR_SEND;
 	send_wr.send_flags = IB_SEND_SIGNALED;
 
+	atomic_inc(&info->send_pending);
 	rc = ib_post_send(info->id->qp, &send_wr, &send_wr_fail);
 	if (!rc)
 		return 0;
 
 	log_rdma_send("ib_post_send failed rc=%d\n", rc);
+	atomic_dec(&info->send_pending);
 	ib_dma_unmap_single(info->id->device, request->sge[0].addr,
 			    request->sge[0].length, DMA_TO_DEVICE);
 
@@ -885,12 +895,14 @@ static int cifs_rdma_post_send_data(
 	send_wr.opcode = IB_WR_SEND;
 	send_wr.send_flags = IB_SEND_SIGNALED;
 
+	atomic_inc(&info->send_pending);
 	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);
+	atomic_dec(&info->send_pending);
 
 dma_mapping_failure:
 	for (i=0; i<n_vec+1; i++)
@@ -1185,6 +1197,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_send_pending);
+	atomic_set(&info->send_pending, 0);
+
 	init_waitqueue_head(&info->wait_recv_pending);
 	atomic_set(&info->recv_pending, 0);
 
@@ -1202,3 +1217,165 @@ struct cifs_rdma_info* cifs_create_rdma_session(
 	kfree(info);
 	return NULL;
 }
+
+/*
+ * Write data to transport
+ * Each rqst is transported as a SMBDirect payload
+ * rqst: the data to write
+ * return value: 0 if successfully write, otherwise error code
+ */
+int cifs_rdma_write(struct cifs_rdma_info *info, struct smb_rqst *rqst)
+{
+	struct kvec vec;
+	int nvecs;
+	int size;
+	int buflen=0, remaining_data_length;
+	int start, i, j;
+	int max_iov_size = info->max_send_size - sizeof(struct smbd_data_transfer);
+	struct kvec *iov;
+	int rc;
+
+	if (info->transport_status != CIFS_RDMA_CONNECTED) {
+		log_cifs_write("disconnected returning -EIO\n");
+		return -EIO;
+	}
+
+	iov = kzalloc(sizeof(struct kvec)*rqst->rq_nvec, GFP_KERNEL);
+	if (!iov) {
+		log_cifs_write("failed to allocate iov returing -ENOMEM\n");
+		return -ENOMEM;
+	}
+
+	/* Strip the first 4 bytes MS-SMB2 section 2.1
+	 * they are used only for TCP transport */
+	iov[0].iov_base = (char*)rqst->rq_iov[0].iov_base + 4;
+	iov[0].iov_len = rqst->rq_iov[0].iov_len - 4;
+	buflen += iov[0].iov_len;
+
+	/* total up iov array first */
+	for (i = 1; i < rqst->rq_nvec; i++) {
+		iov[i].iov_base = rqst->rq_iov[i].iov_base;
+		iov[i].iov_len = rqst->rq_iov[i].iov_len;
+		buflen += iov[i].iov_len;
+	}
+
+	/* add in the page array if there is one */
+	if (rqst->rq_npages) {
+		buflen += rqst->rq_pagesz * (rqst->rq_npages - 1);
+		buflen += rqst->rq_tailsz;
+	}
+
+	if (buflen + sizeof(struct smbd_data_transfer) >
+		info->max_fragmented_send_size) {
+		log_cifs_write("payload size %d > max size %d\n",
+			buflen, info->max_fragmented_send_size);
+		rc = -EINVAL;
+		goto done;
+	}
+
+	remaining_data_length = buflen;
+
+	log_cifs_write("rqst->rq_nvec=%d rqst->rq_npages=%d rq_pagesz=%d "
+		"rq_tailsz=%d buflen=%d\n",
+		rqst->rq_nvec, rqst->rq_npages, rqst->rq_pagesz,
+		rqst->rq_tailsz, buflen);
+
+	start = i = iov[0].iov_len ? 0 : 1;
+	buflen = 0;
+	while (true){
+		buflen += iov[i].iov_len;
+		if (buflen > max_iov_size) {
+			if (i > start) {
+				remaining_data_length -=
+					(buflen-iov[i].iov_len);
+				log_cifs_write("sending iov[] from start=%d "
+					"i=%d nvecs=%d "
+					"remaining_data_length=%d\n",
+					start, i, i-start,
+					remaining_data_length);
+				rc = cifs_rdma_post_send_data(
+					info, &iov[start], i-start,
+					remaining_data_length);
+				if (rc)
+					goto done;
+			} else {
+				// iov[start] is too big, break it to nvecs pieces
+				nvecs = (buflen+max_iov_size-1)/max_iov_size;
+				log_cifs_write("iov[%d] iov_base=%p buflen=%d"
+					" break to %d vectors\n",
+					start, iov[start].iov_base,
+					buflen, nvecs);
+				for (j=0; j<nvecs; j++) {
+					vec.iov_base =
+						(char *)iov[start].iov_base +
+						j*max_iov_size;
+					vec.iov_len = max_iov_size;
+					if (j == nvecs-1)
+						vec.iov_len =
+							buflen -
+							max_iov_size*(nvecs-1);
+					remaining_data_length -= vec.iov_len;
+					log_cifs_write(
+						"sending vec j=%d iov_base=%p"
+						" iov_len=%lu "
+						"remaining_data_length=%d\n",
+						j, vec.iov_base, vec.iov_len,
+						remaining_data_length);
+					rc = cifs_rdma_post_send_data(
+						info, &vec, 1,
+						remaining_data_length);
+					if (rc)
+						goto done;
+				}
+				i++;
+			}
+			start = i;
+			buflen = 0;
+		} else {
+			i++;
+			if (i == rqst->rq_nvec) {
+				// send out all remaining vecs and we are done
+				remaining_data_length -= buflen;
+				log_cifs_write(
+					"sending iov[] from start=%d i=%d "
+					"nvecs=%d remaining_data_length=%d\n",
+					start, i, i-start,
+					remaining_data_length);
+				rc = cifs_rdma_post_send_data(info, &iov[start],
+					i-start, remaining_data_length);
+				if (rc)
+					goto done;
+				break;
+			}
+		}
+		log_cifs_write("looping i=%d buflen=%d\n", i, buflen);
+	}
+
+	// now sending pages
+	for (i = 0; i < rqst->rq_npages; i++) {
+		buflen = (i == rqst->rq_npages-1) ?
+			rqst->rq_tailsz : rqst->rq_pagesz;
+		nvecs = (buflen+max_iov_size-1)/max_iov_size;
+		log_cifs_write("sending pages buflen=%d nvecs=%d\n",
+			buflen, nvecs);
+		for (j=0; j<nvecs; j++) {
+			size = max_iov_size;
+			if (j == nvecs-1)
+				size = buflen - j*max_iov_size;
+			remaining_data_length -= size;
+			log_cifs_write("sending pages i=%d offset=%d size=%d"
+				" remaining_data_length=%d\n",
+				i, j*max_iov_size, size, remaining_data_length);
+			rc = cifs_rdma_post_send_page(
+				info, rqst->rq_pages[i], j*max_iov_size,
+				size, remaining_data_length);
+			if (rc)
+				goto done;
+		}
+	}
+
+done:
+	kfree(iov);
+	wait_event(info->wait_send_pending, atomic_read(&info->send_pending) == 0);
+	return rc;
+}
diff --git a/fs/cifs/cifsrdma.h b/fs/cifs/cifsrdma.h
index 9618e0b..90746a4 100644
--- a/fs/cifs/cifsrdma.h
+++ b/fs/cifs/cifsrdma.h
@@ -73,6 +73,9 @@ struct cifs_rdma_info {
 	atomic_t receive_credits;
 	atomic_t receive_credit_target;
 
+	atomic_t send_pending;
+	wait_queue_head_t wait_send_pending;
+
 	atomic_t recv_pending;
 	wait_queue_head_t wait_recv_pending;
 
@@ -195,4 +198,6 @@ struct cifs_rdma_response {
 // Create a SMBDirect session
 struct cifs_rdma_info* cifs_create_rdma_session(
 	struct TCP_Server_Info *server, struct sockaddr *dstaddr);
+
+int cifs_rdma_write(struct cifs_rdma_info *rdma, struct smb_rqst *rqst);
 #endif
-- 
2.7.4

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

#1711433 — RE: [[PATCH v1] 18/37] [CIFS] SMBD: Implement API for upper layer to send data

> -----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] 18/37] [CIFS] SMBD: Implement API for upper layer to
> send data
> 
> +/*
> + * Write data to transport
> + * Each rqst is transported as a SMBDirect payload
> + * rqst: the data to write
> + * return value: 0 if successfully write, otherwise error code
> + */
> +int cifs_rdma_write(struct cifs_rdma_info *info, struct smb_rqst *rqst)
> +{

!!!
This is a VERY confusing name. It is not sending an RDMA Write, which will
confuse any RDMA-enlightened reader. It's performing an RDMA Send, so
that name is perhaps one possibility. 

> +       if (info->transport_status != CIFS_RDMA_CONNECTED) {
> +               log_cifs_write("disconnected returning -EIO\n");
> +               return -EIO;
> +       }

Isn't this optimizing the error case? There's no guarantee it's still connected by
the time the following request construction occurs. Why not just proceed without
the check? 

> +       /* Strip the first 4 bytes MS-SMB2 section 2.1
> +        * they are used only for TCP transport */
> +       iov[0].iov_base = (char*)rqst->rq_iov[0].iov_base + 4;
> +       iov[0].iov_len = rqst->rq_iov[0].iov_len - 4;
> +       buflen += iov[0].iov_len;

Ok, that layering choice in the cifs.ko client code needs to be corrected. After all,
it will need to be RDMA-aware to build the SMB3 read/write channel structures.
And, the code in cifs_post_send_data() is allocating and building a structure that
could have been accounted for much earlier, avoiding the extra overhead.

That change could happen later, the hack is mostly ok for now. But something
needs to be said in a comment.
 
Tom.

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

#1715773 — RE: [[PATCH v1] 18/37] [CIFS] SMBD: Implement API for upper layer to send data

> -----Original Message-----
> From: Tom Talpey
> Sent: Monday, August 14, 2017 1:44 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; linux-rdma@vger.kernel.org
> Subject: RE: [[PATCH v1] 18/37] [CIFS] SMBD: Implement API for upper layer
> to send data
> 
> > -----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] 18/37] [CIFS] SMBD: Implement API for upper layer
> > to send data
> >
> > +/*
> > + * Write data to transport
> > + * Each rqst is transported as a SMBDirect payload
> > + * rqst: the data to write
> > + * return value: 0 if successfully write, otherwise error code  */
> > +int cifs_rdma_write(struct cifs_rdma_info *info, struct smb_rqst
> > +*rqst) {
> 
> !!!
> This is a VERY confusing name. It is not sending an RDMA Write, which will
> confuse any RDMA-enlightened reader. It's performing an RDMA Send, so
> that name is perhaps one possibility.
> 
> > +       if (info->transport_status != CIFS_RDMA_CONNECTED) {
> > +               log_cifs_write("disconnected returning -EIO\n");
> > +               return -EIO;
> > +       }
> 
> Isn't this optimizing the error case? There's no guarantee it's still connected
> by the time the following request construction occurs. Why not just proceed
> without the check?
> 
> > +       /* Strip the first 4 bytes MS-SMB2 section 2.1
> > +        * they are used only for TCP transport */
> > +       iov[0].iov_base = (char*)rqst->rq_iov[0].iov_base + 4;
> > +       iov[0].iov_len = rqst->rq_iov[0].iov_len - 4;
> > +       buflen += iov[0].iov_len;
> 
> Ok, that layering choice in the cifs.ko client code needs to be corrected. After
> all, it will need to be RDMA-aware to build the SMB3 read/write channel
> structures.
> And, the code in cifs_post_send_data() is allocating and building a structure
> that could have been accounted for much earlier, avoiding the extra
> overhead.
> 
> That change could happen later, the hack is mostly ok for now. But
> something needs to be said in a comment.

Will address those in v2.

> 
> Tom.

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

#1702447 — [[PATCH v1] 12/37] [CIFS] SMBD: Handle send completion from CQ

From: Long Li <longli@microsoft.com>

In preparation for handling sending SMBD requests, add code to handle the send completion. In send complemention, the SMBD transport is responsible for freeing resources used in send.

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

diff --git a/fs/cifs/cifsrdma.c b/fs/cifs/cifsrdma.c
index 20237b7..ecbc832 100644
--- a/fs/cifs/cifsrdma.c
+++ b/fs/cifs/cifsrdma.c
@@ -197,6 +197,31 @@ cifs_rdma_qp_async_error_upcall(struct ib_event *event, void *context)
 	}
 }
 
+/* Called in softirq, when a RDMA send is donea */
+static void send_done(struct ib_cq *cq, struct ib_wc *wc)
+{
+	int i;
+	struct cifs_rdma_request *request =
+		container_of(wc->wr_cqe, struct cifs_rdma_request, cqe);
+
+	log_rdma_send("cifs_rdma_request %p completed wc->status=%d\n",
+		request, wc->status);
+
+	if (wc->status != IB_WC_SUCCESS || wc->opcode != IB_WC_SEND) {
+		log_rdma_send("wc->status=%d wc->opcode=%d\n",
+			wc->status, wc->opcode);
+	}
+
+	for (i=0; i<request->num_sge; i++)
+		ib_dma_unmap_single(request->info->id->device,
+			request->sge[i].addr,
+			request->sge[i].length,
+			DMA_TO_DEVICE);
+
+	kfree(request->sge);
+	mempool_free(request, request->info->request_mempool);
+}
+
 /* Called from softirq, when recv is done */
 static void recv_done(struct ib_cq *cq, struct ib_wc *wc)
 {
-- 
2.7.4

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

#1710470 — Re: [[PATCH v1] 12/37] [CIFS] SMBD: Handle send completion from CQ

You seem to be doing memory allocations and frees for every packet on
the write.  At least for other RDMA protocols that would have been
a major performance issue.  

Do you have any performance numbers and/or profiles of the code?

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

#1711324 — RE: [[PATCH v1] 12/37] [CIFS] SMBD: Handle send completion from CQ

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Sunday, August 13, 2017 3:20 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] 12/37] [CIFS] SMBD: Handle send completion from
> CQ
> 
> You seem to be doing memory allocations and frees for every packet on the
> write.  At least for other RDMA protocols that would have been a major
> performance issue.

The size of SGE array passed to IB is unknown, so I don't know how much to pre-allocate in advance. But it seems this size is not big when passed down from CIFS. I will look at pre-allocating buffer if this is an issue.

> 
> Do you have any performance numbers and/or profiles of the code?

I will look into profiling.

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

#1702448 — [[PATCH v1] 09/37] [CIFS] SMBD: Add SMBD request and cache

From: Long Li <longli@microsoft.com>

Define SMBD request. Unlike SMBD response, SMBD request doesn't use pre-allocated buffers. Data payload is passed from upper layer provided data buffers. SMBD requests are allocated through per-channel mempool.

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

diff --git a/fs/cifs/cifsrdma.c b/fs/cifs/cifsrdma.c
index b3e43b1..9610897 100644
--- a/fs/cifs/cifsrdma.c
+++ b/fs/cifs/cifsrdma.c
@@ -459,6 +459,18 @@ struct cifs_rdma_info* cifs_create_rdma_session(
 
 	log_rdma_event("rdma_connect connected\n");
 
+	sprintf(cache_name, "cifs_smbd_request_%p", info);
+	info->request_cache =
+		kmem_cache_create(
+			cache_name,
+			sizeof(struct cifs_rdma_request) +
+				sizeof(struct smbd_data_transfer),
+			0, SLAB_HWCACHE_ALIGN, NULL);
+
+	info->request_mempool =
+		mempool_create(info->send_credit_target, mempool_alloc_slab,
+			mempool_free_slab, info->request_cache);
+
 	sprintf(cache_name, "cifs_smbd_response_%p", info);
 	info->response_cache =
 		kmem_cache_create(
diff --git a/fs/cifs/cifsrdma.h b/fs/cifs/cifsrdma.h
index ed0ff54..e925aa4 100644
--- a/fs/cifs/cifsrdma.h
+++ b/fs/cifs/cifsrdma.h
@@ -62,6 +62,10 @@ struct cifs_rdma_info {
 	struct list_head receive_queue;
 	spinlock_t receive_queue_lock;
 
+	// request pool for RDMA send
+	struct kmem_cache *request_cache;
+	mempool_t *request_mempool;
+
 	// response pool for RDMA receive
 	struct kmem_cache *response_cache;
 	mempool_t *response_mempool;
@@ -93,6 +97,21 @@ struct smbd_data_transfer {
 	char buffer[0];
 } __packed;
 
+// The context for a SMBD request
+struct cifs_rdma_request {
+	struct cifs_rdma_info *info;
+
+	// completion queue entry
+	struct ib_cqe cqe;
+
+	// the SGE entries for this packet
+	struct ib_sge *sge;
+	int num_sge;
+
+	// SMBD packet header follows this structure
+	char packet[0];
+};
+
 // The context for a SMBD response
 struct cifs_rdma_response {
 	struct cifs_rdma_info *info;
-- 
2.7.4

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

#1702449 — [[PATCH v1] 05/37] [CIFS] SMBD: Implement API for upper layer to create SMBD transport and establish RDMA connection

From: Long Li <longli@microsoft.com>

Implement the code for connecting to SMBD server. The client and server are connected using RC Queue Pair over RDMA API, which suppports Infiniband, RoCE and iWARP. Upper layer code can call cifs_create_rdma_session to establish a SMBD RDMA connection.

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

diff --git a/fs/cifs/cifsrdma.c b/fs/cifs/cifsrdma.c
index 7c4c178..b18fb79 100644
--- a/fs/cifs/cifsrdma.c
+++ b/fs/cifs/cifsrdma.c
@@ -120,3 +120,260 @@ do {									\
 			atomic_read(&info->send_credits),		\
 			info->send_credit_target);			\
 } while (0)
+
+/* Upcall from RDMA CM */
+static int cifs_rdma_conn_upcall(
+		struct rdma_cm_id *id, struct rdma_cm_event *event)
+{
+	struct cifs_rdma_info *info = id->context;
+
+	log_rdma_event("event=%d status=%d\n", event->event, event->status);
+
+	switch (event->event) {
+	case RDMA_CM_EVENT_ADDR_RESOLVED:
+	case RDMA_CM_EVENT_ROUTE_RESOLVED:
+		info->ri_rc = 0;
+		complete(&info->ri_done);
+		break;
+
+	case RDMA_CM_EVENT_ADDR_ERROR:
+		info->ri_rc = -EHOSTUNREACH;
+		complete(&info->ri_done);
+		break;
+
+	case RDMA_CM_EVENT_ROUTE_ERROR:
+		info->ri_rc = -ENETUNREACH;
+		complete(&info->ri_done);
+		break;
+
+	case RDMA_CM_EVENT_ESTABLISHED:
+	case RDMA_CM_EVENT_CONNECT_ERROR:
+	case RDMA_CM_EVENT_UNREACHABLE:
+	case RDMA_CM_EVENT_REJECTED:
+	case RDMA_CM_EVENT_DEVICE_REMOVAL:
+		log_rdma_event("connected event=%d\n", event->event);
+		info->connect_state = event->event;
+		break;
+
+	case RDMA_CM_EVENT_DISCONNECTED:
+		break;
+
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+/* Upcall from RDMA QP */
+static void
+cifs_rdma_qp_async_error_upcall(struct ib_event *event, void *context)
+{
+	struct cifs_rdma_info *info = context;
+	log_rdma_event("%s on device %s info %p\n",
+		ib_event_msg(event->event), event->device->name, info);
+
+	switch (event->event)
+	{
+	case IB_EVENT_CQ_ERR:
+	case IB_EVENT_QP_FATAL:
+	case IB_EVENT_QP_REQ_ERR:
+	case IB_EVENT_QP_ACCESS_ERR:
+
+	default:
+		break;
+	}
+}
+
+static struct rdma_cm_id* cifs_rdma_create_id(
+		struct cifs_rdma_info *info, struct sockaddr *dstaddr)
+{
+	struct rdma_cm_id *id;
+	int rc;
+	struct sockaddr_in *addr_in = (struct sockaddr_in*) dstaddr;
+	__be16 *sport;
+
+	log_rdma_event("connecting to IP %pI4 port %d\n",
+		&addr_in->sin_addr, ntohs(addr_in->sin_port));
+
+	id = rdma_create_id(&init_net, cifs_rdma_conn_upcall, info,
+		RDMA_PS_TCP, IB_QPT_RC);
+	if (IS_ERR(id)) {
+		rc = PTR_ERR(id);
+		log_rdma_event("rdma_create_id() failed %i\n", rc);
+		return id;
+	}
+
+	if (dstaddr->sa_family == AF_INET6)
+		sport = &((struct sockaddr_in6 *)dstaddr)->sin6_port;
+	else
+		sport = &((struct sockaddr_in *)dstaddr)->sin_port;
+
+	*sport = htons(445);
+try_again:
+	init_completion(&info->ri_done);
+	info->ri_rc = -ETIMEDOUT;
+	rc = rdma_resolve_addr(id, NULL, (struct sockaddr*)dstaddr, 5000);
+	if (rc) {
+		log_rdma_event("rdma_resolve_addr() failed %i\n", rc);
+		goto out;
+	}
+	wait_for_completion_interruptible_timeout(
+		&info->ri_done, msecs_to_jiffies(8000));
+	rc = info->ri_rc;
+	if (rc) {
+		log_rdma_event("rdma_resolve_addr() completed %i\n", rc);
+		goto out;
+	}
+
+	info->ri_rc = -ETIMEDOUT;
+	rc = rdma_resolve_route(id, 5000);
+	if (rc) {
+		log_rdma_event("rdma_resolve_route() failed %i\n", rc);
+		goto out;
+	}
+	wait_for_completion_interruptible_timeout(
+		&info->ri_done, msecs_to_jiffies(8000));
+	rc = info->ri_rc;
+	if (rc) {
+		log_rdma_event("rdma_resolve_route() completed %i\n", rc);
+		goto out;
+	}
+
+	return id;
+
+out:
+	// try port number 5445 if port 445 doesn't work
+	if (*sport == htons(445)) {
+		*sport = htons(5445);
+		goto try_again;
+	}
+	rdma_destroy_id(id);
+	return ERR_PTR(rc);
+}
+
+static int cifs_rdma_ia_open(
+		struct cifs_rdma_info *info, struct sockaddr *dstaddr)
+{
+	int rc;
+
+	info->id = cifs_rdma_create_id(info, dstaddr);
+	if (IS_ERR(info->id)) {
+		rc = PTR_ERR(info->id);
+		goto out1;
+	}
+
+	info->pd = ib_alloc_pd(info->id->device, 0);
+	if (IS_ERR(info->pd)) {
+		rc = PTR_ERR(info->pd);
+		log_rdma_event("ib_alloc_pd() returned %d\n", rc);
+		goto out2;
+	}
+
+	return 0;
+
+out2:
+	rdma_destroy_id(info->id);
+	info->id = NULL;
+
+out1:
+	return rc;
+}
+
+struct cifs_rdma_info* cifs_create_rdma_session(
+	struct TCP_Server_Info *server, struct sockaddr *dstaddr)
+{
+	int rc;
+	struct cifs_rdma_info *info;
+	struct rdma_conn_param conn_param;
+	struct ib_qp_init_attr qp_attr;
+	int max_pending = receive_credit_max + send_credit_target;
+
+	info = kzalloc(sizeof(struct cifs_rdma_info), GFP_KERNEL);
+	if (!info)
+		return NULL;
+
+	info->server_info = server;
+
+	rc = cifs_rdma_ia_open(info, dstaddr);
+	if (rc) {
+		log_rdma_event("cifs_rdma_ia_open rc=%d\n", rc);
+		goto out1;
+	}
+
+	if (max_pending > info->id->device->attrs.max_cqe ||
+	    max_pending > info->id->device->attrs.max_qp_wr) {
+		log_rdma_event("consider lowering receive_credit_max and "
+			"send_credit_target. Possible CQE overrun, device "
+			"reporting max_cpe %d max_qp_wr %d\n",
+			info->id->device->attrs.max_cqe,
+			info->id->device->attrs.max_qp_wr);
+		goto out2;
+	}
+
+	info->receive_credit_max = receive_credit_max;
+	info->send_credit_target = send_credit_target;
+	info->max_send_size = max_send_size;
+	info->max_fragmented_recv_size = max_fragmented_recv_size;
+	info->max_receive_size = max_receive_size;
+
+	max_send_sge = min_t(int, max_send_sge,
+		info->id->device->attrs.max_sge);
+	max_recv_sge = min_t(int, max_recv_sge,
+		info->id->device->attrs.max_sge_rd);
+
+	info->cq = ib_alloc_cq(info->id->device, info,
+			info->receive_credit_max + info->send_credit_target,
+			0, IB_POLL_SOFTIRQ);
+	if (IS_ERR(info->cq))
+		goto out2;
+
+	memset(&qp_attr, 0, sizeof qp_attr);
+	qp_attr.event_handler = cifs_rdma_qp_async_error_upcall;
+	qp_attr.qp_context = info;
+	qp_attr.cap.max_send_wr = info->send_credit_target;
+	qp_attr.cap.max_recv_wr = info->receive_credit_max;
+	qp_attr.cap.max_send_sge = max_send_sge;
+	qp_attr.cap.max_recv_sge = max_recv_sge;
+	qp_attr.cap.max_inline_data = 0;
+	qp_attr.sq_sig_type = IB_SIGNAL_REQ_WR;
+	qp_attr.qp_type = IB_QPT_RC;
+	qp_attr.send_cq = info->cq;
+	qp_attr.recv_cq = info->cq;
+	qp_attr.port_num = ~0;
+
+	rc = rdma_create_qp(info->id, info->pd, &qp_attr);
+	if (rc) {
+		log_rdma_event("rdma_create_qp failed %i\n", rc);
+		rc = -ENETUNREACH;
+		goto out2;
+	}
+
+	memset(&conn_param, 0, sizeof(conn_param));
+	conn_param.private_data = NULL;
+	conn_param.private_data_len = 0;
+	conn_param.initiator_depth = 0;
+	conn_param.responder_resources = 32;
+	if (info->id->device->attrs.max_qp_rd_atom < 32)
+		conn_param.responder_resources =
+			info->id->device->attrs.max_qp_rd_atom;
+	conn_param.retry_count = 6;
+	conn_param.rnr_retry_count = 6;
+	conn_param.flow_control = 0;
+	rc = rdma_connect(info->id, &conn_param);
+	if (rc) {
+		log_rdma_event("rdma_connect() failed with %i\n", rc);
+		goto out2;
+	}
+
+	if (info->connect_state != RDMA_CM_EVENT_ESTABLISHED)
+		goto out2;
+
+	log_rdma_event("rdma_connect connected\n");
+out2:
+	rdma_destroy_id(info->id);
+
+out1:
+	kfree(info);
+	return NULL;
+}
diff --git a/fs/cifs/cifsrdma.h b/fs/cifs/cifsrdma.h
index 9979fd4..71ea380 100644
--- a/fs/cifs/cifsrdma.h
+++ b/fs/cifs/cifsrdma.h
@@ -36,6 +36,16 @@
 struct cifs_rdma_info {
 	struct TCP_Server_Info *server_info;
 
+	// RDMA related
+	struct rdma_cm_id *id;
+	struct ib_qp_init_attr qp_attr;
+	struct ib_pd *pd;
+	struct ib_cq *cq;
+	struct ib_device_attr dev_attr;
+	int connect_state;
+	int ri_rc;
+	struct completion ri_done;
+
 	//connection paramters
 	int receive_credit_max;
 	int send_credit_target;
@@ -55,4 +65,8 @@ struct cifs_rdma_info {
 	unsigned int count_put_receive_buffer;
 	unsigned int count_send_empty;
 };
+
+// Create a SMBDirect session
+struct cifs_rdma_info* cifs_create_rdma_session(
+	struct TCP_Server_Info *server, struct sockaddr *dstaddr);
 #endif
-- 
2.7.4

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

#1711390 — RE: [[PATCH v1] 05/37] [CIFS] SMBD: Implement API for upper layer to create SMBD transport and establish RDMA connection

> -----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] 05/37] [CIFS] SMBD: Implement API for upper layer to
> create SMBD transport and establish RDMA connection
> 
> From: Long Li <longli@microsoft.com>
> 
> Implement the code for connecting to SMBD server. The client and server are
> connected using RC Queue Pair over RDMA API, which suppports Infiniband,
> RoCE and iWARP. Upper layer code can call cifs_create_rdma_session to
> establish a SMBD RDMA connection.
> 
> +/* Upcall from RDMA CM */
> +static int cifs_rdma_conn_upcall(
> +               struct rdma_cm_id *id, struct rdma_cm_event *event)
> +{
> +       struct cifs_rdma_info *info = id->context;
> +
> +       log_rdma_event("event=%d status=%d\n", event->event, event->status);
> +
> +       switch (event->event) {
> +       case RDMA_CM_EVENT_ADDR_RESOLVED:
> +       case RDMA_CM_EVENT_ROUTE_RESOLVED:
> +               info->ri_rc = 0;
> +               complete(&info->ri_done);
> +               break;
> +
> +       case RDMA_CM_EVENT_ADDR_ERROR:
> +               info->ri_rc = -EHOSTUNREACH;
> +               complete(&info->ri_done);
> +               break;
> +
> +       case RDMA_CM_EVENT_ROUTE_ERROR:
> +               info->ri_rc = -ENETUNREACH;
> +               complete(&info->ri_done);
> +               break;
> +
> +       case RDMA_CM_EVENT_ESTABLISHED:
> +       case RDMA_CM_EVENT_CONNECT_ERROR:
> +       case RDMA_CM_EVENT_UNREACHABLE:
> +       case RDMA_CM_EVENT_REJECTED:
> +       case RDMA_CM_EVENT_DEVICE_REMOVAL:
> +               log_rdma_event("connected event=%d\n", event->event);
> +               info->connect_state = event->event;
> +               break;
> +
> +       case RDMA_CM_EVENT_DISCONNECTED:
> +               break;
> +
> +       default:
> +               break;
> +       }
> +
> +       return 0;
> +}

This code looks a lot like the connection stuff in the NFS/RDMA RPC transport.
Does your code have the same needs? If so, you might consider moving this to
a common RDMA handler.

> +/* Upcall from RDMA QP */
> +static void
> +cifs_rdma_qp_async_error_upcall(struct ib_event *event, void *context)
> +{
> +       struct cifs_rdma_info *info = context;
> +       log_rdma_event("%s on device %s info %p\n",
> +               ib_event_msg(event->event), event->device->name, info);
> +
> +       switch (event->event)
> +       {
> +       case IB_EVENT_CQ_ERR:
> +       case IB_EVENT_QP_FATAL:
> +       case IB_EVENT_QP_REQ_ERR:
> +       case IB_EVENT_QP_ACCESS_ERR:
> +
> +       default:
> +               break;
> +       }
> +}

Ditto. But, what's up with the empty switch(event->event) processing?

> +static struct rdma_cm_id* cifs_rdma_create_id(
> +               struct cifs_rdma_info *info, struct sockaddr *dstaddr)
> +{
...
> +       log_rdma_event("connecting to IP %pI4 port %d\n",
> +               &addr_in->sin_addr, ntohs(addr_in->sin_port));
>... and then...
> +       if (dstaddr->sa_family == AF_INET6)
> +               sport = &((struct sockaddr_in6 *)dstaddr)->sin6_port;
> +       else
> +               sport = &((struct sockaddr_in *)dstaddr)->sin_port;
> +
> +       *sport = htons(445);
...and
> +out:
> +       // try port number 5445 if port 445 doesn't work
> +       if (*sport == htons(445)) {
> +               *sport = htons(5445);
> +               goto try_again;
> +       }

Suggest rearranging the log_rdma_event() call to reflect reality.

The IANA-assigned port for SMB Direct is 5445, and port 445 will be
listening on TCP. Should you really be probing that port before 5445?
I suggest not doing so unconditionally.

> +struct cifs_rdma_info* cifs_create_rdma_session(
> +       struct TCP_Server_Info *server, struct sockaddr *dstaddr)
> +{
> ...
> +       int max_pending = receive_credit_max + send_credit_target;
>...
> +       if (max_pending > info->id->device->attrs.max_cqe ||
> +           max_pending > info->id->device->attrs.max_qp_wr) {
> +               log_rdma_event("consider lowering receive_credit_max and "
> +                       "send_credit_target. Possible CQE overrun, device "
> +                       "reporting max_cpe %d max_qp_wr %d\n",
> +                       info->id->device->attrs.max_cqe,
> +                       info->id->device->attrs.max_qp_wr);
> +               goto out2;
> +       }

I don't understand this. Why are you directing both Receive and Send completions
to the same CQ, won't that make it very hard to manage completions and their
interrupts? Also, what device(s) have you seen trigger this log? CQ's are generally
allowed to be quite large.

> +       conn_param.responder_resources = 32;
> +       if (info->id->device->attrs.max_qp_rd_atom < 32)
> +               conn_param.responder_resources =
> +                       info->id->device->attrs.max_qp_rd_atom;
> +       conn_param.retry_count = 6;
> +       conn_param.rnr_retry_count = 6;
> +       conn_param.flow_control = 0;

These choices warrant explanation. 32 responder resources is on the large side for
most datacenter networks. And because of SMB Direct's credits, why is RNR_retry not
simply zero?

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

#1702452 — [[PATCH v1] 17/37] [CIFS] SMBD: Track status for transport

From: Long Li <longli@microsoft.com>

Introduce status for tracking the status of SMBD transport. They are used in transport reconnect and shutdown.

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

diff --git a/fs/cifs/cifsrdma.c b/fs/cifs/cifsrdma.c
index cf71bb1..ef21f1c 100644
--- a/fs/cifs/cifsrdma.c
+++ b/fs/cifs/cifsrdma.c
@@ -173,9 +173,12 @@ static int cifs_rdma_conn_upcall(
 	case RDMA_CM_EVENT_DEVICE_REMOVAL:
 		log_rdma_event("connected event=%d\n", event->event);
 		info->connect_state = event->event;
+		info->transport_status = CIFS_RDMA_CONNECTED;
+		wake_up_all(&info->conn_wait);
 		break;
 
 	case RDMA_CM_EVENT_DISCONNECTED:
+		info->transport_status = CIFS_RDMA_DISCONNECTED;
 		break;
 
 	default:
@@ -581,6 +584,12 @@ static int cifs_rdma_post_send_page(struct cifs_rdma_info *info, struct page *pa
 	int rc = -ENOMEM;
 	int i;
 
+	// disconnected?
+	if (info->transport_status != CIFS_RDMA_CONNECTED) {
+		log_outgoing("disconnected not sending\n");
+		return -ENOENT;
+	}
+
 	request = mempool_alloc(info->request_mempool, GFP_KERNEL);
 	if (!request)
 		return rc;
@@ -686,6 +695,12 @@ static int cifs_rdma_post_send_empty(struct cifs_rdma_info *info)
 	int rc;
 	u16 credits_granted, flags=0;
 
+	// disconnected?
+	if (info->transport_status != CIFS_RDMA_CONNECTED) {
+		log_outgoing("disconnected not sending\n");
+		return -ENOENT;
+	}
+
 	request = mempool_alloc(info->request_mempool, GFP_KERNEL);
 	if (!request) {
 		log_rdma_send("failed to get send buffer for empty packet\n");
@@ -785,6 +800,12 @@ static int cifs_rdma_post_send_data(
 	int rc = -ENOMEM, i;
 	u32 data_length;
 
+	// disconnected?
+	if (info->transport_status != CIFS_RDMA_CONNECTED) {
+		log_outgoing("disconnected not sending\n");
+		return -ENOENT;
+	}
+
 	request = mempool_alloc(info->request_mempool, GFP_KERNEL);
 	if (!request)
 		return rc;
@@ -1056,6 +1077,7 @@ struct cifs_rdma_info* cifs_create_rdma_session(
 		return NULL;
 
 	info->server_info = server;
+	info->transport_status = CIFS_RDMA_CONNECTING;
 
 	rc = cifs_rdma_ia_open(info, dstaddr);
 	if (rc) {
@@ -1122,12 +1144,15 @@ struct cifs_rdma_info* cifs_create_rdma_session(
 	conn_param.retry_count = 6;
 	conn_param.rnr_retry_count = 6;
 	conn_param.flow_control = 0;
+	init_waitqueue_head(&info->conn_wait);
 	rc = rdma_connect(info->id, &conn_param);
 	if (rc) {
 		log_rdma_event("rdma_connect() failed with %i\n", rc);
 		goto out2;
 	}
 
+	wait_event_interruptible(
+		info->conn_wait, info->transport_status == CIFS_RDMA_CONNECTED);
 	if (info->connect_state != RDMA_CM_EVENT_ESTABLISHED)
 		goto out2;
 
diff --git a/fs/cifs/cifsrdma.h b/fs/cifs/cifsrdma.h
index 4a4c191..9618e0b 100644
--- a/fs/cifs/cifsrdma.h
+++ b/fs/cifs/cifsrdma.h
@@ -25,6 +25,15 @@
 #include <rdma/rdma_cm.h>
 #include <linux/mempool.h>
 
+enum cifs_rdma_transport_status {
+	CIFS_RDMA_CREATED,
+	CIFS_RDMA_CONNECTING,
+	CIFS_RDMA_CONNECTED,
+	CIFS_RDMA_DISCONNECTING,
+	CIFS_RDMA_DISCONNECTED,
+	CIFS_RDMA_DESTROYED
+};
+
 /*
  * The context for the SMBDirect transport
  * Everything related to the transport is here. It has several logical parts
@@ -35,6 +44,7 @@
  */
 struct cifs_rdma_info {
 	struct TCP_Server_Info *server_info;
+	enum cifs_rdma_transport_status transport_status;
 
 	// RDMA related
 	struct rdma_cm_id *id;
@@ -45,6 +55,7 @@ struct cifs_rdma_info {
 	int connect_state;
 	int ri_rc;
 	struct completion ri_done;
+	wait_queue_head_t conn_wait;
 
 	struct completion negotiate_completion;
 	bool negotiate_done;
-- 
2.7.4

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

#1710474

Hi Long,

a few meta-comments:

first the split into lots of tiny lists makes the series really hard
to read, I'd split it into just a few:

 Patch 1: add the protocol constants
 Patch 2-n: core cifs modifcation required to implement SMBD
 Patch n+1: add the actual SMBD code
 Patch n+2..m: write up the core CIFS code to use it.

stcond a lot of the code doesn't seem to follow the usual Linux style.
It might be a good idea to run it through scripts/checkpach.pl and
at least fix the errors it generates.  The warnings are much more
opinioned so feel free to ignore them if they sound odd for now.

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

#1710476

... and a third one:

please include the linux-rdma mailing list on your next post, to make
sure we get a good review pool of people familiar with the rdma code.

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

#1711257

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Sunday, August 13, 2017 3:28 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] 00/37] Implement SMBD protocol: Series 1
> 
> Hi Long,
> 
> a few meta-comments:
> 
> first the split into lots of tiny lists makes the series really hard to read, I'd split
> it into just a few:
> 
>  Patch 1: add the protocol constants
>  Patch 2-n: core cifs modifcation required to implement SMBD  Patch n+1:
> add the actual SMBD code  Patch n+2..m: write up the core CIFS code to use
> it.
> 
> stcond a lot of the code doesn't seem to follow the usual Linux style.
> It might be a good idea to run it through scripts/checkpach.pl and at least fix
> the errors it generates.  The warnings are much more opinioned so feel free
> to ignore them if they sound odd for now.

Hi Christoph

Thank you for reviewing the patch set.

I'll address those comments and update the patches.

Long

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web