[PATCH v1 0/4] NFS/RDMA server patches for v4.20

[PATCH v1 0/4] NFS/RDMA server patches for v4.20

[Chuck Lever]

Hi Bruce-

These four patches are ready for you to consider for the next merge
window. There are three clean up patches, and one patch to increase
the server's credit grant to the default size of the Linux client's
NFSv4.1 session slot table. I've measured modest performance
increases with this change.

---

Chuck Lever (4):
      svcrdma: Reduce max_send_sges
      svcrdma: Remove ->release_rqst call in bc reply handler
      svcrdma: Remove try_module_get from backchannel
      svcrdma: Increase the default connection credit limit


 include/linux/sunrpc/svc_rdma.h            |   13 +++++++------
 net/sunrpc/xprtrdma/svc_rdma_backchannel.c |   23 ++++-------------------
 net/sunrpc/xprtrdma/svc_rdma_transport.c   |   10 ++++++----
 3 files changed, 17 insertions(+), 29 deletions(-)

--
Chuck Lever

[PATCH v1 1/4] svcrdma: Reduce max_send_sges

[Chuck Lever]

There's no need to request a large number of send SGEs because the
inline threshold already constrains the number of SGEs per Send.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/xprtrdma/svc_rdma_transport.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
index 2848caf..2f7ec89 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
@@ -475,10 +475,12 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
 
 	/* Qualify the transport resource defaults with the
 	 * capabilities of this particular device */
