[v1,05/10] svcrdma: Make RDMA_ERROR messages work

Message ID	20160203155203.13868.93909.stgit@klimt.1015granger.net (mailing list archive)
State	New, archived
Headers	show Return-Path: <linux-nfs-owner@kernel.org> Subject: [PATCH v1 05/10] svcrdma: Make RDMA_ERROR messages work From: Chuck Lever <chuck.lever@oracle.com> To: linux-rdma@vger.kernel.org, linux-nfs@vger.kernel.org Date: Wed, 03 Feb 2016 10:52:03 -0500 Message-ID: <20160203155203.13868.93909.stgit@klimt.1015granger.net> In-Reply-To: <20160203154411.13868.48268.stgit@klimt.1015granger.net> References: <20160203154411.13868.48268.stgit@klimt.1015granger.net> User-Agent: StGit/0.17.1-dirty MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk

Message ID

20160203155203.13868.93909.stgit@klimt.1015granger.net (mailing list archive)

State

New, archived

Headers

Subject: [PATCH v1 05/10] svcrdma: Make RDMA_ERROR messages work
From: Chuck Lever <chuck.lever@oracle.com>
To: linux-rdma@vger.kernel.org, linux-nfs@vger.kernel.org
Date: Wed, 03 Feb 2016 10:52:03 -0500
Message-ID: <20160203155203.13868.93909.stgit@klimt.1015granger.net>
In-Reply-To: <20160203154411.13868.48268.stgit@klimt.1015granger.net>
References: <20160203154411.13868.48268.stgit@klimt.1015granger.net>
User-Agent: StGit/0.17.1-dirty
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

Commit Message

Chuck Lever Feb. 3, 2016, 3:52 p.m. UTC

Fix several issues with svc_rdma_send_error():

 - Post a receive buffer to replace the one that was consumed by
   the incoming request
 - Posting a send should use DMA_TO_DEVICE, not DMA_FROM_DEVICE
 - No need to put_page _and_ free pages in svc_rdma_put_context
 - Make sure the sge is set up completely in case the error
   path goes through svc_rdma_unmap_dma()

Related fixes in svc_rdma_recvfrom():

 - Don't leak the ctxt associated with the incoming request
 - Don't close the connection after sending an error reply
 - Let svc_rdma_send_error() figure out the right header error code

As a last clean up, move svc_rdma_send_error() to svc_rdma_sendto.c
with other similar functions. There is some common logic in these
functions that could someday be combined to reduce code duplication.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/linux/sunrpc/svc_rdma.h          |    4 +-
 net/sunrpc/xprtrdma/svc_rdma_recvfrom.c  |   19 ++++-----
 net/sunrpc/xprtrdma/svc_rdma_sendto.c    |   62 ++++++++++++++++++++++++++++++
 net/sunrpc/xprtrdma/svc_rdma_transport.c |   54 --------------------------
 4 files changed, 73 insertions(+), 66 deletions(-)


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

Comments

Devesh Sharma Feb. 5, 2016, 10:35 a.m. UTC | #1

Looks good

