2017-07-12 16:10:42

by Chuck Lever III

[permalink] [raw]
Subject: [PATCH] sunrpc: Remove NULL element at the end of rq_pages

Bruce points out that the NULL final element of rq_pages is unneeded
by nfs_read_actor. Remove it.

Thus the 260th element of rq_pages is also no longer needed.

Signed-off-by: Chuck Lever <[email protected]>
---
Hi Bruce-

I've been testing this one. No issues for NFSv4.0 on RDMA with
krb5i, but NFSv4.0 on TCP with krb5i encounters a problem. The
server log shows this message:

rpc-srv/tcp: nfsd: sent only 108 when sending 140 bytes - shutting down socket

And iozone stalls on the client. I haven't looked more closely.


include/linux/sunrpc/svc.h | 2 +-
net/sunrpc/svc_xprt.c | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index d741399..5500544 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -246,7 +246,7 @@ struct svc_rqst {
size_t rq_xprt_hlen; /* xprt header len */
struct xdr_buf rq_arg;
struct xdr_buf rq_res;
- struct page *rq_pages[RPCSVC_MAXPAGES + 1];
+ struct page *rq_pages[RPCSVC_MAXPAGES];
struct page * *rq_respages; /* points into rq_pages */
struct page * *rq_next_page; /* next reply page to use */
struct page * *rq_page_end; /* one past the last page */
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index d16a8b4..b7efd16 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -680,7 +680,6 @@ static int svc_alloc_arg(struct svc_rqst *rqstp)
rqstp->rq_pages[i] = p;
}
rqstp->rq_page_end = &rqstp->rq_pages[i];
- rqstp->rq_pages[i++] = NULL; /* this might be seen in nfs_read_actor */

/* Make arg->head point to first page and arg->pages point to rest */
arg = &rqstp->rq_arg;



2017-07-12 19:58:00

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] sunrpc: Remove NULL element at the end of rq_pages

On Wed, Jul 12, 2017 at 12:10:40PM -0400, Chuck Lever wrote:
> Bruce points out that the NULL final element of rq_pages is unneeded
> by nfs_read_actor. Remove it.
>
> Thus the 260th element of rq_pages is also no longer needed.
>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> Hi Bruce-
>
> I've been testing this one. No issues for NFSv4.0 on RDMA with
> krb5i, but NFSv4.0 on TCP with krb5i encounters a problem. The
> server log shows this message:
>
> rpc-srv/tcp: nfsd: sent only 108 when sending 140 bytes - shutting down socket
>
> And iozone stalls on the client. I haven't looked more closely.

Huh.

OK, well maybe there is some code I haven't spotted that depends on that
final NULL in rq_pages, or maybe this is just an unrelated bug, but I
think it's safest just to stick your previous version (from Message-ID:
<[email protected]>) back into the
series and then sort this out later.

But we should clean up these constants and comments, it's confusing
stuff in exactly the sort of code where a mistake could mean letting
anyone on the internet write to your memory.

--b.

>
>
> include/linux/sunrpc/svc.h | 2 +-
> net/sunrpc/svc_xprt.c | 1 -
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index d741399..5500544 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -246,7 +246,7 @@ struct svc_rqst {
> size_t rq_xprt_hlen; /* xprt header len */
> struct xdr_buf rq_arg;
> struct xdr_buf rq_res;
> - struct page *rq_pages[RPCSVC_MAXPAGES + 1];
> + struct page *rq_pages[RPCSVC_MAXPAGES];
> struct page * *rq_respages; /* points into rq_pages */
> struct page * *rq_next_page; /* next reply page to use */
> struct page * *rq_page_end; /* one past the last page */
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index d16a8b4..b7efd16 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -680,7 +680,6 @@ static int svc_alloc_arg(struct svc_rqst *rqstp)
> rqstp->rq_pages[i] = p;
> }
> rqstp->rq_page_end = &rqstp->rq_pages[i];
> - rqstp->rq_pages[i++] = NULL; /* this might be seen in nfs_read_actor */
>
> /* Make arg->head point to first page and arg->pages point to rest */
> arg = &rqstp->rq_arg;