svcrdma: Remove redunant call to xpo_release_rqst method.

The xpo_release_rqst method is called from svc_xprt_release and therefore this direct call of the provider method is redundant. There is no bug here since the provider protects against multiple calls.

Originally, this code was here because the xpo_release_rqst function was being used by the RDMA transport to return credits to the client, i.e. posting a receive buffer. This had to be done before the send in order to avoid a race wherein the client responds immediately with a new request but the buffer has not yet been posted. This is a poor design point and the recv buffer posting has been modified in the rdma transport.

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

---
 net/sunrpc/svc_xprt.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

net/sunrpc/svc_xprt.c +0 -3

index d8e8d79..74e52d4 100644
@@ -751,9 +751,6 @@ int svc_send(struct svc_rqst *rqstp)
 	if (!xprt)
 		return -EFAULT;
 
-	/* release the receive skb before sending the reply */
-	rqstp->rq_xprt->xpt_ops->xpo_release_rqst(rqstp);
-
 	/* calculate over-all length */
 	xb = &rqstp->rq_res;
 	xb->len = xb->head[0].iov_len +

Quoted 4 lines · Tom Tucker

The xpo_release_rqst method is called from svc_xprt_release and therefore this direct call of the provider method is redundant. There is no bug here since the provider protects against multiple calls.

Quoted 6 lines

Originally, this code was here because the xpo_release_rqst function was being used by the RDMA transport to return credits to the client, i.e. posting a receive buffer. This had to be done before the send in order to avoid a race wherein the client responds immediately with a new request but the buffer has not yet been posted. This is a poor design point and the recv buffer posting has been modified in the rdma transport.

The comment there refers to the socket code, not the rdma code, and

there was a svc_release_skb() there before the rdma code came along. I'd like to understand why.

--b.

Quoted 12 lines

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

---
 net/sunrpc/svc_xprt.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

net/sunrpc/svc_xprt.c +0 -0

index d8e8d79..74e52d4 100644

net/sunrpc/svc_xprt.c int svc_send(struct svc_rqst *rqstp) · line 756

Quoted 9 lines

net/sunrpc/svc_xprt.c +0 -3

@@ -751,9 +751,6 @@ int svc_send(struct svc_rqst *rqstp)
 	if (!xprt)
 		return -EFAULT;
 
-	/* release the receive skb before sending the reply */
-	rqstp->rq_xprt->xpt_ops->xpo_release_rqst(rqstp);
-
 	/* calculate over-all length */
 	xb = &rqstp->rq_res;
 	xb->len = xb->head[0].iov_len +

--
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 · J. Bruce Fields

Quoted 4 lines · Tom Tucker

The xpo_release_rqst method is called from svc_xprt_release and therefore this direct call of the provider method is redundant. There is no bug here since the provider protects against multiple calls.

Quoted 8 lines

Quoted 6 lines

Originally, this code was here because the xpo_release_rqst function was being used by the RDMA transport to return credits to the client, i.e. posting a receive buffer. This had to be done before the send in order to avoid a race wherein the client responds immediately with a new request but the buffer has not yet been posted. This is a poor design point and the recv buffer posting has been modified in the rdma transport.

The comment there refers to the socket code, not the rdma code, and

Yes, you are correct.

Quoted 2 lines

there was a svc_release_skb() there before the rdma code came along. I'd like to understand why.

I believe it's because the socket code has buffer quotas and the skb you're holding consumes credits in the recv sock buf. So it's conceivable that you could send a message, another client message arrives prior to releasing the skb your holding, and this new message gets dropped unnecessarily. I don't believe it affects the sending at all because the message you're holding came from sock_recv_datagram and only consumes recv side credits.

Since this patch is only for "clean up" purposes, I'm inclined to just ditch this patch and let us noodle on it a little more. Agreed?

Remove a redundant check for the XPT_DEAD bit in the svc_xprt_enqueue function. This same bit is checked below while holding the pool lock

and prints a debug message if found to be dead.

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

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

net/sunrpc/svc_xprt.c +0 -2

index 74e52d4..fb8a785 100644
@@ -296,8 +296,6 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
 	if (!(xprt->xpt_flags &
 	      ((1<<XPT_CONN)|(1<<XPT_DATA)|(1<<XPT_CLOSE)|(1<<XPT_DEFERRED))))
 		return;
-	if (test_bit(XPT_DEAD, &xprt->xpt_flags))
-		return;
 
 	cpu = get_cpu();
 	pool = svc_pool_for_cpu(xprt->xpt_server, cpu);