On Wed, Feb 3, 2016 at 9:22 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
> Fix several issues with svc_rdma_send_error():
>
>  - Post a receive buffer to replace the one that was consumed by
>    the incoming request
>  - Posting a send should use DMA_TO_DEVICE, not DMA_FROM_DEVICE
>  - No need to put_page _and_ free pages in svc_rdma_put_context
>  - Make sure the sge is set up completely in case the error
>    path goes through svc_rdma_unmap_dma()
>
> Related fixes in svc_rdma_recvfrom():
>
>  - Don't leak the ctxt associated with the incoming request
>  - Don't close the connection after sending an error reply
>  - Let svc_rdma_send_error() figure out the right header error code
>
> As a last clean up, move svc_rdma_send_error() to svc_rdma_sendto.c
> with other similar functions. There is some common logic in these
> functions that could someday be combined to reduce code duplication.
>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  include/linux/sunrpc/svc_rdma.h          |    4 +-
>  net/sunrpc/xprtrdma/svc_rdma_recvfrom.c  |   19 ++++-----
>  net/sunrpc/xprtrdma/svc_rdma_sendto.c    |   62 ++++++++++++++++++++++++++++++
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |   54 --------------------------
>  4 files changed, 73 insertions(+), 66 deletions(-)
>
> diff --git a/include/linux/sunrpc/svc_rdma.h b/include/linux/sunrpc/svc_rdma.h
> index d8fc58d..0589918 100644
> --- a/include/linux/sunrpc/svc_rdma.h
> +++ b/include/linux/sunrpc/svc_rdma.h
> @@ -228,11 +228,11 @@ extern int svc_rdma_map_xdr(struct svcxprt_rdma *, struct xdr_buf *,
>  extern int svc_rdma_sendto(struct svc_rqst *);
>  extern struct rpcrdma_read_chunk *
>         svc_rdma_get_read_chunk(struct rpcrdma_msg *);
> +extern void svc_rdma_send_error(struct svcxprt_rdma *, struct rpcrdma_msg *,
> +                               int);
>
>  /* svc_rdma_transport.c */
>  extern int svc_rdma_send(struct svcxprt_rdma *, struct ib_send_wr *);
> -extern void svc_rdma_send_error(struct svcxprt_rdma *, struct rpcrdma_msg *,
> -                               enum rpcrdma_errcode);
>  extern int svc_rdma_post_recv(struct svcxprt_rdma *, gfp_t);
>  extern int svc_rdma_repost_recv(struct svcxprt_rdma *, gfp_t);
>  extern int svc_rdma_create_listen(struct svc_serv *, int, struct sockaddr *);
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> index acf15b8..0f09052 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> @@ -612,7 +612,6 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
>         struct svc_rdma_op_ctxt *ctxt = NULL;
>         struct rpcrdma_msg *rmsgp;
>         int ret = 0;
> -       int len;
>
>         dprintk("svcrdma: rqstp=%p\n", rqstp);
>
> @@ -654,15 +653,10 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
>         rdma_build_arg_xdr(rqstp, ctxt, ctxt->byte_len);
>
>         /* Decode the RDMA header. */
> -       len = svc_rdma_xdr_decode_req(&rmsgp, rqstp);
> -       rqstp->rq_xprt_hlen = len;
> -
> -       /* If the request is invalid, reply with an error */
> -       if (len < 0) {
> -               if (len == -ENOSYS)
> -                       svc_rdma_send_error(rdma_xprt, rmsgp, ERR_VERS);
> -               goto close_out;
> -       }
> +       ret = svc_rdma_xdr_decode_req(&rmsgp, rqstp);
> +       if (ret < 0)
> +               goto out_err;
> +       rqstp->rq_xprt_hlen = ret;
>
>         if (svc_rdma_is_backchannel_reply(xprt, rmsgp)) {
>                 ret = svc_rdma_handle_bc_reply(xprt->xpt_bc_xprt, rmsgp,
> @@ -698,6 +692,11 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
>         svc_xprt_copy_addrs(rqstp, xprt);
>         return ret;
>
> +out_err:
> +       svc_rdma_send_error(rdma_xprt, rmsgp, ret);
> +       svc_rdma_put_context(ctxt, 0);
> +       return 0;
> +
>   close_out:
>         if (ctxt)
>                 svc_rdma_put_context(ctxt, 1);
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
> index 687a91afe..73793dd 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
> @@ -639,3 +639,65 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)
>         svc_rdma_put_context(ctxt, 0);
>         return ret;
>  }
> +
> +void svc_rdma_send_error(struct svcxprt_rdma *xprt, struct rpcrdma_msg *rmsgp,
> +                        int status)
> +{
> +       struct ib_send_wr err_wr;
> +       struct page *p;
> +       struct svc_rdma_op_ctxt *ctxt;
> +       enum rpcrdma_errcode err;
> +       __be32 *va;
> +       int length;
> +       int ret;
> +
> +       ret = svc_rdma_repost_recv(xprt, GFP_KERNEL);
> +       if (ret)
> +               return;
> +
> +       p = alloc_page(GFP_KERNEL);
> +       if (!p)
> +               return;
> +       va = page_address(p);
> +
> +       /* XDR encode an error reply */
> +       err = ERR_CHUNK;
> +       if (status == -ENOSYS)
> +               err = ERR_VERS;
> +       length = svc_rdma_xdr_encode_error(xprt, rmsgp, err, va);
> +
> +       ctxt = svc_rdma_get_context(xprt);
> +       ctxt->direction = DMA_TO_DEVICE;
> +       ctxt->count = 1;
> +       ctxt->pages[0] = p;
> +
> +       /* Prepare SGE for local address */
> +       ctxt->sge[0].lkey = xprt->sc_pd->local_dma_lkey;
> +       ctxt->sge[0].length = length;
> +       ctxt->sge[0].addr = ib_dma_map_page(xprt->sc_cm_id->device,
> +                                           p, 0, length, DMA_TO_DEVICE);
> +       if (ib_dma_mapping_error(xprt->sc_cm_id->device, ctxt->sge[0].addr)) {
> +               dprintk("svcrdma: Error mapping buffer for protocol error\n");
> +               svc_rdma_put_context(ctxt, 1);
> +               return;
> +       }
> +       atomic_inc(&xprt->sc_dma_used);
> +
> +       /* Prepare SEND WR */
> +       memset(&err_wr, 0, sizeof err_wr);
> +       ctxt->wr_op = IB_WR_SEND;
> +       err_wr.wr_id = (unsigned long)ctxt;
> +       err_wr.sg_list = ctxt->sge;
> +       err_wr.num_sge = 1;
> +       err_wr.opcode = IB_WR_SEND;
> +       err_wr.send_flags = IB_SEND_SIGNALED;
> +
> +       /* Post It */
> +       ret = svc_rdma_send(xprt, &err_wr);
> +       if (ret) {
> +               dprintk("svcrdma: Error %d posting send for protocol error\n",
> +                       ret);
> +               svc_rdma_unmap_dma(ctxt);
> +               svc_rdma_put_context(ctxt, 1);
> +       }
> +}
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index da82e36..e7bda1e 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -1465,57 +1465,3 @@ int svc_rdma_send(struct svcxprt_rdma *xprt, struct ib_send_wr *wr)
>         }
>         return ret;
>  }
> -
> -void svc_rdma_send_error(struct svcxprt_rdma *xprt, struct rpcrdma_msg *rmsgp,
> -                        enum rpcrdma_errcode err)
> -{
> -       struct ib_send_wr err_wr;
> -       struct page *p;
> -       struct svc_rdma_op_ctxt *ctxt;
> -       __be32 *va;
> -       int length;
> -       int ret;
> -
> -       p = alloc_page(GFP_KERNEL);
> -       if (!p)
> -               return;
> -       va = page_address(p);
> -
> -       /* XDR encode error */
> -       length = svc_rdma_xdr_encode_error(xprt, rmsgp, err, va);
> -
> -       ctxt = svc_rdma_get_context(xprt);
> -       ctxt->direction = DMA_FROM_DEVICE;
> -       ctxt->count = 1;
> -       ctxt->pages[0] = p;
> -
> -       /* Prepare SGE for local address */
> -       ctxt->sge[0].addr = ib_dma_map_page(xprt->sc_cm_id->device,
> -                                           p, 0, length, DMA_FROM_DEVICE);
> -       if (ib_dma_mapping_error(xprt->sc_cm_id->device, ctxt->sge[0].addr)) {
> -               put_page(p);
> -               svc_rdma_put_context(ctxt, 1);
> -               return;
> -       }
> -       atomic_inc(&xprt->sc_dma_used);
> -       ctxt->sge[0].lkey = xprt->sc_pd->local_dma_lkey;
> -       ctxt->sge[0].length = length;
> -
> -       /* Prepare SEND WR */
> -       memset(&err_wr, 0, sizeof err_wr);
> -       ctxt->wr_op = IB_WR_SEND;
> -       err_wr.wr_id = (unsigned long)ctxt;
> -       err_wr.sg_list = ctxt->sge;
> -       err_wr.num_sge = 1;
> -       err_wr.opcode = IB_WR_SEND;
> -       err_wr.send_flags = IB_SEND_SIGNALED;
> -
> -       /* Post It */
> -       ret = svc_rdma_send(xprt, &err_wr);
> -       if (ret) {
> -               dprintk("svcrdma: Error %d posting send for protocol error\n",
> -                       ret);
> -               svc_rdma_unmap_dma(ctxt);
> -               svc_rdma_put_context(ctxt, 1);
> -       }
> -}
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

