Hi Bruce-
Here is part of what we discussed recently about trying to align
pages in NFS WRITE requests so that splice can be used. This
series updates server-side RPC and svcrdma to ensure that an
aligned xdr_buf::pages array is presented to NFSD, which is then
converted into an aligned rq_vec for the VFS layer.
The next step would be to look at how to make the best use of the
aligned rq_vec. My naive thought is that where there is a PAGE_SIZE
entry in rq_vec and there is no page in the file's page cache at
that offset, the transport-provided page can be flipped into place.
Might work for replacing whole pages as well, but baby steps first.
This series has been exercised a bit with both TCP and RDMA, but no
guarantees that it is completely bug-free. NFSv4 compounds with
multiple WRITE payloads on RDMA are treated like TCP: the RPC
message is contained in an unstructured stream of unaligned pages.
Comments encouraged.
---
Chuck Lever (6):
SUNRPC: Capture value of xdr_buf::page_base
SUNRPC: xdr_stream_subsegment() must handle non-zero page_bases
NFSD: Have legacy NFSD WRITE decoders use xdr_stream_subsegment()
SUNRPC: svc_fill_write_vector() must handle non-zero page_bases
NFSD: Add a transport hook for pulling argument payloads
svcrdma: Pull Read chunks in ->xpo_argument_payload
fs/nfsd/nfs3proc.c | 3 +-
fs/nfsd/nfs3xdr.c | 16 +--
fs/nfsd/nfs4proc.c | 3 +-
fs/nfsd/nfs4xdr.c | 6 +
fs/nfsd/nfsproc.c | 3 +-
fs/nfsd/nfsxdr.c | 13 +--
fs/nfsd/xdr.h | 2 +-
fs/nfsd/xdr3.h | 2 +-
include/linux/sunrpc/svc.h | 6 +-
include/linux/sunrpc/svc_rdma.h | 8 ++
include/linux/sunrpc/svc_xprt.h | 3 +
include/trace/events/rpcrdma.h | 26 +++++
include/trace/events/sunrpc.h | 20 +++-
net/sunrpc/svc.c | 38 +++++--
net/sunrpc/svcsock.c | 8 ++
net/sunrpc/xdr.c | 32 +++---
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 37 +++++-
net/sunrpc/xprtrdma/svc_rdma_rw.c | 139 ++++++++++++++++++++---
net/sunrpc/xprtrdma/svc_rdma_transport.c | 1 +
19 files changed, 292 insertions(+), 74 deletions(-)
--
Chuck Lever
Refactor.
The new hook is a no-op at the moment, but it will soon be used to
pull RDMA Read chunks such that the payload is aligned to the pages
in the target file's page cache.
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfs3xdr.c | 4 ++++
fs/nfsd/nfs4xdr.c | 6 ++++++
fs/nfsd/nfsxdr.c | 4 ++++
include/linux/sunrpc/svc.h | 3 +++
include/linux/sunrpc/svc_rdma.h | 2 ++
include/linux/sunrpc/svc_xprt.h | 3 +++
net/sunrpc/svc.c | 20 ++++++++++++++++++++
net/sunrpc/svcsock.c | 8 ++++++++
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 22 ++++++++++++++++++++++
net/sunrpc/xprtrdma/svc_rdma_transport.c | 1 +
10 files changed, 73 insertions(+)
diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
index 91d4e2b8b854..1bb61a7d125a 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -642,6 +642,8 @@ nfs3svc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p)
args->count = max_blocksize;
args->len = max_blocksize;
}
+ if (svc_decode_argument_payload(rqstp, args->offset, args->len) < 0)
+ return 0;
if (!xdr_stream_subsegment(xdr, &args->payload, args->count))
return 0;
@@ -705,6 +707,8 @@ nfs3svc_decode_symlinkargs(struct svc_rqst *rqstp, __be32 *p)
remaining -= xdr_stream_pos(xdr);
if (remaining < xdr_align_size(args->tlen))
return 0;
+ if (svc_decode_argument_payload(rqstp, 0, args->tlen) < 0)
+ return 0;
args->first.iov_base = xdr->p;
args->first.iov_len = head->iov_len - xdr_stream_pos(xdr);
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 7abeccb975b2..e919f409cb9c 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -813,6 +813,9 @@ nfsd4_decode_create(struct nfsd4_compoundargs *argp, struct nfsd4_create *create
p = xdr_inline_decode(argp->xdr, create->cr_datalen);
if (!p)
return nfserr_bad_xdr;
+ if (svc_decode_argument_payload(argp->rqstp, 0,
+ create->cr_datalen) < 0)
+ return nfserr_bad_xdr;
create->cr_data = svcxdr_dupstr(argp, p, create->cr_datalen);
if (!create->cr_data)
return nfserr_jukebox;
@@ -1410,6 +1413,9 @@ nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write)
return nfserr_bad_xdr;
if (xdr_stream_decode_u32(argp->xdr, &write->wr_buflen) < 0)
return nfserr_bad_xdr;
+ if (svc_decode_argument_payload(argp->rqstp, write->wr_offset,
+ write->wr_buflen) < 0)
+ return nfserr_bad_xdr;
if (!xdr_stream_subsegment(argp->xdr, &write->wr_payload, write->wr_buflen))
return nfserr_bad_xdr;
diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
index 26a42f87c240..7c15f31f91da 100644
--- a/fs/nfsd/nfsxdr.c
+++ b/fs/nfsd/nfsxdr.c
@@ -343,6 +343,8 @@ nfssvc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p)
return 0;
if (args->len > NFSSVC_MAXBLKSIZE_V2)
return 0;
+ if (svc_decode_argument_payload(rqstp, args->offset, args->len) < 0)
+ return 0;
if (!xdr_stream_subsegment(xdr, &args->payload, args->len))
return 0;
@@ -397,6 +399,8 @@ nfssvc_decode_symlinkargs(struct svc_rqst *rqstp, __be32 *p)
if (args->tlen == 0)
return 0;
+ if (svc_decode_argument_payload(rqstp, 0, args->tlen) < 0)
+ return 0;
args->first.iov_len = head->iov_len - xdr_stream_pos(xdr);
args->first.iov_base = xdr_inline_decode(xdr, args->tlen);
if (!args->first.iov_base)
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 5a277acf2667..59d5016d9ec4 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -528,6 +528,9 @@ void svc_reserve(struct svc_rqst *rqstp, int space);
struct svc_pool * svc_pool_for_cpu(struct svc_serv *serv, int cpu);
char * svc_print_addr(struct svc_rqst *, char *, size_t);
const char * svc_proc_name(const struct svc_rqst *rqstp);
+int svc_decode_argument_payload(struct svc_rqst *rqstp,
+ unsigned int offset,
+ unsigned int length);
int svc_encode_result_payload(struct svc_rqst *rqstp,
unsigned int offset,
unsigned int length);
diff --git a/include/linux/sunrpc/svc_rdma.h b/include/linux/sunrpc/svc_rdma.h
index 24aa159d29a7..f660244cc8ba 100644
--- a/include/linux/sunrpc/svc_rdma.h
+++ b/include/linux/sunrpc/svc_rdma.h
@@ -178,6 +178,8 @@ extern void svc_rdma_recv_ctxt_put(struct svcxprt_rdma *rdma,
extern void svc_rdma_flush_recv_queues(struct svcxprt_rdma *rdma);
extern void svc_rdma_release_rqst(struct svc_rqst *rqstp);
extern int svc_rdma_recvfrom(struct svc_rqst *);
+extern int svc_rdma_argument_payload(struct svc_rqst *rqstp,
+ unsigned int offset, unsigned int length);
/* svc_rdma_rw.c */
extern void svc_rdma_destroy_rw_ctxts(struct svcxprt_rdma *rdma);
diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
index 571f605bc91e..2d4c61f3307e 100644
--- a/include/linux/sunrpc/svc_xprt.h
+++ b/include/linux/sunrpc/svc_xprt.h
@@ -20,6 +20,9 @@ struct svc_xprt_ops {
struct svc_xprt *(*xpo_accept)(struct svc_xprt *);
int (*xpo_has_wspace)(struct svc_xprt *);
int (*xpo_recvfrom)(struct svc_rqst *);
+ int (*xpo_argument_payload)(struct svc_rqst *rqstp,
+ unsigned int offset,
+ unsigned int length);
int (*xpo_sendto)(struct svc_rqst *);
int (*xpo_result_payload)(struct svc_rqst *, unsigned int,
unsigned int);
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 3d9f9da98aed..6eca2f420371 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1673,6 +1673,26 @@ const char *svc_proc_name(const struct svc_rqst *rqstp)
}
+/**
+ * svc_decode_argument_payload - set up pages containing an argument
+ * @rqstp: svc_rqst to operate on
+ * @offset: byte offset of payload in file's page cache
+ * @length: size of payload, in bytes
+ *
+ * This function can modify rqstp->rq_arg.page_base and the content
+ * of rqstp->rq_arg.pages, but no other fields of rq_arg are changed.
+ *
+ * Returns zero on success, or a negative errno if a permanent error
+ * occurred.
+ */
+int svc_decode_argument_payload(struct svc_rqst *rqstp, unsigned int offset,
+ unsigned int length)
+{
+ return rqstp->rq_xprt->xpt_ops->xpo_argument_payload(rqstp, offset,
+ length);
+}
+EXPORT_SYMBOL_GPL(svc_decode_argument_payload);
+
/**
* svc_encode_result_payload - mark a range of bytes as a result payload
* @rqstp: svc_rqst to operate on
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 478f857cdaed..05eb63c182fd 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -181,6 +181,12 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
}
}
+static int svc_sock_argument_payload(struct svc_rqst *rqstp,
+ unsigned int offset, unsigned int length)
+{
+ return 0;
+}
+
static int svc_sock_result_payload(struct svc_rqst *rqstp, unsigned int offset,
unsigned int length)
{
@@ -637,6 +643,7 @@ static struct svc_xprt *svc_udp_create(struct svc_serv *serv,
static const struct svc_xprt_ops svc_udp_ops = {
.xpo_create = svc_udp_create,
.xpo_recvfrom = svc_udp_recvfrom,
+ .xpo_argument_payload = svc_sock_argument_payload,
.xpo_sendto = svc_udp_sendto,
.xpo_result_payload = svc_sock_result_payload,
.xpo_release_rqst = svc_udp_release_rqst,
@@ -1208,6 +1215,7 @@ static struct svc_xprt *svc_tcp_create(struct svc_serv *serv,
static const struct svc_xprt_ops svc_tcp_ops = {
.xpo_create = svc_tcp_create,
.xpo_recvfrom = svc_tcp_recvfrom,
+ .xpo_argument_payload = svc_sock_argument_payload,
.xpo_sendto = svc_tcp_sendto,
.xpo_result_payload = svc_sock_result_payload,
.xpo_release_rqst = svc_tcp_release_rqst,
diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
index cf76a6ad127b..08a620b370ae 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
@@ -867,3 +867,25 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
svc_rdma_recv_ctxt_put(rdma_xprt, ctxt);
return 0;
}
+
+/**
+ * svc_rdma_argument_payload - special processing for an argument payload
+ * @rqstp: svc_rqst to operate on
+ * @offset: offset of payload in file's page cache
+ * @length: size of payload, in bytes
+ *
+ * Pull an RDMA Read payload chunk into rqstp->rq_arg.
+ *
+ * Return values:
+ * %0 if successful or nothing needed to be done
+ * %-EMSGSIZE on XDR buffer overflow
+ * %-EINVAL if client provided too many segments
+ * %-ENOMEM if rdma_rw context pool was exhausted
+ * %-ENOTCONN if posting failed (connection is lost)
+ * %-EIO if rdma_rw initialization failed (DMA mapping, etc)
+ */
+int svc_rdma_argument_payload(struct svc_rqst *rqstp, unsigned int offset,
+ unsigned int length)
+{
+ return 0;
+}
diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
index 94b20fb47135..ee7cc112504c 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
@@ -79,6 +79,7 @@ static void svc_rdma_kill_temp_xprt(struct svc_xprt *);
static const struct svc_xprt_ops svc_rdma_ops = {
.xpo_create = svc_rdma_create,
.xpo_recvfrom = svc_rdma_recvfrom,
+ .xpo_argument_payload = svc_rdma_argument_payload,
.xpo_sendto = svc_rdma_sendto,
.xpo_result_payload = svc_rdma_result_payload,
.xpo_release_rqst = svc_rdma_release_rqst,
xdr_stream_subsegment() was introduced in commit c1346a1216ab
("NFSD: Replace the internals of the READ_BUF() macro").
There are two call sites for xdr_stream_subsegment(). One is
nfsd4_decode_write(), and the other is nfsd4_decode_setxattr().
Currently neither of these call sites calls this API when
xdr_buf::page_base is a non-zero value.
However, I'm about to add a case where page_base will sometimes not
be zero when nfsd4_decode_write() invokes this API. Replace the
logic in xdr_stream_subsegment() that advances to the next data item
in the xdr_stream with something more generic in order to handle
this new use case.
Signed-off-by: Chuck Lever <[email protected]>
---
net/sunrpc/xdr.c | 32 +++++++++++++++++---------------
1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index ca10ba2626f2..df194cc07035 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1633,7 +1633,7 @@ EXPORT_SYMBOL_GPL(xdr_buf_subsegment);
* Sets up @subbuf to represent a portion of @xdr. The portion
* starts at the current offset in @xdr, and extends for a length
* of @nbytes. If this is successful, @xdr is advanced to the next
- * position following that portion.
+ * XDR data item following that portion.
*
* Return values:
* %true: @subbuf has been initialized, and @xdr has been advanced.
@@ -1642,29 +1642,31 @@ EXPORT_SYMBOL_GPL(xdr_buf_subsegment);
bool xdr_stream_subsegment(struct xdr_stream *xdr, struct xdr_buf *subbuf,
unsigned int nbytes)
{
- unsigned int remaining, offset, len;
+ unsigned int start = xdr_stream_pos(xdr);
+ unsigned int remaining, len;
- if (xdr_buf_subsegment(xdr->buf, subbuf, xdr_stream_pos(xdr), nbytes))
+ /* Extract @subbuf and bounds-check the fn arguments */
+ if (xdr_buf_subsegment(xdr->buf, subbuf, start, nbytes))
return false;
- if (subbuf->head[0].iov_len)
- if (!__xdr_inline_decode(xdr, subbuf->head[0].iov_len))
- return false;
-
- remaining = subbuf->page_len;
- offset = subbuf->page_base;
- while (remaining) {
- len = min_t(unsigned int, remaining, PAGE_SIZE) - offset;
-
+ /* Advance @xdr by @nbytes */
+ for (remaining = nbytes; remaining;) {
if (xdr->p == xdr->end && !xdr_set_next_buffer(xdr))
return false;
- if (!__xdr_inline_decode(xdr, len))
- return false;
+ len = (char *)xdr->end - (char *)xdr->p;
+ if (remaining <= len) {
+ xdr->p = (__be32 *)((char *)xdr->p +
+ (remaining + xdr_pad_size(nbytes)));
+ break;
+ }
+
+ xdr->p = (__be32 *)((char *)xdr->p + len);
+ xdr->end = xdr->p;
remaining -= len;
- offset = 0;
}
+ xdr_stream_set_pos(xdr, start + nbytes);
return true;
}
EXPORT_SYMBOL_GPL(xdr_stream_subsegment);
On Tue, Aug 31, 2021 at 03:05:09PM -0400, Chuck Lever wrote:
> Hi Bruce-
>
> Here is part of what we discussed recently about trying to align
> pages in NFS WRITE requests so that splice can be used. This
> series updates server-side RPC and svcrdma to ensure that an
> aligned xdr_buf::pages array is presented to NFSD, which is then
> converted into an aligned rq_vec for the VFS layer.
Seems sensible to me.
Do you have a git tree? It didn't apply cleanly to 5.14 when I tried,
but I didn't stop to figure out why.
> The next step would be to look at how to make the best use of the
> aligned rq_vec.
Have you done any performance comparison just with this?
Doesn't seem like it should make a significant difference, but it might
be interesting to check anyway.
--b.
> My naive thought is that where there is a PAGE_SIZE
> entry in rq_vec and there is no page in the file's page cache at
> that offset, the transport-provided page can be flipped into place.
> Might work for replacing whole pages as well, but baby steps first.
>
> This series has been exercised a bit with both TCP and RDMA, but no
> guarantees that it is completely bug-free. NFSv4 compounds with
> multiple WRITE payloads on RDMA are treated like TCP: the RPC
> message is contained in an unstructured stream of unaligned pages.
>
> Comments encouraged.
>
> ---
>
> Chuck Lever (6):
> SUNRPC: Capture value of xdr_buf::page_base
> SUNRPC: xdr_stream_subsegment() must handle non-zero page_bases
> NFSD: Have legacy NFSD WRITE decoders use xdr_stream_subsegment()
> SUNRPC: svc_fill_write_vector() must handle non-zero page_bases
> NFSD: Add a transport hook for pulling argument payloads
> svcrdma: Pull Read chunks in ->xpo_argument_payload
>
>
> fs/nfsd/nfs3proc.c | 3 +-
> fs/nfsd/nfs3xdr.c | 16 +--
> fs/nfsd/nfs4proc.c | 3 +-
> fs/nfsd/nfs4xdr.c | 6 +
> fs/nfsd/nfsproc.c | 3 +-
> fs/nfsd/nfsxdr.c | 13 +--
> fs/nfsd/xdr.h | 2 +-
> fs/nfsd/xdr3.h | 2 +-
> include/linux/sunrpc/svc.h | 6 +-
> include/linux/sunrpc/svc_rdma.h | 8 ++
> include/linux/sunrpc/svc_xprt.h | 3 +
> include/trace/events/rpcrdma.h | 26 +++++
> include/trace/events/sunrpc.h | 20 +++-
> net/sunrpc/svc.c | 38 +++++--
> net/sunrpc/svcsock.c | 8 ++
> net/sunrpc/xdr.c | 32 +++---
> net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 37 +++++-
> net/sunrpc/xprtrdma/svc_rdma_rw.c | 139 ++++++++++++++++++++---
> net/sunrpc/xprtrdma/svc_rdma_transport.c | 1 +
> 19 files changed, 292 insertions(+), 74 deletions(-)
>
> --
> Chuck Lever
> On Aug 31, 2021, at 4:42 PM, J. Bruce Fields <[email protected]> wrote:
>
> On Tue, Aug 31, 2021 at 03:05:09PM -0400, Chuck Lever wrote:
>> Hi Bruce-
>>
>> Here is part of what we discussed recently about trying to align
>> pages in NFS WRITE requests so that splice can be used. This
>> series updates server-side RPC and svcrdma to ensure that an
>> aligned xdr_buf::pages array is presented to NFSD, which is then
>> converted into an aligned rq_vec for the VFS layer.
>
> Seems sensible to me.
>
> Do you have a git tree?
I don't yet, but can set up a topic branch somewhere where we're
a little further along.
> It didn't apply cleanly to 5.14 when I tried,
> but I didn't stop to figure out why.
It might apply to 5.15-pre now that nfsd-5.15 has been merged.
>> The next step would be to look at how to make the best use of the
>> aligned rq_vec.
>
> Have you done any performance comparison just with this?
Not yet. I just got it behaving correctly with the usual tests.
Baby steps, sir.
> Doesn't seem like it should make a significant difference, but it might
> be interesting to check anyway.
I expect it to add a small amount of latency to NFS WRITEs, since
RDMA Reads are done just a little later than before.
> --b.
>
>> My naive thought is that where there is a PAGE_SIZE
>> entry in rq_vec and there is no page in the file's page cache at
>> that offset, the transport-provided page can be flipped into place.
>> Might work for replacing whole pages as well, but baby steps first.
>>
>> This series has been exercised a bit with both TCP and RDMA, but no
>> guarantees that it is completely bug-free. NFSv4 compounds with
>> multiple WRITE payloads on RDMA are treated like TCP: the RPC
>> message is contained in an unstructured stream of unaligned pages.
>>
>> Comments encouraged.
>>
>> ---
>>
>> Chuck Lever (6):
>> SUNRPC: Capture value of xdr_buf::page_base
>> SUNRPC: xdr_stream_subsegment() must handle non-zero page_bases
>> NFSD: Have legacy NFSD WRITE decoders use xdr_stream_subsegment()
>> SUNRPC: svc_fill_write_vector() must handle non-zero page_bases
>> NFSD: Add a transport hook for pulling argument payloads
>> svcrdma: Pull Read chunks in ->xpo_argument_payload
>>
>>
>> fs/nfsd/nfs3proc.c | 3 +-
>> fs/nfsd/nfs3xdr.c | 16 +--
>> fs/nfsd/nfs4proc.c | 3 +-
>> fs/nfsd/nfs4xdr.c | 6 +
>> fs/nfsd/nfsproc.c | 3 +-
>> fs/nfsd/nfsxdr.c | 13 +--
>> fs/nfsd/xdr.h | 2 +-
>> fs/nfsd/xdr3.h | 2 +-
>> include/linux/sunrpc/svc.h | 6 +-
>> include/linux/sunrpc/svc_rdma.h | 8 ++
>> include/linux/sunrpc/svc_xprt.h | 3 +
>> include/trace/events/rpcrdma.h | 26 +++++
>> include/trace/events/sunrpc.h | 20 +++-
>> net/sunrpc/svc.c | 38 +++++--
>> net/sunrpc/svcsock.c | 8 ++
>> net/sunrpc/xdr.c | 32 +++---
>> net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 37 +++++-
>> net/sunrpc/xprtrdma/svc_rdma_rw.c | 139 ++++++++++++++++++++---
>> net/sunrpc/xprtrdma/svc_rdma_transport.c | 1 +
>> 19 files changed, 292 insertions(+), 74 deletions(-)
>>
>> --
>> Chuck Lever
--
Chuck Lever