diff mbox

[23/50] nfsd4: "backfill" using write_bytes_to_xdr_buf

Message ID 1395537141-10389-24-git-send-email-bfields@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bruce Fields March 23, 2014, 1:11 a.m. UTC 
From: "J. Bruce Fields" <bfields@redhat.com>

Normally xdr encoding proceeds in a single pass from start of a buffer
to end, but sometimes we have to write a few bytes to an earlier
position.

Use write_bytes_to_xdr_buf for these cases rather than saving a pointer
to write to.  We plan to rewrite xdr_reserve_space to handle encoding
across page boundaries using a scratch buffer, and don't want to risk
writing to a pointer that was contained in a scratch buffer.

Also it will no longer be safe to calculate lengths by subtracting two
pointers, so use xdr_buf offsets instead.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4xdr.c | 45 ++++++++++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 19 deletions(-)

Comments

Christoph Hellwig March 23, 2014, 6:51 a.m. UTC | #1 
Looks good, but shouldn't there be an abstract function to skip writing
into an XDR buf instead of the pointer increment as well?

I wonder if the XDR buf internal should even be entirely hidden to the
consummers and it should only be accessed through procedural interfaces?

--
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
J. Bruce Fields March 23, 2014, 2:43 p.m. UTC | #2 
On Sat, Mar 22, 2014 at 11:51:54PM -0700, Christoph Hellwig wrote:
> Looks good, but shouldn't there be an abstract function to skip writing
> into an XDR buf instead of the pointer increment as well?

So you're talking about the earlier spot in the code where we've got a

	p++; /* to be filled in later? */

I'm not convinced it'd be worth it, but maybe.

> I wonder if the XDR buf internal should even be entirely hidden to the
> consummers and it should only be accessed through procedural interfaces?

By the end of the series, most xdr encoding looks like

	p = xdr_reserve_space(xdr, <nbytes>);

	*p++ = cpu_to_be32(<some value>);
	p = xdr_encode_something_more_complicated(p, <some struct>);
	...

It doesn't require the encoders to know much about xdr_buf's.

The main exception is read, especially the splice case.  We could
probably do better there, I just haven't thought much about it yet.

Hm, but maybe what you're asking for here is just a
write_bytes_to_xdr_buf wrapper that takes the xdr_stream instead of an
xdr_buf, and a corresponding function to read xdr->buf->len when we want
an offset?  OK, maybe so.

--b.
--
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
Christoph Hellwig March 23, 2014, 2:52 p.m. UTC | #3 
On Sun, Mar 23, 2014 at 10:43:44AM -0400, J. Bruce Fields wrote:
> By the end of the series, most xdr encoding looks like
> 
> 	p = xdr_reserve_space(xdr, <nbytes>);
> 
> 	*p++ = cpu_to_be32(<some value>);
> 	p = xdr_encode_something_more_complicated(p, <some struct>);
> 	...
> 
> It doesn't require the encoders to know much about xdr_buf's.
> 
> The main exception is read, especially the splice case.  We could
> probably do better there, I just haven't thought much about it yet.
> 
> Hm, but maybe what you're asking for here is just a
> write_bytes_to_xdr_buf wrapper that takes the xdr_stream instead of an
> xdr_buf, and a corresponding function to read xdr->buf->len when we want
> an offset?  OK, maybe so.

I was thinking of something like that, but looking over the rest of the
series I don't think it fits in here.  I like the way where your series
is moving the XDR code, and that seems enough for now.

--
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 mbox

