write_bytes_to_xdr_buf() is a generic way to place a variable-length
data item in an already-reserved spot in the encoding buffer.
However, it is costly, and here, it is unnecessary because the
data item is fixed in size, the buffer destination address is
always word-aligned, and the destination location is already in
@p.
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfs4xdr.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 2acea7792bb2..b52ea7d8313a 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -5373,8 +5373,7 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
so->so_replay.rp_buf, len);
}
status:
- /* Note that op->status is already in network byte order: */
- write_bytes_to_xdr_buf(xdr->buf, post_err_offset - 4, &op->status, 4);
+ *p = op->status;
}
/*
Refactor: Make the EOF result available in the entire NFSv4 READ
path.
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfs4xdr.c | 11 +++++------
fs/nfsd/xdr4.h | 5 +++--
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index c9468f205dad..16ae1be1bbac 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3879,7 +3879,6 @@ static __be32 nfsd4_encode_splice_read(
struct xdr_stream *xdr = resp->xdr;
struct xdr_buf *buf = xdr->buf;
int status, space_left;
- u32 eof;
__be32 nfserr;
__be32 *p = xdr->p - 2;
@@ -3888,7 +3887,8 @@ static __be32 nfsd4_encode_splice_read(
return nfserr_resource;
nfserr = nfsd_splice_read(read->rd_rqstp, read->rd_fhp,
- file, read->rd_offset, &maxcount, &eof);
+ file, read->rd_offset, &maxcount,
+ &read->rd_eof);
read->rd_length = maxcount;
if (nfserr)
goto out_err;
@@ -3899,7 +3899,7 @@ static __be32 nfsd4_encode_splice_read(
goto out_err;
}
- *(p++) = htonl(eof);
+ *(p++) = htonl(read->rd_eof);
*(p++) = htonl(maxcount);
buf->page_len = maxcount;
@@ -3943,7 +3943,6 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
struct file *file, unsigned long maxcount)
{
struct xdr_stream *xdr = resp->xdr;
- u32 eof;
int starting_len = xdr->buf->len - 8;
__be32 nfserr;
__be32 tmp;
@@ -3955,7 +3954,7 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
nfserr = nfsd_readv(resp->rqstp, read->rd_fhp, file, read->rd_offset,
resp->rqstp->rq_vec, read->rd_vlen, &maxcount,
- &eof);
+ &read->rd_eof);
read->rd_length = maxcount;
if (nfserr)
return nfserr;
@@ -3963,7 +3962,7 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
return nfserr_io;
xdr_truncate_encode(xdr, starting_len + 8 + xdr_align_size(maxcount));
- tmp = htonl(eof);
+ tmp = htonl(read->rd_eof);
write_bytes_to_xdr_buf(xdr->buf, starting_len , &tmp, 4);
tmp = htonl(maxcount);
write_bytes_to_xdr_buf(xdr->buf, starting_len + 4, &tmp, 4);
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 7b744011f2d3..6e6a89008ce1 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -302,9 +302,10 @@ struct nfsd4_read {
u32 rd_length; /* request */
int rd_vlen;
struct nfsd_file *rd_nf;
-
+
struct svc_rqst *rd_rqstp; /* response */
- struct svc_fh *rd_fhp; /* response */
+ struct svc_fh *rd_fhp; /* response */
+ u32 rd_eof; /* response */
};
struct nfsd4_readdir {
write_bytes_to_xdr_buf() is a generic way to place a variable-length
data item in an already-reserved spot in the encoding buffer.
However, it is costly. In nfsd4_encode_fattr(), it is unnecessary
because the data item is fixed in size and the buffer destination
address is always word-aligned.
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfs4xdr.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index b52ea7d8313a..e590236a60ab 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2828,10 +2828,9 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
struct kstat stat;
struct svc_fh *tempfh = NULL;
struct kstatfs statfs;
- __be32 *p;
+ __be32 *p, *attrlen_p;
int starting_len = xdr->buf->len;
int attrlen_offset;
- __be32 attrlen;
u32 dummy;
u64 dummy64;
u32 rdattr_err = 0;
@@ -2919,10 +2918,9 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
goto out;
attrlen_offset = xdr->buf->len;
- p = xdr_reserve_space(xdr, 4);
- if (!p)
+ attrlen_p = xdr_reserve_space(xdr, XDR_UNIT);
+ if (!attrlen_p)
goto out_resource;
- p++; /* to be backfilled later */
if (bmval0 & FATTR4_WORD0_SUPPORTED_ATTRS) {
u32 supp[3];
@@ -3344,8 +3342,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
*p++ = cpu_to_be32(err == 0);
}
- attrlen = htonl(xdr->buf->len - attrlen_offset - 4);
- write_bytes_to_xdr_buf(xdr->buf, attrlen_offset, &attrlen, 4);
+ *attrlen_p = cpu_to_be32(xdr->buf->len - attrlen_offset - XDR_UNIT);
status = nfs_ok;
out:
Do the test_bit() once -- this reduces the number of locked-bus
operations and makes the function a little easier to read.
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfs4xdr.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index e590236a60ab..c9468f205dad 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3980,6 +3980,7 @@ static __be32
nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
struct nfsd4_read *read)
{
+ bool splice_ok = test_bit(RQ_SPLICE_OK, &resp->rqstp->rq_flags);
unsigned long maxcount;
struct xdr_stream *xdr = resp->xdr;
struct file *file;
@@ -3992,11 +3993,10 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
p = xdr_reserve_space(xdr, 8); /* eof flag and byte count */
if (!p) {
- WARN_ON_ONCE(test_bit(RQ_SPLICE_OK, &resp->rqstp->rq_flags));
+ WARN_ON_ONCE(splice_ok);
return nfserr_resource;
}
- if (resp->xdr->buf->page_len &&
- test_bit(RQ_SPLICE_OK, &resp->rqstp->rq_flags)) {
+ if (resp->xdr->buf->page_len && splice_ok) {
WARN_ON_ONCE(1);
return nfserr_resource;
}
@@ -4005,8 +4005,7 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
maxcount = min_t(unsigned long, read->rd_length,
(xdr->buf->buflen - xdr->buf->len));
- if (file->f_op->splice_read &&
- test_bit(RQ_SPLICE_OK, &resp->rqstp->rq_flags))
+ if (file->f_op->splice_read && splice_ok)
nfserr = nfsd4_encode_splice_read(resp, read, file, maxcount);
else
nfserr = nfsd4_encode_readv(resp, read, file, maxcount);
Clean-up: Now that nfsd4_encode_readv() does not have to encode the
EOF or rd_length values, it no longer needs to subtract 8 from
@starting_len.
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfs4xdr.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 1e59d4ce529f..32f4f48458e6 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3939,7 +3939,7 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
struct file *file, unsigned long maxcount)
{
struct xdr_stream *xdr = resp->xdr;
- int starting_len = xdr->buf->len - 8;
+ unsigned int starting_len = xdr->buf->len;
__be32 nfserr;
__be32 tmp;
int pad;
@@ -3954,14 +3954,13 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
read->rd_length = maxcount;
if (nfserr)
return nfserr;
- if (svc_encode_result_payload(resp->rqstp, starting_len + 8, maxcount))
+ if (svc_encode_result_payload(resp->rqstp, starting_len, maxcount))
return nfserr_io;
- xdr_truncate_encode(xdr, starting_len + 8 + xdr_align_size(maxcount));
+ xdr_truncate_encode(xdr, starting_len + xdr_align_size(maxcount));
tmp = xdr_zero;
pad = (maxcount&3) ? 4 - (maxcount&3) : 0;
- write_bytes_to_xdr_buf(xdr->buf, starting_len + 8 + maxcount,
- &tmp, pad);
+ write_bytes_to_xdr_buf(xdr->buf, starting_len + maxcount, &tmp, pad);
return 0;
}
Clean up: Use a helper instead of open-coding the calculation of
the XDR pad size.
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfs4xdr.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 32f4f48458e6..71bac0039c42 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3940,9 +3940,8 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
{
struct xdr_stream *xdr = resp->xdr;
unsigned int starting_len = xdr->buf->len;
+ __be32 zero = xdr_zero;
__be32 nfserr;
- __be32 tmp;
- int pad;
read->rd_vlen = xdr_reserve_space_vec(xdr, resp->rqstp->rq_vec, maxcount);
if (read->rd_vlen < 0)
@@ -3958,11 +3957,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
return nfserr_io;
xdr_truncate_encode(xdr, starting_len + xdr_align_size(maxcount));
- tmp = xdr_zero;
- pad = (maxcount&3) ? 4 - (maxcount&3) : 0;
- write_bytes_to_xdr_buf(xdr->buf, starting_len + maxcount, &tmp, pad);
- return 0;
-
+ write_bytes_to_xdr_buf(xdr->buf, starting_len + maxcount, &zero,
+ xdr_pad_size(maxcount));
+ return nfs_ok;
}
static __be32
Similar changes to nfsd4_encode_readv(), all bundled into a single
patch.
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfs4xdr.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 71bac0039c42..358b3338c4cc 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -4008,16 +4008,13 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
static __be32
nfsd4_encode_readlink(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_readlink *readlink)
{
- int maxcount;
- __be32 wire_count;
- int zero = 0;
+ __be32 *p, *maxcount_p, zero = xdr_zero;
struct xdr_stream *xdr = resp->xdr;
int length_offset = xdr->buf->len;
- int status;
- __be32 *p;
+ int maxcount, status;
- p = xdr_reserve_space(xdr, 4);
- if (!p)
+ maxcount_p = xdr_reserve_space(xdr, XDR_UNIT);
+ if (!maxcount_p)
return nfserr_resource;
maxcount = PAGE_SIZE;
@@ -4042,14 +4039,11 @@ nfsd4_encode_readlink(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd
nfserr = nfserrno(status);
goto out_err;
}
-
- wire_count = htonl(maxcount);
- write_bytes_to_xdr_buf(xdr->buf, length_offset, &wire_count, 4);
- xdr_truncate_encode(xdr, length_offset + 4 + ALIGN(maxcount, 4));
- if (maxcount & 3)
- write_bytes_to_xdr_buf(xdr->buf, length_offset + 4 + maxcount,
- &zero, 4 - (maxcount&3));
- return 0;
+ *maxcount_p = cpu_to_be32(maxcount);
+ xdr_truncate_encode(xdr, length_offset + 4 + xdr_align_size(maxcount));
+ write_bytes_to_xdr_buf(xdr->buf, length_offset + 4 + maxcount, &zero,
+ xdr_pad_size(maxcount));
+ return nfs_ok;
out_err:
xdr_truncate_encode(xdr, length_offset);
write_bytes_to_xdr_buf() is pretty expensive to use for inserting
an XDR data item that is always 1 XDR_UNIT at an address that is
always XDR word-aligned.
Since both the readv and splice read paths encode EOF and maxcount
values, move both to a common code path.
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfs4xdr.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 16ae1be1bbac..1e59d4ce529f 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3880,7 +3880,6 @@ static __be32 nfsd4_encode_splice_read(
struct xdr_buf *buf = xdr->buf;
int status, space_left;
__be32 nfserr;
- __be32 *p = xdr->p - 2;
/* Make sure there will be room for padding if needed */
if (xdr->end - xdr->p < 1)
@@ -3899,9 +3898,6 @@ static __be32 nfsd4_encode_splice_read(
goto out_err;
}
- *(p++) = htonl(read->rd_eof);
- *(p++) = htonl(maxcount);
-
buf->page_len = maxcount;
buf->len += maxcount;
xdr->page_ptr += (buf->page_base + maxcount + PAGE_SIZE - 1)
@@ -3962,11 +3958,6 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
return nfserr_io;
xdr_truncate_encode(xdr, starting_len + 8 + xdr_align_size(maxcount));
- tmp = htonl(read->rd_eof);
- write_bytes_to_xdr_buf(xdr->buf, starting_len , &tmp, 4);
- tmp = htonl(maxcount);
- write_bytes_to_xdr_buf(xdr->buf, starting_len + 4, &tmp, 4);
-
tmp = xdr_zero;
pad = (maxcount&3) ? 4 - (maxcount&3) : 0;
write_bytes_to_xdr_buf(xdr->buf, starting_len + 8 + maxcount,
@@ -4008,11 +3999,14 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
nfserr = nfsd4_encode_splice_read(resp, read, file, maxcount);
else
nfserr = nfsd4_encode_readv(resp, read, file, maxcount);
-
- if (nfserr)
+ if (nfserr) {
xdr_truncate_encode(xdr, starting_len);
+ return nfserr;
+ }
- return nfserr;
+ p = xdr_encode_bool(p, read->rd_eof);
+ *p = cpu_to_be32(read->rd_length);
+ return nfs_ok;
}
static __be32
> On Jul 22, 2022, at 4:08 PM, Chuck Lever <[email protected]> wrote:
>
> write_bytes_to_xdr_buf() is a generic way to place a variable-length
> data item in an already-reserved spot in the encoding buffer.
> However, it is costly, and here, it is unnecessary because the
> data item is fixed in size, the buffer destination address is
> always word-aligned, and the destination location is already in
> @p.
>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> fs/nfsd/nfs4xdr.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 2acea7792bb2..b52ea7d8313a 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -5373,8 +5373,7 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
> so->so_replay.rp_buf, len);
> }
> status:
> - /* Note that op->status is already in network byte order: */
> - write_bytes_to_xdr_buf(xdr->buf, post_err_offset - 4, &op->status, 4);
> + *p = op->status;
> }
>
> /*
I hit "Send" too soon. Here's the cover letter for this series.
write_bytes_to_xdr_buf() is a generic mechanism by which to push
an arbitrary set of bytes to an arbitrary alignment within an
xdr_buf. It's expensive to use, though.
I found several hot paths in the server's NFSv4 reply encoders
that write a 4-byte XDR data item on a 4-byte alignment, and
thus can use the classic "*p = cpu_to_be32(yada)" form instead
of write_bytes_to_xdr_buf().
This series replaces some write_bytes_to_xdr_buf() call sites.
--
Chuck Lever
On Fri, 2022-07-22 at 20:12 +0000, Chuck Lever III wrote:
>
> > On Jul 22, 2022, at 4:08 PM, Chuck Lever <[email protected]> wrote:
> >
> > write_bytes_to_xdr_buf() is a generic way to place a variable-length
> > data item in an already-reserved spot in the encoding buffer.
> > However, it is costly, and here, it is unnecessary because the
> > data item is fixed in size, the buffer destination address is
> > always word-aligned, and the destination location is already in
> > @p.
> >
> > Signed-off-by: Chuck Lever <[email protected]>
> > ---
> > fs/nfsd/nfs4xdr.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index 2acea7792bb2..b52ea7d8313a 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -5373,8 +5373,7 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
> > so->so_replay.rp_buf, len);
> > }
> > status:
> > - /* Note that op->status is already in network byte order: */
> > - write_bytes_to_xdr_buf(xdr->buf, post_err_offset - 4, &op->status, 4);
> > + *p = op->status;
> > }
> >
> > /*
>
> I hit "Send" too soon. Here's the cover letter for this series.
>
> write_bytes_to_xdr_buf() is a generic mechanism by which to push
> an arbitrary set of bytes to an arbitrary alignment within an
> xdr_buf. It's expensive to use, though.
>
> I found several hot paths in the server's NFSv4 reply encoders
> that write a 4-byte XDR data item on a 4-byte alignment, and
> thus can use the classic "*p = cpu_to_be32(yada)" form instead
> of write_bytes_to_xdr_buf().
>
> This series replaces some write_bytes_to_xdr_buf() call sites.
>
>
> --
> Chuck Lever
>
>
>
Nice cleanup. This series looks good to me (and it's a net-negative LoC
too).
Reviewed-by: Jeff Layton <[email protected]>
> On Jul 25, 2022, at 11:36 AM, Jeff Layton <[email protected]> wrote:
>
> On Fri, 2022-07-22 at 20:12 +0000, Chuck Lever III wrote:
>>
>>> On Jul 22, 2022, at 4:08 PM, Chuck Lever <[email protected]> wrote:
>>>
>>> write_bytes_to_xdr_buf() is a generic way to place a variable-length
>>> data item in an already-reserved spot in the encoding buffer.
>>> However, it is costly, and here, it is unnecessary because the
>>> data item is fixed in size, the buffer destination address is
>>> always word-aligned, and the destination location is already in
>>> @p.
>>>
>>> Signed-off-by: Chuck Lever <[email protected]>
>>> ---
>>> fs/nfsd/nfs4xdr.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>>> index 2acea7792bb2..b52ea7d8313a 100644
>>> --- a/fs/nfsd/nfs4xdr.c
>>> +++ b/fs/nfsd/nfs4xdr.c
>>> @@ -5373,8 +5373,7 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
>>> so->so_replay.rp_buf, len);
>>> }
>>> status:
>>> - /* Note that op->status is already in network byte order: */
>>> - write_bytes_to_xdr_buf(xdr->buf, post_err_offset - 4, &op->status, 4);
>>> + *p = op->status;
>>> }
>>>
>>> /*
>>
>> I hit "Send" too soon. Here's the cover letter for this series.
>>
>> write_bytes_to_xdr_buf() is a generic mechanism by which to push
>> an arbitrary set of bytes to an arbitrary alignment within an
>> xdr_buf. It's expensive to use, though.
>>
>> I found several hot paths in the server's NFSv4 reply encoders
>> that write a 4-byte XDR data item on a 4-byte alignment, and
>> thus can use the classic "*p = cpu_to_be32(yada)" form instead
>> of write_bytes_to_xdr_buf().
>>
>> This series replaces some write_bytes_to_xdr_buf() call sites.
>>
>>
>> --
>> Chuck Lever
>>
>>
>>
>
> Nice cleanup. This series looks good to me (and it's a net-negative LoC
> too).
>
> Reviewed-by: Jeff Layton <[email protected]>
Thanks, Jeff!
--
Chuck Lever