diff --git a/include/linux/sunrpc/svc_rdma.h b/include/linux/sunrpc/svc_rdma.h
index d8fc58d..0589918 100644
--- a/include/linux/sunrpc/svc_rdma.h
+++ b/include/linux/sunrpc/svc_rdma.h
@@ -228,11 +228,11 @@  extern int svc_rdma_map_xdr(struct svcxprt_rdma *, struct xdr_buf *,
 extern int svc_rdma_sendto(struct svc_rqst *);
 extern struct rpcrdma_read_chunk *
 	svc_rdma_get_read_chunk(struct rpcrdma_msg *);
+extern void svc_rdma_send_error(struct svcxprt_rdma *, struct rpcrdma_msg *,
+				int);
 
 /* svc_rdma_transport.c */
 extern int svc_rdma_send(struct svcxprt_rdma *, struct ib_send_wr *);
-extern void svc_rdma_send_error(struct svcxprt_rdma *, struct rpcrdma_msg *,
-				enum rpcrdma_errcode);
 extern int svc_rdma_post_recv(struct svcxprt_rdma *, gfp_t);
 extern int svc_rdma_repost_recv(struct svcxprt_rdma *, gfp_t);
 extern int svc_rdma_create_listen(struct svc_serv *, int, struct sockaddr *);
diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
index acf15b8..0f09052 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
@@ -612,7 +612,6 @@  int svc_rdma_recvfrom(struct svc_rqst *rqstp)
 	struct svc_rdma_op_ctxt *ctxt = NULL;
 	struct rpcrdma_msg *rmsgp;
 	int ret = 0;
-	int len;
 
 	dprintk("svcrdma: rqstp=%p\n", rqstp);
 
@@ -654,15 +653,10 @@  int svc_rdma_recvfrom(struct svc_rqst *rqstp)
 	rdma_build_arg_xdr(rqstp, ctxt, ctxt->byte_len);
 
 	/* Decode the RDMA header. */
-	len = svc_rdma_xdr_decode_req(&rmsgp, rqstp);
-	rqstp->rq_xprt_hlen = len;
-
-	/* If the request is invalid, reply with an error */
-	if (len < 0) {
-		if (len == -ENOSYS)
-			svc_rdma_send_error(rdma_xprt, rmsgp, ERR_VERS);
-		goto close_out;
-	}
+	ret = svc_rdma_xdr_decode_req(&rmsgp, rqstp);
+	if (ret < 0)
+		goto out_err;
+	rqstp->rq_xprt_hlen = ret;
 
 	if (svc_rdma_is_backchannel_reply(xprt, rmsgp)) {
 		ret = svc_rdma_handle_bc_reply(xprt->xpt_bc_xprt, rmsgp,
@@ -698,6 +692,11 @@  int svc_rdma_recvfrom(struct svc_rqst *rqstp)
 	svc_xprt_copy_addrs(rqstp, xprt);
 	return ret;
 
+out_err:
+	svc_rdma_send_error(rdma_xprt, rmsgp, ret);
+	svc_rdma_put_context(ctxt, 0);
+	return 0;
+
  close_out:
 	if (ctxt)
 		svc_rdma_put_context(ctxt, 1);
diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
index 687a91afe..73793dd 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
@@ -639,3 +639,65 @@  int svc_rdma_sendto(struct svc_rqst *rqstp)
 	svc_rdma_put_context(ctxt, 0);
 	return ret;
 }
+
+void svc_rdma_send_error(struct svcxprt_rdma *xprt, struct rpcrdma_msg *rmsgp,
+			 int status)
+{
+	struct ib_send_wr err_wr;
+	struct page *p;
+	struct svc_rdma_op_ctxt *ctxt;
+	enum rpcrdma_errcode err;
+	__be32 *va;
+	int length;
+	int ret;
+
+	ret = svc_rdma_repost_recv(xprt, GFP_KERNEL);
+	if (ret)
+		return;
+
+	p = alloc_page(GFP_KERNEL);
+	if (!p)
+		return;
+	va = page_address(p);
+
+	/* XDR encode an error reply */
+	err = ERR_CHUNK;
+	if (status == -ENOSYS)
+		err = ERR_VERS;
+	length = svc_rdma_xdr_encode_error(xprt, rmsgp, err, va);
+
+	ctxt = svc_rdma_get_context(xprt);
+	ctxt->direction = DMA_TO_DEVICE;
+	ctxt->count = 1;
+	ctxt->pages[0] = p;
+
+	/* Prepare SGE for local address */
+	ctxt->sge[0].lkey = xprt->sc_pd->local_dma_lkey;
+	ctxt->sge[0].length = length;
+	ctxt->sge[0].addr = ib_dma_map_page(xprt->sc_cm_id->device,
+					    p, 0, length, DMA_TO_DEVICE);
+	if (ib_dma_mapping_error(xprt->sc_cm_id->device, ctxt->sge[0].addr)) {
+		dprintk("svcrdma: Error mapping buffer for protocol error\n");
+		svc_rdma_put_context(ctxt, 1);
+		return;
+	}
+	atomic_inc(&xprt->sc_dma_used);
+
+	/* Prepare SEND WR */
+	memset(&err_wr, 0, sizeof err_wr);
+	ctxt->wr_op = IB_WR_SEND;
+	err_wr.wr_id = (unsigned long)ctxt;
+	err_wr.sg_list = ctxt->sge;
+	err_wr.num_sge = 1;
+	err_wr.opcode = IB_WR_SEND;
+	err_wr.send_flags = IB_SEND_SIGNALED;
+
+	/* Post It */
+	ret = svc_rdma_send(xprt, &err_wr);
+	if (ret) {
+		dprintk("svcrdma: Error %d posting send for protocol error\n",
+			ret);
+		svc_rdma_unmap_dma(ctxt);
+		svc_rdma_put_context(ctxt, 1);
+	}
+}
diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
index da82e36..e7bda1e 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
@@ -1465,57 +1465,3 @@  int svc_rdma_send(struct svcxprt_rdma *xprt, struct ib_send_wr *wr)
 	}
 	return ret;
 }