Patch

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index e5cc354..43c2a99 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1759,16 +1759,19 @@  static void write_cinfo(__be32 **p, struct nfsd4_change_info *c)
 static __be32 nfsd4_encode_components_esc(struct xdr_stream *xdr, char sep, char *components, char esc_enter, char esc_exit)
 {
 	__be32 *p;
-	__be32 *countp;
+	__be32 pathlen;
+	int pathlen_offset;
 	int strlen, count=0;
 	char *str, *end, *next;
 
 	dprintk("nfsd4_encode_components(%s)\n", components);
+
+	pathlen_offset = xdr->buf->len;
 	p = xdr_reserve_space(xdr, 4);
 	if (!p)
 		return nfserr_resource;
-	countp = p;
-	WRITE32(0); /* We will fill this in with @count later */
+	p++; /* We will fill this in with @count later */
+
 	end = str = components;
 	while (*end) {
 		bool found_esc = false;
@@ -1801,8 +1804,8 @@  static __be32 nfsd4_encode_components_esc(struct xdr_stream *xdr, char sep, char
 			end++;
 		str = end;
 	}
-	p = countp;
-	WRITE32(count);
+	pathlen = htonl(xdr->buf->len - pathlen_offset);
+	write_bytes_to_xdr_buf(xdr->buf, pathlen_offset, &pathlen, 4);
 	return 0;
 }
 
@@ -2044,7 +2047,8 @@  nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp, struct svc_export
 	struct kstatfs statfs;
 	__be32 *p;
 	int starting_len = xdr->buf->len;
-	__be32 *attrlenp;
+	int attrlen_offset;
+	__be32 attrlen;
 	u32 dummy;
 	u64 dummy64;
 	u32 rdattr_err = 0;
@@ -2149,10 +2153,12 @@  nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp, struct svc_export
 		WRITE32(1);
 		WRITE32(bmval0);
 	}
+
+	attrlen_offset = xdr->buf->len;
 	p = xdr_reserve_space(xdr, 4);
 	if (!p)
 		goto out_resource;
-	attrlenp = p++;                /* to be backfilled later */
+	p++;                /* to be backfilled later */
 
 	if (bmval0 & FATTR4_WORD0_SUPPORTED_ATTRS) {
 		u32 word0 = nfsd_suppattrs0(minorversion);
@@ -2523,7 +2529,8 @@  out_acl:
 		WRITE32(NFSD_SUPPATTR_EXCLCREAT_WORD2);
 	}
 
-	*attrlenp = htonl((char *)xdr->p - (char *)attrlenp - 4);
+	attrlen = htonl(xdr->buf->len - attrlen_offset - 4);
+	write_bytes_to_xdr_buf(xdr->buf, attrlen_offset, &attrlen, 4);
 	status = nfs_ok;
 
 out:
@@ -3679,16 +3686,16 @@  __be32 nfsd4_check_resp_size(struct nfsd4_compoundres *resp, u32 pad)
 void
 nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
 {
+	struct xdr_stream *xdr = &resp->xdr;
 	struct nfs4_stateowner *so = resp->cstate.replay_owner;
 	struct svc_rqst *rqstp = resp->rqstp;
-	__be32 *statp;
+	int post_err_offset;
 	nfsd4_enc encoder;
 	__be32 *p;
 
 	RESERVE_SPACE(8);
 	WRITE32(op->opnum);
-	statp = p++;	/* to be backfilled at the end */
-	ADJUST_ARGS();
+	post_err_offset = xdr->buf->len;
 
 	if (op->opnum == OP_ILLEGAL)
 		goto status;
@@ -3727,19 +3734,19 @@  nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
 			       nfsd4_op_name(op->opnum));
 			WARN_ON_ONCE(1);
 		}
-		resp->xdr.p = statp + 1;
+		xdr_truncate_encode(xdr, post_err_offset);
 	}
 	if (so) {
+		int len = xdr->buf->len - post_err_offset;
+
 		so->so_replay.rp_status = op->status;
-		so->so_replay.rp_buflen = (char *)resp->xdr.p - (char *)(statp+1);
-		memcpy(so->so_replay.rp_buf, statp+1, so->so_replay.rp_buflen);
+		so->so_replay.rp_buflen = len;
+		read_bytes_from_xdr_buf(xdr->buf, post_err_offset,
+						so->so_replay.rp_buf, len);
 	}
 status:
-	/*
-	 * Note: We write the status directly, instead of using WRITE32(),
-	 * since it is already in network byte order.
-	 */
-	*statp = op->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);
 }
 
 /*