RPC/RDMA patchset for next merge window

Internal RPC/RDMA structure updates in preparation for FRMR support.

Signed-off-by Tom Tucker <[email protected]>

Signed-off-by Tom Talpey <[email protected]>

---

 net/sunrpc/xprtrdma/xprt_rdma.h |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

include/linux/sunrpc/xprtrdma.h +1 -0

index 4de56b1..55a5d92 100644
@@ -78,6 +78,7 @@ enum rpcrdma_memreg {
 	RPCRDMA_MEMWINDOWS,
 	RPCRDMA_MEMWINDOWS_ASYNC,
 	RPCRDMA_MTHCAFMR,
+	RPCRDMA_FRMR,
 	RPCRDMA_ALLPHYSICAL,
 	RPCRDMA_LAST
 };

net/sunrpc/xprtrdma/xprt_rdma.h +7 -2

index 2427822..05b7898 100644
@@ -58,6 +58,8 @@ struct rpcrdma_ia {
 	struct rdma_cm_id 	*ri_id;
 	struct ib_pd		*ri_pd;
 	struct ib_mr		*ri_bind_mem;
+	u32			ri_dma_lkey;
+	int			ri_have_dma_lkey;
 	struct completion	ri_done;
 	int			ri_async_rc;
 	enum rpcrdma_memreg	ri_memreg_strategy;

@@ -156,6 +158,10 @@ struct rpcrdma_mr_seg {		/* chunk descriptors */
 			union {
 				struct ib_mw	*mw;
 				struct ib_fmr	*fmr;
+				struct {
+					struct ib_fast_reg_page_list *fr_pgl;
+					struct ib_mr *fr_mr;
+				} frmr;
 			} r;
 			struct list_head mw_list;
 		} *rl_mw;

@@ -198,7 +204,7 @@ struct rpcrdma_buffer {
 	atomic_t	rb_credits;	/* most recent server credits */
 	unsigned long	rb_cwndscale;	/* cached framework rpc_cwndscale */
 	int		rb_max_requests;/* client max requests */
-	struct list_head rb_mws;	/* optional memory windows/fmrs */
+	struct list_head rb_mws;	/* optional memory windows/fmrs/frmrs */
 	int		rb_send_index;
 	struct rpcrdma_req	**rb_send_bufs;
 	int		rb_recv_index;
 
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · Tom Talpey

Internal RPC/RDMA structure updates in preparation for FRMR support.

Signed-off-by Tom Tucker <[email protected]>

Signed-off-by Tom Talpey <[email protected]>

Shouldn't there be a

From: Tom Tucker <[email protected]>

at the top of this email in order to indicate that Tom Tucker is the author?

Review

Quoted 7 lines

---

 net/sunrpc/xprtrdma/xprt_rdma.h |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

(quoted diff) +0 -0

b/include/linux/sunrpc/xprtrdma.h

Review

Quoted 12 lines

index 4de56b1..55a5d92 100644

include/linux/sunrpc/xprtrdma.h +1 -0

@@ -78,6 +78,7 @@ enum rpcrdma_memreg {
 	RPCRDMA_MEMWINDOWS,
 	RPCRDMA_MEMWINDOWS_ASYNC,
 	RPCRDMA_MTHCAFMR,
+	RPCRDMA_FRMR,
 	RPCRDMA_ALLPHYSICAL,
 	RPCRDMA_LAST
 };

(quoted diff) +0 -0

b/net/sunrpc/xprtrdma/xprt_rdma.h

Quoted 37 lines

index 2427822..05b7898 100644

net/sunrpc/xprtrdma/xprt_rdma.h +7 -2

@@ -58,6 +58,8 @@ struct rpcrdma_ia {
 	struct rdma_cm_id 	*ri_id;
 	struct ib_pd		*ri_pd;
 	struct ib_mr		*ri_bind_mem;
+	u32			ri_dma_lkey;
+	int			ri_have_dma_lkey;
 	struct completion	ri_done;
 	int			ri_async_rc;
 	enum rpcrdma_memreg	ri_memreg_strategy;

@@ -156,6 +158,10 @@ struct rpcrdma_mr_seg {		/* chunk descriptors */
 			union {
 				struct ib_mw	*mw;
 				struct ib_fmr	*fmr;
+				struct {
+					struct ib_fast_reg_page_list *fr_pgl;
+					struct ib_mr *fr_mr;
+				} frmr;
 			} r;
 			struct list_head mw_list;
 		} *rl_mw;

@@ -198,7 +204,7 @@ struct rpcrdma_buffer {
 	atomic_t	rb_credits;	/* most recent server credits */
 	unsigned long	rb_cwndscale;	/* cached framework rpc_cwndscale */
 	int		rb_max_requests;/* client max requests */
-	struct list_head rb_mws;	/* optional memory windows/fmrs */
+	struct list_head rb_mws;	/* optional memory windows/fmrs/frmrs */
 	int		rb_send_index;
 	struct rpcrdma_req	**rb_send_bufs;
 	int		rb_recv_index;
 
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 6 lines · Trond Myklebust

Quoted 4 lines · Tom Talpey

Internal RPC/RDMA structure updates in preparation for FRMR support.

Signed-off-by Tom Tucker <[email protected]>

Signed-off-by Tom Talpey <[email protected]>

Quoted 6 lines

Shouldn't there be a

From: Tom Tucker <[email protected]>

at the top of this email in order to indicate that Tom Tucker is the author?

Co-author. Should it have two From lines?

Quoted 6 lines · Trond Myklebust

Quoted 4 lines · Tom Talpey

Internal RPC/RDMA structure updates in preparation for FRMR support.

Signed-off-by Tom Tucker <[email protected]>

Signed-off-by Tom Talpey <[email protected]>

Quoted 6 lines

Shouldn't there be a

From: Tom Tucker <[email protected]>

at the top of this email in order to indicate that Tom Tucker is the author?

Co-author. Should it have two From lines?

Tom.

Quoted 54 lines

Review

Quoted 7 lines

---

 net/sunrpc/xprtrdma/xprt_rdma.h |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

(quoted diff) +0 -0

b/include/linux/sunrpc/xprtrdma.h

Review

Quoted 12 lines

index 4de56b1..55a5d92 100644

include/linux/sunrpc/xprtrdma.h +1 -0

@@ -78,6 +78,7 @@ enum rpcrdma_memreg {
 	RPCRDMA_MEMWINDOWS,
 	RPCRDMA_MEMWINDOWS_ASYNC,
 	RPCRDMA_MTHCAFMR,
+	RPCRDMA_FRMR,
 	RPCRDMA_ALLPHYSICAL,
 	RPCRDMA_LAST
 };

(quoted diff) +0 -0

b/net/sunrpc/xprtrdma/xprt_rdma.h

Quoted 33 lines

index 2427822..05b7898 100644

net/sunrpc/xprtrdma/xprt_rdma.h +7 -1

@@ -58,6 +58,8 @@ struct rpcrdma_ia {
 	struct rdma_cm_id 	*ri_id;
 	struct ib_pd		*ri_pd;
 	struct ib_mr		*ri_bind_mem;
+	u32			ri_dma_lkey;
+	int			ri_have_dma_lkey;
 	struct completion	ri_done;
 	int			ri_async_rc;
 	enum rpcrdma_memreg	ri_memreg_strategy;

@@ -156,6 +158,10 @@ struct rpcrdma_mr_seg {		/* chunk descriptors */
 			union {
 				struct ib_mw	*mw;
 				struct ib_fmr	*fmr;
+				struct {
+					struct ib_fast_reg_page_list *fr_pgl;
+					struct ib_mr *fr_mr;
+				} frmr;
 			} r;
 			struct list_head mw_list;
 		} *rl_mw;

@@ -198,7 +204,7 @@ struct rpcrdma_buffer {
 	atomic_t	rb_credits;	/* most recent server credits */
 	unsigned long	rb_cwndscale;	/* cached framework rpc_cwndscale */
 	int		rb_max_requests;/* client max requests */
-	struct list_head rb_mws;	/* optional memory windows/fmrs */
+	struct list_head rb_mws;	/* optional memory windows/fmrs/frmrs */
 	int		rb_send_index;
 	struct rpcrdma_req	**rb_send_bufs;
 	int		rb_recv_index;

Quoted 4 lines

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines · Thomas

Quoted 8 lines · Trond Myklebust

Quoted 4 lines · Tom Talpey

Internal RPC/RDMA structure updates in preparation for FRMR support.

Signed-off-by Tom Tucker <[email protected]>

Signed-off-by Tom Talpey <[email protected]>

Shouldn't there be a

Quoted 6 lines

Quoted 4 lines

From: Tom Tucker <[email protected]>

at the top of this email in order to indicate that Tom Tucker is the author?

Co-author. Should it have two From lines?

Gotta pick one. I tend to leave whoever got there first as the author.

If it was a pretty involved collaboration I suppose you could even do something cheesy like assinging half the series to one person and half to the other.

--b.

Quoted 66 lines

Tom.

Quoted 54 lines

Review

Quoted 7 lines

---

 net/sunrpc/xprtrdma/xprt_rdma.h |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

(quoted diff) +0 -0

b/include/linux/sunrpc/xprtrdma.h

Review

Quoted 12 lines

index 4de56b1..55a5d92 100644

include/linux/sunrpc/xprtrdma.h +1 -0

@@ -78,6 +78,7 @@ enum rpcrdma_memreg {
 	RPCRDMA_MEMWINDOWS,
 	RPCRDMA_MEMWINDOWS_ASYNC,
 	RPCRDMA_MTHCAFMR,
+	RPCRDMA_FRMR,
 	RPCRDMA_ALLPHYSICAL,
 	RPCRDMA_LAST
 };

(quoted diff) +0 -0

b/net/sunrpc/xprtrdma/xprt_rdma.h

Quoted 33 lines

index 2427822..05b7898 100644

net/sunrpc/xprtrdma/xprt_rdma.h +7 -1

@@ -58,6 +58,8 @@ struct rpcrdma_ia {
 	struct rdma_cm_id 	*ri_id;
 	struct ib_pd		*ri_pd;
 	struct ib_mr		*ri_bind_mem;
+	u32			ri_dma_lkey;
+	int			ri_have_dma_lkey;
 	struct completion	ri_done;
 	int			ri_async_rc;
 	enum rpcrdma_memreg	ri_memreg_strategy;

@@ -156,6 +158,10 @@ struct rpcrdma_mr_seg {		/* chunk descriptors */
 			union {
 				struct ib_mw	*mw;
 				struct ib_fmr	*fmr;
+				struct {
+					struct ib_fast_reg_page_list *fr_pgl;
+					struct ib_mr *fr_mr;
+				} frmr;
 			} r;
 			struct list_head mw_list;
 		} *rl_mw;

@@ -198,7 +204,7 @@ struct rpcrdma_buffer {
 	atomic_t	rb_credits;	/* most recent server credits */
 	unsigned long	rb_cwndscale;	/* cached framework rpc_cwndscale */
 	int		rb_max_requests;/* client max requests */
-	struct list_head rb_mws;	/* optional memory windows/fmrs */
+	struct list_head rb_mws;	/* optional memory windows/fmrs/frmrs */
 	int		rb_send_index;
 	struct rpcrdma_req	**rb_send_bufs;
 	int		rb_recv_index;

Quoted 4 lines

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

At transport creation, check for, and use, any local dma lkey. Then, check that the selected memory registration mode is in fact supported by the RDMA adapter selected for the mount. Fall back to best alternative if not.

Signed-off-by Tom Talpey <[email protected]>

Signed-off-by Tom Tucker <[email protected]>

---

 net/sunrpc/xprtrdma/verbs.c |   95 ++++++++++++++++++++++++++++++++++++-------
 1 files changed, 80 insertions(+), 15 deletions(-)

net/sunrpc/xprtrdma/verbs.c +80 -16

index d04208a..0f3b431 100644
@@ -423,7 +423,8 @@ rpcrdma_clean_cq(struct ib_cq *cq)
 int
 rpcrdma_ia_open(struct rpcrdma_xprt *xprt, struct sockaddr *addr, int memreg)
 {
-	int rc;
+	int rc, mem_priv;
+	struct ib_device_attr devattr;
 	struct rpcrdma_ia *ia = &xprt->rx_ia;
 
 	init_completion(&ia->ri_done);

@@ -443,6 +444,53 @@ rpcrdma_ia_open(struct rpcrdma_xprt *xprt, struct sockaddr *addr, int memreg)
 	}
 
 	/*
+	 * Query the device to determine if the requested memory
+	 * registration strategy is supported. If it isn't, set the
+	 * strategy to a globally supported model.
+	 */
+	rc = ib_query_device(ia->ri_id->device, &devattr);
+	if (rc) {
+		dprintk("RPC:       %s: ib_query_device failed %d\n",
+			__func__, rc);
+		goto out2;
+	}
+
+	if (devattr.device_cap_flags & IB_DEVICE_LOCAL_DMA_LKEY) {
+		ia->ri_have_dma_lkey = 1;
+		ia->ri_dma_lkey = ia->ri_id->device->local_dma_lkey;
+	}
+
+	switch (memreg) {
+	case RPCRDMA_MEMWINDOWS:
+	case RPCRDMA_MEMWINDOWS_ASYNC:
+		if (!(devattr.device_cap_flags & IB_DEVICE_MEM_WINDOW)) {
+			dprintk("RPC:       %s: MEMWINDOWS registration "
+				"specified but not supported by adapter, "
+				"using slower RPCRDMA_REGISTER\n",
+				__func__);
+			memreg = RPCRDMA_REGISTER;
+		}
+		break;
+	case RPCRDMA_MTHCAFMR:
+		if (!ia->ri_id->device->alloc_fmr) {
+#if RPCRDMA_PERSISTENT_REGISTRATION
+			dprintk("RPC:       %s: MTHCAFMR registration "
+				"specified but not supported by adapter, "
+				"using riskier RPCRDMA_ALLPHYSICAL\n",
+				__func__);
+			memreg = RPCRDMA_ALLPHYSICAL;
+#else
+			dprintk("RPC:       %s: MTHCAFMR registration "
+				"specified but not supported by adapter, "
+				"using slower RPCRDMA_REGISTER\n",
+				__func__);
+			memreg = RPCRDMA_REGISTER;
+#endif
+		}
+		break;
+	}
+
+	/*
 	 * Optionally obtain an underlying physical identity mapping in
 	 * order to do a memory window-based bind. This base registration
 	 * is protected from remote access - that is enabled only by binding

@@ -450,22 +498,27 @@ rpcrdma_ia_open(struct rpcrdma_xprt *xprt, struct sockaddr *addr, int memreg)
 	 * revoked after the corresponding completion similar to a storage
 	 * adapter.
 	 */
-	if (memreg > RPCRDMA_REGISTER) {
-		int mem_priv = IB_ACCESS_LOCAL_WRITE;
-		switch (memreg) {
+	switch (memreg) {
+	case RPCRDMA_BOUNCEBUFFERS:
+	case RPCRDMA_REGISTER:
+		break;
 #if RPCRDMA_PERSISTENT_REGISTRATION
-		case RPCRDMA_ALLPHYSICAL:
-			mem_priv |= IB_ACCESS_REMOTE_WRITE;
-			mem_priv |= IB_ACCESS_REMOTE_READ;
-			break;
+	case RPCRDMA_ALLPHYSICAL:
+		mem_priv = IB_ACCESS_LOCAL_WRITE |
+				IB_ACCESS_REMOTE_WRITE |
+				IB_ACCESS_REMOTE_READ;
+		goto register_setup;
 #endif
-		case RPCRDMA_MEMWINDOWS_ASYNC:
-		case RPCRDMA_MEMWINDOWS:
-			mem_priv |= IB_ACCESS_MW_BIND;
-			break;
-		default:
+	case RPCRDMA_MEMWINDOWS_ASYNC:
+	case RPCRDMA_MEMWINDOWS:
+		mem_priv = IB_ACCESS_LOCAL_WRITE |
+				IB_ACCESS_MW_BIND;
+		goto register_setup;
+	case RPCRDMA_MTHCAFMR:
+		if (ia->ri_have_dma_lkey)
 			break;
-		}
+		mem_priv = IB_ACCESS_LOCAL_WRITE;
+	register_setup:
 		ia->ri_bind_mem = ib_get_dma_mr(ia->ri_pd, mem_priv);
 		if (IS_ERR(ia->ri_bind_mem)) {
 			printk(KERN_ALERT "%s: ib_get_dma_mr for "

@@ -475,7 +528,15 @@ rpcrdma_ia_open(struct rpcrdma_xprt *xprt, struct sockaddr *addr, int memreg)
 			memreg = RPCRDMA_REGISTER;
 			ia->ri_bind_mem = NULL;
 		}
+		break;
+	default:
+		printk(KERN_ERR "%s: invalid memory registration mode %d\n",
+				__func__, memreg);
+		rc = -EINVAL;
+		goto out2;
 	}
+	dprintk("RPC:       %s: memory registration strategy is %d\n",
+		__func__, memreg);
 
 	/* Else will do memory reg/dereg for each chunk */
 	ia->ri_memreg_strategy = memreg;

@@ -1248,7 +1309,11 @@ rpcrdma_register_internal(struct rpcrdma_ia *ia, void *va, int len,
 			va, len, DMA_BIDIRECTIONAL);
 	iov->length = len;
 
-	if (ia->ri_bind_mem != NULL) {
+	if (ia->ri_have_dma_lkey) {
+		*mrp = NULL;
+		iov->lkey = ia->ri_dma_lkey;
+		return 0;
+	} else if (ia->ri_bind_mem != NULL) {
 		*mrp = NULL;
 		iov->lkey = ia->ri_bind_mem->lkey;
 		return 0;
 
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · Tom Talpey

At transport creation, check for, and use, any local dma lkey. Then, check that the selected memory registration mode is in fact supported by the RDMA adapter selected for the mount. Fall back to best alternative if not.

Quoted 2 lines

Signed-off-by Tom Talpey <[email protected]>

Signed-off-by Tom Tucker <[email protected]>

I'm confused... Who is signing off on what? AFAICS, Tom Talpey is the author and is the one sending this patch series. Where does Tom Tucker come into the picture?

Quoted 4 lines

---

 net/sunrpc/xprtrdma/verbs.c |   95 

++++++++++++++++++++++++++++++++++++-------

net/sunrpc/xprtrdma/verbs.c rpcrdma_clean_cq(struct ib_cq *cq) · line 424

Quoted 9 lines

 1 files changed, 80 insertions(+), 15 deletions(-)

net/sunrpc/xprtrdma/verbs.c +0 -0

index d04208a..0f3b431 100644
@@ -423,7 +423,8 @@ rpcrdma_clean_cq(struct ib_cq *cq)
 int
 rpcrdma_ia_open(struct rpcrdma_xprt *xprt, struct sockaddr *addr, 

int memreg)

net/sunrpc/xprtrdma/verbs.c rpcrdma_ia_open(struct rpcrdma_xprt *xprt,

Quoted 8 lines

net/sunrpc/xprtrdma/verbs.c +2 -1

 {
-	int rc;
+	int rc, mem_priv;
+	struct ib_device_attr devattr;
 	struct rpcrdma_ia *ia = &xprt->rx_ia;
 
 	init_completion(&ia->ri_done);

@@ -443,6 +444,53 @@ rpcrdma_ia_open(struct rpcrdma_xprt *xprt, 

struct sockaddr *addr, int memreg)

net/sunrpc/xprtrdma/verbs.c rpcrdma_ia_open(struct rpcrdma_xprt *xprt,

Quoted 54 lines

net/sunrpc/xprtrdma/verbs.c +47 -0

 	}
 
 	/*
+	* Query the device to determine if the requested memory
+	* registration strategy is supported. If it isn't, set the
+	* strategy to a globally supported model.
+	*/
+	rc = ib_query_device(ia->ri_id->device, &devattr);
+	if (rc) {
+		dprintk("RPC:       %s: ib_query_device failed %d\n",
+			__func__, rc);
+		goto out2;
+	}
+
+	if (devattr.device_cap_flags & IB_DEVICE_LOCAL_DMA_LKEY) {
+		ia->ri_have_dma_lkey = 1;
+		ia->ri_dma_lkey = ia->ri_id->device->local_dma_lkey;
+	}
+
+	switch (memreg) {
+	case RPCRDMA_MEMWINDOWS:
+	case RPCRDMA_MEMWINDOWS_ASYNC:
+		if (!(devattr.device_cap_flags & IB_DEVICE_MEM_WINDOW)) {
+			dprintk("RPC:       %s: MEMWINDOWS registration "
+				"specified but not supported by adapter, "
+				"using slower RPCRDMA_REGISTER\n",
+				__func__);
+			memreg = RPCRDMA_REGISTER;
+		}
+		break;
+	case RPCRDMA_MTHCAFMR:
+		if (!ia->ri_id->device->alloc_fmr) {
+#if RPCRDMA_PERSISTENT_REGISTRATION
+			dprintk("RPC:       %s: MTHCAFMR registration "
+				"specified but not supported by adapter, "
+				"using riskier RPCRDMA_ALLPHYSICAL\n",
+				__func__);
+			memreg = RPCRDMA_ALLPHYSICAL;
+#else
+			dprintk("RPC:       %s: MTHCAFMR registration "
+				"specified but not supported by adapter, "
+				"using slower RPCRDMA_REGISTER\n",
+				__func__);
+			memreg = RPCRDMA_REGISTER;
+#endif
+		}
+		break;
+	}
+
+	/*
 	* Optionally obtain an underlying physical identity mapping in
 	* order to do a memory window-based bind. This base registration
 	* is protected from remote access - that is enabled only by binding

@@ -450,22 +498,27 @@ rpcrdma_ia_open(struct rpcrdma_xprt *xprt, 

struct sockaddr *addr, int memreg)

net/sunrpc/xprtrdma/verbs.c rpcrdma_ia_open(struct rpcrdma_xprt *xprt,

Quoted 41 lines

net/sunrpc/xprtrdma/verbs.c +18 -13

 	* revoked after the corresponding completion similar to a storage
 	* adapter.
 	*/
-	if (memreg > RPCRDMA_REGISTER) {
-		int mem_priv = IB_ACCESS_LOCAL_WRITE;
-		switch (memreg) {
+	switch (memreg) {
+	case RPCRDMA_BOUNCEBUFFERS:
+	case RPCRDMA_REGISTER:
+		break;
 #if RPCRDMA_PERSISTENT_REGISTRATION
-		case RPCRDMA_ALLPHYSICAL:
-			mem_priv |= IB_ACCESS_REMOTE_WRITE;
-			mem_priv |= IB_ACCESS_REMOTE_READ;
-			break;
+	case RPCRDMA_ALLPHYSICAL:
+		mem_priv = IB_ACCESS_LOCAL_WRITE |
+				IB_ACCESS_REMOTE_WRITE |
+				IB_ACCESS_REMOTE_READ;
+		goto register_setup;
 #endif
-		case RPCRDMA_MEMWINDOWS_ASYNC:
-		case RPCRDMA_MEMWINDOWS:
-			mem_priv |= IB_ACCESS_MW_BIND;
-			break;
-		default:
+	case RPCRDMA_MEMWINDOWS_ASYNC:
+	case RPCRDMA_MEMWINDOWS:
+		mem_priv = IB_ACCESS_LOCAL_WRITE |
+				IB_ACCESS_MW_BIND;
+		goto register_setup;
+	case RPCRDMA_MTHCAFMR:
+		if (ia->ri_have_dma_lkey)
 			break;
-		}
+		mem_priv = IB_ACCESS_LOCAL_WRITE;
+	register_setup:
 		ia->ri_bind_mem = ib_get_dma_mr(ia->ri_pd, mem_priv);
 		if (IS_ERR(ia->ri_bind_mem)) {
 			printk(KERN_ALERT "%s: ib_get_dma_mr for "

@@ -475,7 +528,15 @@ rpcrdma_ia_open(struct rpcrdma_xprt *xprt, 

struct sockaddr *addr, int memreg)

net/sunrpc/xprtrdma/verbs.c rpcrdma_register_internal(struct rpcrdma_ia

Quoted 16 lines

net/sunrpc/xprtrdma/verbs.c +8 -0

 			memreg = RPCRDMA_REGISTER;
 			ia->ri_bind_mem = NULL;
 		}
+		break;
+	default:
+		printk(KERN_ERR "%s: invalid memory registration mode %d\n",
+				__func__, memreg);
+		rc = -EINVAL;
+		goto out2;
 	}
+	dprintk("RPC:       %s: memory registration strategy is %d\n",
+		__func__, memreg);
 
 	/* Else will do memory reg/dereg for each chunk */
 	ia->ri_memreg_strategy = memreg;

@@ -1248,7 +1309,11 @@ rpcrdma_register_internal(struct rpcrdma_ia 

*ia, void *va, int len,

Quoted 17 lines

net/sunrpc/xprtrdma/verbs.c +5 -2

 			va, len, DMA_BIDIRECTIONAL);
 	iov->length = len;
 
-	if (ia->ri_bind_mem != NULL) {
+	if (ia->ri_have_dma_lkey) {
+		*mrp = NULL;
+		iov->lkey = ia->ri_dma_lkey;
+		return 0;
+	} else if (ia->ri_bind_mem != NULL) {
 		*mrp = NULL;
 		iov->lkey = ia->ri_bind_mem->lkey;
 		return 0;
 
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines · Trond Myklebust

Quoted 7 lines · Tom Talpey

At transport creation, check for, and use, any local dma lkey. Then, check that the selected memory registration mode is in fact supported by the RDMA adapter selected for the mount. Fall back to best alternative if not.

Signed-off-by Tom Talpey <[email protected]>

Signed-off-by Tom Tucker <[email protected]>

Quoted 3 lines

I'm confused... Who is signing off on what? AFAICS, Tom Talpey is the author and is the one sending this patch series. Where does Tom Tucker come into the picture?

Tom Tucker wrote the initial version of some of the FRMR series, he had emailed them to linux-nfs a while back. I left his sign-off in place on those, as a co-author, and then signed for my own contributions.

From: Tom Tucker <[email protected]>

This logic sets the connection parameter that configures the local device and informs the remote peer how many concurrent incoming RDMA_READ requests are supported. The original logic didn't really do what was intended for two reasons:

- The max number supported by the device is typically smaller than any one factor in the calculation used, and

- The field in the connection parameter structure where the value is stored is a u8 and always overflows for the default settings.

So what really happens is the value requested for responder resources is the left over 8 bits from the "desired value". If the desired value happened to be a multiple of 256, the result was zero and it wouldn't connect at all.

Given the above and the fact that max_requests is almost always larger than the max responder resources supported by the adapter, this patch simplifies this logic and simply requests the max supported by the device, subject to a reasonable limit.

This bug was found by Jim Schutt at Sandia.

Signed-off-by Tom Tucker <[email protected]>

Signed-off-by Tom Talpey <[email protected]>

---

 net/sunrpc/xprtrdma/verbs.c |   51 ++++++++++++-------------------------------
 1 files changed, 14 insertions(+), 37 deletions(-)

net/sunrpc/xprtrdma/verbs.c +14 -38

index 39a1652..e3fe905 100644
@@ -705,30 +705,13 @@ rpcrdma_ep_create(struct rpcrdma_ep *ep, struct rpcrdma_ia *ia,
 	ep->rep_remote_cma.private_data_len = 0;
 
 	/* Client offers RDMA Read but does not initiate */
-	switch (ia->ri_memreg_strategy) {
-	case RPCRDMA_BOUNCEBUFFERS:
+	ep->rep_remote_cma.initiator_depth = 0;
+	if (ia->ri_memreg_strategy == RPCRDMA_BOUNCEBUFFERS)
 		ep->rep_remote_cma.responder_resources = 0;
-		break;
-	case RPCRDMA_MTHCAFMR:
-	case RPCRDMA_REGISTER:
-	case RPCRDMA_FRMR:
-		ep->rep_remote_cma.responder_resources = cdata->max_requests *
-				(RPCRDMA_MAX_DATA_SEGS / 8);
-		break;
-	case RPCRDMA_MEMWINDOWS:
-	case RPCRDMA_MEMWINDOWS_ASYNC:
-#if RPCRDMA_PERSISTENT_REGISTRATION
-	case RPCRDMA_ALLPHYSICAL:
-#endif
-		ep->rep_remote_cma.responder_resources = cdata->max_requests *
-				(RPCRDMA_MAX_DATA_SEGS / 2);
-		break;
-	default:
-		break;
-	}
-	if (ep->rep_remote_cma.responder_resources > devattr.max_qp_rd_atom)
+	else if (devattr.max_qp_rd_atom > 32)	/* arbitrary but <= 255 */
+		ep->rep_remote_cma.responder_resources = 32;
+	else
 		ep->rep_remote_cma.responder_resources = devattr.max_qp_rd_atom;
-	ep->rep_remote_cma.initiator_depth = 0;
 
 	ep->rep_remote_cma.retry_count = 7;
 	ep->rep_remote_cma.flow_control = 0;

@@ -858,14 +841,6 @@ if (strnicmp(ia->ri_id->device->dma_device->bus->name, "pci", 3) == 0) {
 	}
 }
 
-	/* Theoretically a client initiator_depth > 0 is not needed,
-	 * but many peers fail to complete the connection unless they
-	 * == responder_resources! */
-	if (ep->rep_remote_cma.initiator_depth !=
-				ep->rep_remote_cma.responder_resources)
-		ep->rep_remote_cma.initiator_depth =
-			ep->rep_remote_cma.responder_resources;
-
 	ep->rep_connected = 0;
 
 	rc = rdma_connect(ia->ri_id, &ep->rep_remote_cma);

@@ -894,14 +869,16 @@ if (strnicmp(ia->ri_id->device->dma_device->bus->name, "pci", 3) == 0) {
 	if (ep->rep_connected <= 0) {
 		/* Sometimes, the only way to reliably connect to remote
 		 * CMs is to use same nonzero values for ORD and IRD. */
-		ep->rep_remote_cma.initiator_depth =
-					ep->rep_remote_cma.responder_resources;
-		if (ep->rep_remote_cma.initiator_depth == 0)
-			++ep->rep_remote_cma.initiator_depth;
-		if (ep->rep_remote_cma.responder_resources == 0)
-			++ep->rep_remote_cma.responder_resources;
-		if (retry_count++ == 0)
+		if (retry_count++ <= RDMA_CONNECT_RETRY_MAX + 1 &&
+		    (ep->rep_remote_cma.responder_resources == 0 ||
+		     ep->rep_remote_cma.initiator_depth !=
+				ep->rep_remote_cma.responder_resources)) {
+			if (ep->rep_remote_cma.responder_resources == 0)
+				ep->rep_remote_cma.responder_resources = 1;
+			ep->rep_remote_cma.initiator_depth =
+				ep->rep_remote_cma.responder_resources;
 			goto retry;
+		}
 		rc = ep->rep_connected;
 	} else {
 		dprintk("RPC:       %s: connected\n", __func__);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 1 line · Tom Talpey

From: Tom Tucker <[email protected]>

Now I'm really confused!

Quoted 22 lines

This logic sets the connection parameter that configures the local device and informs the remote peer how many concurrent incoming RDMA_READ requests are supported. The original logic didn't really do what was intended for two reasons:

- The max number supported by the device is typically smaller than any one factor in the calculation used, and

- The field in the connection parameter structure where the value is stored is a u8 and always overflows for the default settings.

So what really happens is the value requested for responder resources is the left over 8 bits from the "desired value". If the desired value happened to be a multiple of 256, the result was zero and it wouldn't connect at all.

Given the above and the fact that max_requests is almost always larger than the max responder resources supported by the adapter, this patch simplifies this logic and simply requests the max supported by the device, subject to a reasonable limit.

Quoted 7 lines

This bug was found by Jim Schutt at Sandia.

Signed-off-by Tom Tucker <[email protected]>

Signed-off-by Tom Talpey <[email protected]>

---

 net/sunrpc/xprtrdma/verbs.c |   51 

++++++++++++-------------------------------

net/sunrpc/xprtrdma/verbs.c rpcrdma_ep_create(struct rpcrdma_ep *ep,

Quoted 7 lines

 1 files changed, 14 insertions(+), 37 deletions(-)

net/sunrpc/xprtrdma/verbs.c +0 -0

index 39a1652..e3fe905 100644
@@ -705,30 +705,13 @@ rpcrdma_ep_create(struct rpcrdma_ep *ep, 

struct rpcrdma_ia *ia,

net/sunrpc/xprtrdma/verbs.c if

Quoted 36 lines

net/sunrpc/xprtrdma/verbs.c +5 -22

 	ep->rep_remote_cma.private_data_len = 0;
 
 	/* Client offers RDMA Read but does not initiate */
-	switch (ia->ri_memreg_strategy) {
-	case RPCRDMA_BOUNCEBUFFERS:
+	ep->rep_remote_cma.initiator_depth = 0;
+	if (ia->ri_memreg_strategy == RPCRDMA_BOUNCEBUFFERS)
 		ep->rep_remote_cma.responder_resources = 0;
-		break;
-	case RPCRDMA_MTHCAFMR:
-	case RPCRDMA_REGISTER:
-	case RPCRDMA_FRMR:
-		ep->rep_remote_cma.responder_resources = cdata->max_requests *
-				(RPCRDMA_MAX_DATA_SEGS / 8);
-		break;
-	case RPCRDMA_MEMWINDOWS:
-	case RPCRDMA_MEMWINDOWS_ASYNC:
-#if RPCRDMA_PERSISTENT_REGISTRATION
-	case RPCRDMA_ALLPHYSICAL:
-#endif
-		ep->rep_remote_cma.responder_resources = cdata->max_requests *
-				(RPCRDMA_MAX_DATA_SEGS / 2);
-		break;
-	default:
-		break;
-	}
-	if (ep->rep_remote_cma.responder_resources > devattr.max_qp_rd_atom)
+	else if (devattr.max_qp_rd_atom > 32)	/* arbitrary but <= 255 */
+		ep->rep_remote_cma.responder_resources = 32;
+	else
 		ep->rep_remote_cma.responder_resources = devattr.max_qp_rd_atom;
-	ep->rep_remote_cma.initiator_depth = 0;
 
 	ep->rep_remote_cma.retry_count = 7;
 	ep->rep_remote_cma.flow_control = 0;

@@ -858,14 +841,6 @@ if 

(strnicmp(ia->ri_id->device->dma_device->bus->name, "pci", 3) == 0) {

net/sunrpc/xprtrdma/verbs.c if

Quoted 15 lines

net/sunrpc/xprtrdma/verbs.c +0 -8

 	}
 }
 
-	/* Theoretically a client initiator_depth > 0 is not needed,
-	* but many peers fail to complete the connection unless they
-	* == responder_resources! */
-	if (ep->rep_remote_cma.initiator_depth !=
-				ep->rep_remote_cma.responder_resources)
-		ep->rep_remote_cma.initiator_depth =
-			ep->rep_remote_cma.responder_resources;
-
 	ep->rep_connected = 0;
 
 	rc = rdma_connect(ia->ri_id, &ep->rep_remote_cma);

@@ -894,14 +869,16 @@ if 

(strnicmp(ia->ri_id->device->dma_device->bus->name, "pci", 3) == 0) {

Quoted 28 lines

net/sunrpc/xprtrdma/verbs.c +9 -8

 	if (ep->rep_connected <= 0) {
 		/* Sometimes, the only way to reliably connect to remote
 		* CMs is to use same nonzero values for ORD and IRD. */
-		ep->rep_remote_cma.initiator_depth =
-					ep->rep_remote_cma.responder_resources;
-		if (ep->rep_remote_cma.initiator_depth == 0)
-			++ep->rep_remote_cma.initiator_depth;
-		if (ep->rep_remote_cma.responder_resources == 0)
-			++ep->rep_remote_cma.responder_resources;
-		if (retry_count++ == 0)
+		if (retry_count++ <= RDMA_CONNECT_RETRY_MAX + 1 &&
+		    (ep->rep_remote_cma.responder_resources == 0 ||
+		     ep->rep_remote_cma.initiator_depth !=
+				ep->rep_remote_cma.responder_resources)) {
+			if (ep->rep_remote_cma.responder_resources == 0)
+				ep->rep_remote_cma.responder_resources = 1;
+			ep->rep_remote_cma.initiator_depth =
+				ep->rep_remote_cma.responder_resources;
 			goto retry;
+		}
 		rc = ep->rep_connected;
 	} else {
 		dprintk("RPC:       %s: connected\n", __func__);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

The RPC/RDMA protocol allows clients and servers to avoid RDMA operations for data which is purely the result of XDR padding. On the client, automatically insert the necessary padding for such server replies, and optionally don't marshal such chunks.

Signed-off-by Tom Talpey <[email protected]>

---

 net/sunrpc/xprtrdma/rpc_rdma.c  |   22 ++++++++++++++++++++--
 net/sunrpc/xprtrdma/transport.c |    9 +++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

net/sunrpc/xprtrdma/rpc_rdma.c +0 -0

index 721dae7..c4b8011 100644

net/sunrpc/xprtrdma/rpc_rdma.c +17 -2

+		if (xdrbuf->tail[0].iov_len < 4 && xprt_rdma_pad_optimize)
+			return n;
 		if (n == nsegs)
 			return 0;
 		seg[n].mr_page = NULL;

@@ -594,7 +599,7 @@ rpcrdma_count_chunks(struct rpcrdma_rep *rep, unsigned int max, int wrchunk, __b
  * Scatter inline received data back into provided iov's.
  */
 static void
-rpcrdma_inline_fixup(struct rpc_rqst *rqst, char *srcp, int copy_len)
+rpcrdma_inline_fixup(struct rpc_rqst *rqst, char *srcp, int copy_len, int pad)
 {
 	int i, npages, curlen, olen;
 	char *destp;

@@ -660,6 +665,13 @@ rpcrdma_inline_fixup(struct rpc_rqst *rqst, char *srcp, int copy_len)
 	} else
 		rqst->rq_rcv_buf.tail[0].iov_len = 0;
 
+	if (pad) {
+		/* implicit padding on terminal chunk */
+		unsigned char *p = rqst->rq_rcv_buf.tail[0].iov_base;
+		while (pad--)
+			p[rqst->rq_rcv_buf.tail[0].iov_len++] = 0;
+	}
+
 	if (copy_len)
 		dprintk("RPC:       %s: %d bytes in"
 			" %d extra segments (%d lost)\n",

@@ -794,14 +806,20 @@ repost:
 			    ((unsigned char *)iptr - (unsigned char *)headerp);
 			status = rep->rr_len + rdmalen;
 			r_xprt->rx_stats.total_rdma_reply += rdmalen;
+			/* special case - last chunk may omit padding */
+			if (rdmalen &= 3) {
+				rdmalen = 4 - rdmalen;
+				status += rdmalen;
+			}
 		} else {
 			/* else ordinary inline */
+			rdmalen = 0;
 			iptr = (__be32 *)((unsigned char *)headerp + 28);
 			rep->rr_len -= 28; /*sizeof *headerp;*/
 			status = rep->rr_len;
 		}
 		/* Fix up the rpc results for upper layer */
-		rpcrdma_inline_fixup(rqst, (char *)iptr, rep->rr_len);
+		rpcrdma_inline_fixup(rqst, (char *)iptr, rep->rr_len, rdmalen);
 		break;
 
 	case __constant_htonl(RDMA_NOMSG):

net/sunrpc/xprtrdma/transport.c +9 -1

index ec6d1e7..c7d2380 100644
@@ -71,6 +71,7 @@ static unsigned int xprt_rdma_max_inline_read = RPCRDMA_DEF_INLINE;
 static unsigned int xprt_rdma_max_inline_write = RPCRDMA_DEF_INLINE;
 static unsigned int xprt_rdma_inline_write_padding;
 static unsigned int xprt_rdma_memreg_strategy = RPCRDMA_FRMR;
+                int xprt_rdma_pad_optimize = 0;
 
 #ifdef RPC_DEBUG
 

@@ -136,6 +137,14 @@ static ctl_table xr_tunables_table[] = {
 		.extra2		= &max_memreg,
 	},
 	{
+		.ctl_name       = CTL_UNNUMBERED,
+		.procname	= "rdma_pad_optimize",
+		.data		= &xprt_rdma_pad_optimize,
+		.maxlen		= sizeof(unsigned int),
+		.mode		= 0644,
+		.proc_handler	= &proc_dointvec,
+	},
+	{
 		.ctl_name = 0,
 	},
 };
 
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

net/sunrpc/xprtrdma/rpc_rdma.c rpcrdma_convert_iovs(struct xdr_buf *xdrbuf,

Quoted 17 lines · Tom Talpey

The RPC/RDMA protocol allows clients and servers to avoid RDMA operations for data which is purely the result of XDR padding. On the client, automatically insert the necessary padding for such server replies, and optionally don't marshal such chunks.

Signed-off-by Tom Talpey <[email protected]>

---

 net/sunrpc/xprtrdma/rpc_rdma.c  |   22 ++++++++++++++++++++--
 net/sunrpc/xprtrdma/transport.c |    9 +++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

net/sunrpc/xprtrdma/rpc_rdma.c +0 -0

index 721dae7..c4b8011 100644
@@ -118,6 +118,11 @@ rpcrdma_convert_iovs(struct xdr_buf *xdrbuf, 

unsigned int pos,

Quoted 2 lines

net/sunrpc/xprtrdma/rpc_rdma.c +0 -0

 	}
 

net/sunrpc/xprtrdma/rpc_rdma.c rpcrdma_convert_iovs(struct xdr_buf *xdrbuf, unsigned int pos, · line 123

Quoted 4 lines

net/sunrpc/xprtrdma/rpc_rdma.c +3 -0

 	if (xdrbuf->tail[0].iov_len) {
+		/* the rpcrdma protocol allows us to omit any trailing
+		* xdr pad bytes, saving the server an RDMA operation. */
+		extern int xprt_rdma_pad_optimize; /* 0 == old server compat */

                ^^^^^^^^^^^^^^^^^^^

Globals should really be declared in a header file. 'sparse' will complain if you don't...

net/sunrpc/xprtrdma/rpc_rdma.c rpcrdma_count_chunks(struct rpcrdma_rep *rep,

Quoted 7 lines

net/sunrpc/xprtrdma/rpc_rdma.c +2 -0

+		if (xdrbuf->tail[0].iov_len < 4 && xprt_rdma_pad_optimize)
+			return n;
 		if (n == nsegs)
 			return 0;
 		seg[n].mr_page = NULL;

@@ -594,7 +599,7 @@ rpcrdma_count_chunks(struct rpcrdma_rep *rep, 

unsigned int max, int wrchunk, __b

net/sunrpc/xprtrdma/rpc_rdma.c rpcrdma_count_chunks(struct rpcrdma_rep *rep, unsigned int max, int wrchunk, __b

Quoted 5 lines

net/sunrpc/xprtrdma/rpc_rdma.c +1 -1

  * Scatter inline received data back into provided iov's.
  */
 static void
-rpcrdma_inline_fixup(struct rpc_rqst *rqst, char *srcp, int copy_len)
+rpcrdma_inline_fixup(struct rpc_rqst *rqst, char *srcp, int 

copy_len, int pad)

net/sunrpc/xprtrdma/rpc_rdma.c rpcrdma_inline_fixup(struct rpc_rqst *rqst,

Quoted 4 lines

net/sunrpc/xprtrdma/rpc_rdma.c +0 -0

 {
 	int i, npages, curlen, olen;
 	char *destp;

@@ -660,6 +665,13 @@ rpcrdma_inline_fixup(struct rpc_rqst *rqst, 

char *srcp, int copy_len)

Review

Quoted 36 lines

net/sunrpc/xprtrdma/rpc_rdma.c +14 -1

 	} else
 		rqst->rq_rcv_buf.tail[0].iov_len = 0;
 
+	if (pad) {
+		/* implicit padding on terminal chunk */
+		unsigned char *p = rqst->rq_rcv_buf.tail[0].iov_base;
+		while (pad--)
+			p[rqst->rq_rcv_buf.tail[0].iov_len++] = 0;
+	}
+
 	if (copy_len)
 		dprintk("RPC:       %s: %d bytes in"
 			" %d extra segments (%d lost)\n",

@@ -794,14 +806,20 @@ repost:
 			    ((unsigned char *)iptr - (unsigned char *)headerp);
 			status = rep->rr_len + rdmalen;
 			r_xprt->rx_stats.total_rdma_reply += rdmalen;
+			/* special case - last chunk may omit padding */
+			if (rdmalen &= 3) {
+				rdmalen = 4 - rdmalen;
+				status += rdmalen;
+			}
 		} else {
 			/* else ordinary inline */
+			rdmalen = 0;
 			iptr = (__be32 *)((unsigned char *)headerp + 28);
 			rep->rr_len -= 28; /*sizeof *headerp;*/
 			status = rep->rr_len;
 		}
 		/* Fix up the rpc results for upper layer */
-		rpcrdma_inline_fixup(rqst, (char *)iptr, rep->rr_len);
+		rpcrdma_inline_fixup(rqst, (char *)iptr, rep->rr_len, rdmalen);
 		break;
 
 	case __constant_htonl(RDMA_NOMSG):

(quoted diff) +0 -0

b/net/sunrpc/xprtrdma/transport.c

net/sunrpc/xprtrdma/transport.c static unsigned int xprt_rdma_max_inline_read =

Quoted 4 lines

index ec6d1e7..c7d2380 100644

net/sunrpc/xprtrdma/transport.c +0 -0

@@ -71,6 +71,7 @@ static unsigned int xprt_rdma_max_inline_read = 

RPCRDMA_DEF_INLINE;

Quoted 27 lines

net/sunrpc/xprtrdma/transport.c +9 -1

 static unsigned int xprt_rdma_max_inline_write = RPCRDMA_DEF_INLINE;
 static unsigned int xprt_rdma_inline_write_padding;
 static unsigned int xprt_rdma_memreg_strategy = RPCRDMA_FRMR;
+                int xprt_rdma_pad_optimize = 0;
 
 #ifdef RPC_DEBUG
 

@@ -136,6 +137,14 @@ static ctl_table xr_tunables_table[] = {
 		.extra2		= &max_memreg,
 	},
 	{
+		.ctl_name       = CTL_UNNUMBERED,
+		.procname	= "rdma_pad_optimize",
+		.data		= &xprt_rdma_pad_optimize,
+		.maxlen		= sizeof(unsigned int),
+		.mode		= 0644,
+		.proc_handler	= &proc_dointvec,
+	},
+	{
 		.ctl_name = 0,
 	},
 };
 
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

The mount system call path does not expect such errors as ECONNREFUSED to be returned from failed transport connection attempts, otherwise it prints simply "internal error". Translate all such errors to ENOTCONN from RPC/RDMA to match sockets behavior.

Signed-off-by Tom Talpey <[email protected]>

---

 net/sunrpc/xprtrdma/rpc_rdma.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

net/sunrpc/xprtrdma/rpc_rdma.c +1 -2

index c4b8011..11ea8da 100644
@@ -700,7 +700,7 @@ rpcrdma_conn_func(struct rpcrdma_ep *ep)
 			xprt_wake_pending_tasks(xprt, 0);
 	} else {
 		if (xprt_test_and_clear_connected(xprt))
-			xprt_wake_pending_tasks(xprt, ep->rep_connected);
+			xprt_wake_pending_tasks(xprt, -ENOTCONN);
 	}
 	spin_unlock_bh(&xprt->transport_lock);
 }
 
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · Tom Talpey

The mount system call path does not expect such errors as ECONNREFUSED to be returned from failed transport connection attempts, otherwise it prints simply "internal error". Translate all such errors to ENOTCONN from RPC/RDMA to match sockets behavior.

Hmm... Shouldn't we be passing the ECONNREFUSED error here, and rather fix the downstream error paths?

Quoted 24 lines

Signed-off-by Tom Talpey <[email protected]>

---

 net/sunrpc/xprtrdma/rpc_rdma.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

net/sunrpc/xprtrdma/rpc_rdma.c +1 -2

index c4b8011..11ea8da 100644
@@ -700,7 +700,7 @@ rpcrdma_conn_func(struct rpcrdma_ep *ep)
 			xprt_wake_pending_tasks(xprt, 0);
 	} else {
 		if (xprt_test_and_clear_connected(xprt))
-			xprt_wake_pending_tasks(xprt, ep->rep_connected);
+			xprt_wake_pending_tasks(xprt, -ENOTCONN);
 	}
 	spin_unlock_bh(&xprt->transport_lock);
 }
 
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 8 lines · Trond Myklebust

Quoted 4 lines · Tom Talpey

The mount system call path does not expect such errors as ECONNREFUSED to be returned from failed transport connection attempts, otherwise it prints simply "internal error". Translate all such errors to ENOTCONN from RPC/RDMA to match sockets behavior.

Hmm... Shouldn't we be passing the ECONNREFUSED error here, and rather fix the downstream error paths?

That means fixing /sbin/mount.nfs, and an earlier conversation concluded that "doing what TCP does" was preferred. The error path from NFS and RPC is, frankly, more than a little tortuous. The error is translated and filtered in both layers, after being returned from the transport. Then, the mount command makes up its own diagnostic from what comes back from the syscall. Well beyond the scope of RDMA.

Your call. As proposed, it is more compatible with current practice, IMO.

Quoted 7 lines · Thomas

Quoted 6 lines · Trond Myklebust

Quoted 4 lines · Tom Talpey

The mount system call path does not expect such errors as ECONNREFUSED to be returned from failed transport connection attempts, otherwise it prints simply "internal error". Translate all such errors to ENOTCONN from RPC/RDMA to match sockets behavior.

Quoted 8 lines

Quoted 2 lines

Hmm... Shouldn't we be passing the ECONNREFUSED error here, and rather fix the downstream error paths?

That means fixing /sbin/mount.nfs, and an earlier conversation concluded that "doing what TCP does" was preferred. The error path from NFS and RPC is, frankly, more than a little tortuous. The error is translated and filtered in both layers, after being returned from the transport. Then, the mount command makes up its own diagnostic from what comes back from the syscall.

Well beyond

Quoted 3 lines

the scope of RDMA.

Your call. As proposed, it is more compatible with current practice, IMO.

Are you saying that mount.nfs translates 'ECONNREFUSED' as 'internal error'? That would be a bug...

Trond

The RPC/RDMA code always performed a reconnect-with-backoff, even when re-establishing a connection to a server after the RPC layer closed it due to being idle.
---

 net/sunrpc/xprtrdma/transport.c |    5 +++--
 net/sunrpc/xprtrdma/verbs.c     |    2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

net/sunrpc/xprtrdma/transport.c +3 -2

index c7d2380..278a544 100644
@@ -486,8 +486,9 @@ xprt_rdma_connect(struct rpc_task *task)
 	struct rpcrdma_xprt *r_xprt = rpcx_to_rdmax(xprt);
 
 	if (!xprt_test_and_set_connecting(xprt)) {
-		if (r_xprt->rx_ep.rep_connected != 0) {
-			/* Reconnect */
+		if (r_xprt->rx_ep.rep_connected &&
+		    r_xprt->rx_ep.rep_connected != -EPIPE) {
+			/* Reconnect with backoff */
 			schedule_delayed_work(&r_xprt->rdma_connect,
 				xprt->reestablish_timeout);
 		} else {

net/sunrpc/xprtrdma/verbs.c +0 -0

index a63d0c0..9ef7e0d 100644

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Review

Quoted 10 lines · Tom Talpey

The RPC/RDMA code always performed a reconnect-with-backoff, even when re-establishing a connection to a server after the RPC layer closed it due to being idle.
---

 net/sunrpc/xprtrdma/transport.c |    5 +++--
 net/sunrpc/xprtrdma/verbs.c     |    2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

(quoted diff) +0 -0

b/net/sunrpc/xprtrdma/transport.c

net/sunrpc/xprtrdma/verbs.c rpcrdma_conn_upcall(struct rdma_cm_id *id,

Quoted 20 lines

index c7d2380..278a544 100644

net/sunrpc/xprtrdma/transport.c +3 -2

@@ -486,8 +486,9 @@ xprt_rdma_connect(struct rpc_task *task)
 	struct rpcrdma_xprt *r_xprt = rpcx_to_rdmax(xprt);
 
 	if (!xprt_test_and_set_connecting(xprt)) {
-		if (r_xprt->rx_ep.rep_connected != 0) {
-			/* Reconnect */
+		if (r_xprt->rx_ep.rep_connected &&
+		    r_xprt->rx_ep.rep_connected != -EPIPE) {
+			/* Reconnect with backoff */
 			schedule_delayed_work(&r_xprt->rdma_connect,
 				xprt->reestablish_timeout);
 		} else {

net/sunrpc/xprtrdma/verbs.c +0 -0

index a63d0c0..9ef7e0d 100644
@@ -317,7 +317,7 @@ rpcrdma_conn_upcall(struct rdma_cm_id *id, 

struct rdma_cm_event *event)

net/sunrpc/xprtrdma/verbs.c rpcrdma_conn_upcall(struct rdma_cm_id *id, struct rdma_cm_event *event) · line 323

Quoted 8 lines

net/sunrpc/xprtrdma/verbs.c +1 -1

 		connstate = -ECONNREFUSED;
 		goto connected;
 	case RDMA_CM_EVENT_DISCONNECTED:
-		connstate = -ECONNABORTED;
+		connstate = -EPIPE;
 		goto connected;
 	case RDMA_CM_EVENT_DEVICE_REMOVAL:
 		connstate = -ENODEV;

Hmm... Why not rather do the same as the socket code: have the disconnect handler paths that don't require exponential backoff just reset xprt->reestablish_timeout to 0?

Quoted 4 lines

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 3 lines · Trond Myklebust

Hmm... Why not rather do the same as the socket code: have the disconnect handler paths that don't require exponential backoff just reset xprt->reestablish_timeout to 0?

Because we do want a non-zero reestablishment timeout in general, and the RDMA client has not implemented a connection backoff. So in effect the value is constant for this code, and I thought treating it as such is the safer fix.

I'm not 100% convinced the TCP code is correct, btw. It appears to zero out the reestablish timeout on idle-disconnect, but it's not obvious to me where it sets it back to a non-zero value. It does try to double it in xs_connect() though! :-)

Quoted 10 lines · Thomas

Quoted 3 lines · Trond Myklebust

Hmm... Why not rather do the same as the socket code: have the disconnect handler paths that don't require exponential backoff just reset xprt->reestablish_timeout to 0?

Because we do want a non-zero reestablishment timeout in general, and the RDMA client has not implemented a connection backoff. So in effect the value is constant for this code, and I thought treating it as such is the safer fix.

Quoted 4 lines

I'm not 100% convinced the TCP code is correct, btw. It appears to zero out the reestablish timeout on idle-disconnect, but it's not obvious to me where it sets it back to a non-zero value. It does try to double it in xs_connect() though! :-)

The TCP code sets the xprt->reestablish_timeout to a non-zero value whenever the _server_ closes the connection (i.e. if ever we enter a SYN_SENT state followed by a reset, a CLOSE_WAIT state or a CLOSING state.

Why would the RDMA client want to do anything different?