-	newxprt->sc_max_send_sges = dev->attrs.max_send_sge;
-	/* transport hdr, head iovec, one page list entry, tail iovec */
-	if (newxprt->sc_max_send_sges < 4) {
-		pr_err("svcrdma: too few Send SGEs available (%d)\n",
+	/* Transport header, head iovec, tail iovec */
+	newxprt->sc_max_send_sges = 3;
+	/* Add one SGE per page list entry */
+	newxprt->sc_max_send_sges += svcrdma_max_req_size / PAGE_SIZE;
+	if (newxprt->sc_max_send_sges > dev->attrs.max_send_sge) {
+		pr_err("svcrdma: too few Send SGEs available (%d needed)\n",
 		       newxprt->sc_max_send_sges);
 		goto errout;
 	}

Re: [PATCH v1 1/4] svcrdma: Reduce max_send_sges

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH v1 2/4] svcrdma: Remove ->release_rqst call in bc reply handler

[Chuck Lever]

Similar to a change made in the client's forward channel reply
handler: The xprt_release_rqst_cong() call is not necessary.

Also, release xprt->recv_lock when taking xprt->transport_lock
to avoid disabling and enabling BH's while holding another
spin lock.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/xprtrdma/svc_rdma_backchannel.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/sunrpc/xprtrdma/svc_rdma_backchannel.c b/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
index a681800..642f031 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
@@ -32,7 +32,6 @@ int svc_rdma_handle_bc_reply(struct rpc_xprt *xprt, __be32 *rdma_resp,
 	struct rpcrdma_xprt *r_xprt = rpcx_to_rdmax(xprt);
 	struct kvec *dst, *src = &rcvbuf->head[0];
 	struct rpc_rqst *req;
-	unsigned long cwnd;
 	u32 credits;
 	size_t len;
 	__be32 xid;
@@ -66,6 +65,8 @@ int svc_rdma_handle_bc_reply(struct rpc_xprt *xprt, __be32 *rdma_resp,
 	if (dst->iov_len < len)
 		goto out_unlock;
 	memcpy(dst->iov_base, p, len);
+	xprt_pin_rqst(req);
+	spin_unlock(&xprt->recv_lock);
 
 	credits = be32_to_cpup(rdma_resp + 2);
 	if (credits == 0)
@@ -74,15 +75,13 @@ int svc_rdma_handle_bc_reply(struct rpc_xprt *xprt, __be32 *rdma_resp,
 		credits = r_xprt->rx_buf.rb_bc_max_requests;
 
 	spin_lock_bh(&xprt->transport_lock);
-	cwnd = xprt->cwnd;
 	xprt->cwnd = credits << RPC_CWNDSHIFT;
-	if (xprt->cwnd > cwnd)
-		xprt_release_rqst_cong(req->rq_task);
 	spin_unlock_bh(&xprt->transport_lock);
 
-
+	spin_lock(&xprt->recv_lock);
 	ret = 0;
 	xprt_complete_rqst(req->rq_task, rcvbuf->len);
+	xprt_unpin_rqst(req);
 	rcvbuf->len = 0;
 
 out_unlock:

Re: [PATCH v1 2/4] svcrdma: Remove ->release_rqst call in bc reply handler

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH v1 3/4] svcrdma: Remove try_module_get from backchannel

[Chuck Lever]

Since commit ffe1f0df5862 ("rpcrdma: Merge svcrdma and xprtrdma
modules into one"), the forward and backchannel components are part
of the same kernel module. A separate try_module_get() call in the
backchannel code is no longer necessary.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/xprtrdma/svc_rdma_backchannel.c |   14 --------------
 1 file changed, 14 deletions(-)

diff --git a/net/sunrpc/xprtrdma/svc_rdma_backchannel.c b/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
index 642f031..7e188bc 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
@@ -5,8 +5,6 @@
  * Support for backward direction RPCs on RPC/RDMA (server-side).
  */
 
-#include <linux/module.h>
-
 #include <linux/sunrpc/svc_rdma.h>
 
 #include "xprt_rdma.h"
@@ -255,7 +253,6 @@ static int svc_rdma_bc_sendto(struct svcxprt_rdma *rdma,
 	dprintk("svcrdma: %s: xprt %p\n", __func__, xprt);
 
 	xprt_free(xprt);
-	module_put(THIS_MODULE);
 }
 
 static const struct rpc_xprt_ops xprt_rdma_bc_procs = {
@@ -327,20 +324,9 @@ static int svc_rdma_bc_sendto(struct svcxprt_rdma *rdma,
 	args->bc_xprt->xpt_bc_xprt = xprt;
 	xprt->bc_xprt = args->bc_xprt;
 
-	if (!try_module_get(THIS_MODULE))
-		goto out_fail;
-
 	/* Final put for backchannel xprt is in __svc_rdma_free */
 	xprt_get(xprt);
 	return xprt;
-
-out_fail:
-	xprt_rdma_free_addresses(xprt);
-	args->bc_xprt->xpt_bc_xprt = NULL;
-	args->bc_xprt->xpt_bc_xps = NULL;
-	xprt_put(xprt);
-	xprt_free(xprt);
-	return ERR_PTR(-EINVAL);
 }
 
 struct xprt_class xprt_rdma_bc = {

Re: [PATCH v1 3/4] svcrdma: Remove try_module_get from backchannel

[Sagi Grimberg]

> Since commit ffe1f0df5862 ("rpcrdma: Merge svcrdma and xprtrdma
> modules into one"), the forward and backchannel components are part
> of the same kernel module. A separate try_module_get() call in the
> backchannel code is no longer necessary.

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Would it make sense to promote this to xprt.c and remove it
from the transports altogether?

Re: [PATCH v1 3/4] svcrdma: Remove try_module_get from backchannel

[Sagi Grimberg]

> Would it make sense to promote this to xprt.c and remove it
> from the transports altogether?

Never mind... got it confused...

[PATCH v1 4/4] svcrdma: Increase the default connection credit limit

[Chuck Lever]

Reduce queuing on clients by allowing more credits by default.

64 is the default NFSv4.1 slot table size on Linux clients. This
size prevents the credit limit from putting RPC requests to sleep
again after they have already slept waiting for a session slot.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/linux/sunrpc/svc_rdma.h |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/linux/sunrpc/svc_rdma.h b/include/linux/sunrpc/svc_rdma.h
index fd78f78..e6e2691 100644
--- a/include/linux/sunrpc/svc_rdma.h
+++ b/include/linux/sunrpc/svc_rdma.h
@@ -113,13 +113,14 @@ struct svcxprt_rdma {
 /* sc_flags */
 #define RDMAXPRT_CONN_PENDING	3
 
-#define RPCRDMA_LISTEN_BACKLOG  10
-#define RPCRDMA_MAX_REQUESTS    32
-
-/* Typical ULP usage of BC requests is NFSv4.1 backchannel. Our
- * current NFSv4.1 implementation supports one backchannel slot.
+/*
+ * Default connection parameters
  */
-#define RPCRDMA_MAX_BC_REQUESTS	2
+enum {
+	RPCRDMA_LISTEN_BACKLOG	= 10,
+	RPCRDMA_MAX_REQUESTS	= 64,
+	RPCRDMA_MAX_BC_REQUESTS	= 2,
+};
 
 #define RPCSVC_MAXPAYLOAD_RDMA	RPCSVC_MAXPAYLOAD
 

Re: [PATCH v1 4/4] svcrdma: Increase the default connection credit limit

[Sagi Grimberg]

> Reduce queuing on clients by allowing more credits by default.
> 
> 64 is the default NFSv4.1 slot table size on Linux clients. This
> size prevents the credit limit from putting RPC requests to sleep
> again after they have already slept waiting for a session slot.

Would it make sense to have it in a shared header with the client?

Otherwise,
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH v1 4/4] svcrdma: Increase the default connection credit limit

[Chuck Lever]

> On Oct 1, 2018, at 6:33 PM, Sagi Grimberg <sagi@grimberg.me> wrote:
> 
> 
>> Reduce queuing on clients by allowing more credits by default.
>> 64 is the default NFSv4.1 slot table size on Linux clients. This
>> size prevents the credit limit from putting RPC requests to sleep
>> again after they have already slept waiting for a session slot.
> 
> Would it make sense to have it in a shared header with the client?
> 
> Otherwise,
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Thanks for the review!

Typically all the server-side stuff is kept in separate headers,
I guess that's historical and matches the generic layer (..) .


--
Chuck Lever

Re: [PATCH v1 0/4] NFS/RDMA server patches for v4.20

[J. Bruce Fields]

On Mon, Oct 01, 2018 at 02:15:50PM -0400, Chuck Lever wrote:
> These four patches are ready for you to consider for the next merge
> window. There are three clean up patches, and one patch to increase
> the server's credit grant to the default size of the Linux client's
> NFSv4.1 session slot table. I've measured modest performance
> increases with this change.

Thanks, added Sagi Grimberg's Reviewed-by's and applied.

--b.