-
-void svc_rdma_send_error(struct svcxprt_rdma *xprt, struct rpcrdma_msg *rmsgp,
-			 enum rpcrdma_errcode err)
-{
-	struct ib_send_wr err_wr;
-	struct page *p;
-	struct svc_rdma_op_ctxt *ctxt;
-	__be32 *va;
-	int length;
-	int ret;
-
-	p = alloc_page(GFP_KERNEL);
-	if (!p)
-		return;
-	va = page_address(p);
-
-	/* XDR encode error */
-	length = svc_rdma_xdr_encode_error(xprt, rmsgp, err, va);
-
-	ctxt = svc_rdma_get_context(xprt);
-	ctxt->direction = DMA_FROM_DEVICE;
-	ctxt->count = 1;
-	ctxt->pages[0] = p;
-
-	/* Prepare SGE for local address */
-	ctxt->sge[0].addr = ib_dma_map_page(xprt->sc_cm_id->device,
-					    p, 0, length, DMA_FROM_DEVICE);
-	if (ib_dma_mapping_error(xprt->sc_cm_id->device, ctxt->sge[0].addr)) {
-		put_page(p);
-		svc_rdma_put_context(ctxt, 1);
-		return;
-	}
-	atomic_inc(&xprt->sc_dma_used);
-	ctxt->sge[0].lkey = xprt->sc_pd->local_dma_lkey;
-	ctxt->sge[0].length = length;
-
-	/* Prepare SEND WR */
-	memset(&err_wr, 0, sizeof err_wr);
-	ctxt->wr_op = IB_WR_SEND;
-	err_wr.wr_id = (unsigned long)ctxt;
-	err_wr.sg_list = ctxt->sge;
-	err_wr.num_sge = 1;
-	err_wr.opcode = IB_WR_SEND;
-	err_wr.send_flags = IB_SEND_SIGNALED;
-
-	/* Post It */
-	ret = svc_rdma_send(xprt, &err_wr);
-	if (ret) {
-		dprintk("svcrdma: Error %d posting send for protocol error\n",
-			ret);
-		svc_rdma_unmap_dma(ctxt);
-		svc_rdma_put_context(ctxt, 1);
-	}
-}

[v1,05/10] svcrdma: Make RDMA_ERROR messages work

Commit Message

Comments

Patch