[5/6] NFSv4: simplify getacl decoding
diff mbox

Message ID 1487470070-32358-6-git-send-email-bfields@redhat.com
State New
Headers show

Commit Message

J. Bruce Fields Feb. 19, 2017, 2:07 a.m. UTC
From: "J. Bruce Fields" <bfields@redhat.com>

There's not really much point to shifting the data around with
xdr_enter_page() (and xdr_shrink_bufhead) when we're just going to
copying the data out of the xdr buf anyway instead of trying to place
the data right for zero-copy.  And it turns out if we want to leave
page allocation to the xdr_partial_copy_from_skb() (as we will in a
following patch) shifting the data brings more ocmplications.

So let's just pass that buf down to the xdr layer and let it copy data
directly into it.

That means we need to allocate our own buf in the case the user didn't
give us one, so that we can still cache in that case (to efficiently
handle the common case of a getxattr(., ., NULL, 0) to get the size
followed immediately by a getxattr(., ., buf, size)).

But this still works out to be much simpler.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfs/nfs4proc.c       | 73 +++++++++++++++++++++++++------------------------
 fs/nfs/nfs4xdr.c        | 27 ++++--------------
 include/linux/nfs_xdr.h |  4 +--
 3 files changed, 45 insertions(+), 59 deletions(-)

Comments

Andreas Gruenbacher Feb. 20, 2017, 10:30 p.m. UTC | #1
On Sun, Feb 19, 2017 at 3:07 AM, J. Bruce Fields <bfields@redhat.com> wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
>
> There's not really much point to shifting the data around with
> xdr_enter_page() (and xdr_shrink_bufhead) when we're just going to
> copying the data out of the xdr buf anyway instead of trying to place
> the data right for zero-copy.  And it turns out if we want to leave
> page allocation to the xdr_partial_copy_from_skb() (as we will in a
> following patch) shifting the data brings more ocmplications.

Typo above ("complications").

> So let's just pass that buf down to the xdr layer and let it copy data
> directly into it.
>
> That means we need to allocate our own buf in the case the user didn't
> give us one, so that we can still cache in that case (to efficiently
> handle the common case of a getxattr(., ., NULL, 0) to get the size
> followed immediately by a getxattr(., ., buf, size)).
>
> But this still works out to be much simpler.
>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>

> ---
>  fs/nfs/nfs4proc.c       | 73 +++++++++++++++++++++++++------------------------
>  fs/nfs/nfs4xdr.c        | 27 ++++--------------
>  include/linux/nfs_xdr.h |  4 +--
>  3 files changed, 45 insertions(+), 59 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index eb6c34db9a79..3e3dbba4aa74 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5041,7 +5041,7 @@ static inline ssize_t nfs4_read_cached_acl(struct inode *inode, char *buf, size_
>   */
>  #define NFS4_MAX_CACHED_ACL PAGE_SIZE
>
> -static void nfs4_write_cached_acl(struct inode *inode, struct page **pages, size_t pgbase, size_t acl_len)
> +static void nfs4_write_cached_acl(struct inode *inode, void *buf, size_t acl_len)
>  {
>         struct nfs4_cached_acl *acl;
>         size_t buflen = sizeof(*acl) + acl_len;
> @@ -5051,7 +5051,7 @@ static void nfs4_write_cached_acl(struct inode *inode, struct page **pages, size
>                 if (acl == NULL)
>                         goto out;
>                 acl->cached = 1;
> -               _copy_from_pages(acl->data, pages, pgbase, acl_len);
> +               memcpy(acl->data, buf, acl_len);
>         } else {
>                 acl = kmalloc(sizeof(*acl), GFP_KERNEL);
>                 if (acl == NULL)
> @@ -5063,26 +5063,16 @@ static void nfs4_write_cached_acl(struct inode *inode, struct page **pages, size
>         nfs4_set_cached_acl(inode, acl);
>  }
>
> -/*
> - * The getxattr API returns the required buffer length when called with a
> - * NULL buf. The NFSv4 acl tool then calls getxattr again after allocating
> - * the required buf.  On a NULL buf, we send a page of data to the server
> - * guessing that the ACL request can be serviced by a page. If so, we cache
> - * up to the page of ACL data, and the 2nd call to getxattr is serviced by
> - * the cache. If not so, we throw away the page, and cache the required
> - * length. The next getxattr call will then produce another round trip to
> - * the server, this time with the input buf of the required size.
> - */
> -static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t buflen)
> +static ssize_t nfs4_do_get_acl(struct inode *inode, void *buf, size_t buflen)
>  {
>         struct page *pages[NFS4ACL_MAXPAGES + 1] = {NULL, };
>         struct nfs_getaclargs args = {
>                 .fh = NFS_FH(inode),
>                 .acl_pages = pages,
> -               .acl_len = buflen,
>         };
>         struct nfs_getaclres res = {
>                 .acl_len = buflen,
> +               .buf = buf,
>         };
>         struct rpc_message msg = {
>                 .rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_GETACL],
> @@ -5112,27 +5102,8 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
>                 __func__, buf, buflen, npages, args.acl_len);
>         ret = nfs4_call_sync(NFS_SERVER(inode)->client, NFS_SERVER(inode),
>                              &msg, &args.seq_args, &res.seq_res, 0);
> -       if (ret)
> -               goto out_free;
> -
> -       /* Handle the case where the passed-in buffer is too short */
> -       if (res.acl_flags & NFS4_ACL_TRUNC) {
> -               /* Did the user only issue a request for the acl length? */
> -               if (buf == NULL)
> -                       goto out_ok;
> -               ret = -ERANGE;
> -               goto out_free;
> -       }
> -       nfs4_write_cached_acl(inode, pages, res.acl_data_offset, res.acl_len);
> -       if (buf) {
> -               if (res.acl_len > buflen) {
> -                       ret = -ERANGE;
> -                       goto out_free;
> -               }
> -               _copy_from_pages(buf, pages, res.acl_data_offset, res.acl_len);
> -       }
> -out_ok:
> -       ret = res.acl_len;
> +       if (ret == 0)
> +               ret = res.acl_len;
>  out_free:
>         for (i = 0; i < ARRAY_SIZE(pages) && pages[i]; i++)
>                 __free_page(pages[i]);
> @@ -5140,6 +5111,38 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
>         return ret;
>  }
>
> +/*
> + * The getxattr API returns the required buffer length when called with a
> + * NULL buf. The NFSv4 acl tool then calls getxattr again after allocating
> + * the required buf. Cache the result from the first getxattr call to avoid
> + * sending another RPC.
> + */
> +static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t buflen)
> +{
> +       void *priv_buf = NULL;
> +       void *our_buf = buf;
> +       int our_buflen = buflen;
> +       static ssize_t ret = -ENOMEM;
> +
> +       if (!buf) {
> +               priv_buf = kmalloc(NFS4_MAX_CACHED_ACL, GFP_KERNEL);
> +               if (!priv_buf)
> +                       goto out;
> +               our_buf = priv_buf;
> +               our_buflen = NFS4_MAX_CACHED_ACL;
> +       }
> +       ret = nfs4_do_get_acl(inode, our_buf, our_buflen);
> +       if (ret < 0)
> +               goto out;
> +       if (ret <= our_buflen)
> +               nfs4_write_cached_acl(inode, our_buf, ret);
> +       if (buf && ret > buflen)
> +               ret = -ERANGE;
> +out:
> +       kfree(priv_buf);
> +       return ret;
> +}
> +
>  static ssize_t nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t buflen)
>  {
>         struct nfs4_exception exception = { };
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index bb95dd2edeef..534b377084fb 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -5353,39 +5353,24 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
>         uint32_t attrlen,
>                  bitmap[3] = {0};
>         int status;
> -       unsigned int pg_offset;
>
> -       res->acl_len = 0;
>         if ((status = decode_op_hdr(xdr, OP_GETATTR)) != 0)
>                 goto out;
> -
> -       xdr_enter_page(xdr, xdr->buf->page_len);
> -
> -       /* Calculate the offset of the page data */
> -       pg_offset = xdr->buf->head[0].iov_len;
> -
>         if ((status = decode_attr_bitmap(xdr, bitmap)) != 0)
>                 goto out;
>         if ((status = decode_attr_length(xdr, &attrlen, &savep)) != 0)
>                 goto out;
> -
> +       if (unlikely(attrlen > (xdr->nwords << 2)))
> +               return -EIO;
>         if (unlikely(bitmap[0] & (FATTR4_WORD0_ACL - 1U)))
>                 return -EIO;
>         if (likely(bitmap[0] & FATTR4_WORD0_ACL)) {
> +               unsigned int offset = xdr_stream_pos(xdr);
>
> -               /* The bitmap (xdr len + bitmaps) and the attr xdr len words
> -                * are stored with the acl data to handle the problem of
> -                * variable length bitmaps.*/
> -               res->acl_data_offset = xdr_stream_pos(xdr) - pg_offset;
> +               if (attrlen <= res->acl_len)
> +                       read_bytes_from_xdr_buf(xdr->buf, offset,
> +                                               res->buf, attrlen);
>                 res->acl_len = attrlen;
> -
> -               /* Check for receive buffer overflow */
> -               if (res->acl_len > (xdr->nwords << 2) ||
> -                   res->acl_len + res->acl_data_offset > xdr->buf->page_len) {
> -                       res->acl_flags |= NFS4_ACL_TRUNC;
> -                       dprintk("NFS: acl reply: attrlen %u > page_len %u\n",
> -                                       attrlen, xdr->nwords << 2);
> -               }
>         } else
>                 status = -EOPNOTSUPP;
>
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index 348f7c158084..418116d62740 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -747,13 +747,11 @@ struct nfs_getaclargs {
>         struct page **                  acl_pages;
>  };
>
> -/* getxattr ACL interface flags */
> -#define NFS4_ACL_TRUNC         0x0001  /* ACL was truncated */
>  struct nfs_getaclres {
>         struct nfs4_sequence_res        seq_res;
> +       void *                          buf;
>         size_t                          acl_len;
>         size_t                          acl_data_offset;
> -       int                             acl_flags;
>         struct page *                   acl_scratch;
>  };
>
> --
> 2.9.3
>
--
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

Patch
diff mbox

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index eb6c34db9a79..3e3dbba4aa74 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5041,7 +5041,7 @@  static inline ssize_t nfs4_read_cached_acl(struct inode *inode, char *buf, size_
  */
 #define NFS4_MAX_CACHED_ACL PAGE_SIZE
 
-static void nfs4_write_cached_acl(struct inode *inode, struct page **pages, size_t pgbase, size_t acl_len)
+static void nfs4_write_cached_acl(struct inode *inode, void *buf, size_t acl_len)
 {
 	struct nfs4_cached_acl *acl;
 	size_t buflen = sizeof(*acl) + acl_len;
@@ -5051,7 +5051,7 @@  static void nfs4_write_cached_acl(struct inode *inode, struct page **pages, size
 		if (acl == NULL)
 			goto out;
 		acl->cached = 1;
-		_copy_from_pages(acl->data, pages, pgbase, acl_len);
+		memcpy(acl->data, buf, acl_len);
 	} else {
 		acl = kmalloc(sizeof(*acl), GFP_KERNEL);
 		if (acl == NULL)
@@ -5063,26 +5063,16 @@  static void nfs4_write_cached_acl(struct inode *inode, struct page **pages, size
 	nfs4_set_cached_acl(inode, acl);
 }
 
-/*
- * The getxattr API returns the required buffer length when called with a
- * NULL buf. The NFSv4 acl tool then calls getxattr again after allocating
- * the required buf.  On a NULL buf, we send a page of data to the server
- * guessing that the ACL request can be serviced by a page. If so, we cache
- * up to the page of ACL data, and the 2nd call to getxattr is serviced by
- * the cache. If not so, we throw away the page, and cache the required
- * length. The next getxattr call will then produce another round trip to
- * the server, this time with the input buf of the required size.
- */
-static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t buflen)
+static ssize_t nfs4_do_get_acl(struct inode *inode, void *buf, size_t buflen)
 {
 	struct page *pages[NFS4ACL_MAXPAGES + 1] = {NULL, };
 	struct nfs_getaclargs args = {
 		.fh = NFS_FH(inode),
 		.acl_pages = pages,
-		.acl_len = buflen,
 	};
 	struct nfs_getaclres res = {
 		.acl_len = buflen,
+		.buf = buf,
 	};
 	struct rpc_message msg = {
 		.rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_GETACL],
@@ -5112,27 +5102,8 @@  static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
 		__func__, buf, buflen, npages, args.acl_len);
 	ret = nfs4_call_sync(NFS_SERVER(inode)->client, NFS_SERVER(inode),
 			     &msg, &args.seq_args, &res.seq_res, 0);
-	if (ret)
-		goto out_free;
-
-	/* Handle the case where the passed-in buffer is too short */
-	if (res.acl_flags & NFS4_ACL_TRUNC) {
-		/* Did the user only issue a request for the acl length? */
-		if (buf == NULL)
-			goto out_ok;
-		ret = -ERANGE;
-		goto out_free;
-	}
-	nfs4_write_cached_acl(inode, pages, res.acl_data_offset, res.acl_len);
-	if (buf) {
-		if (res.acl_len > buflen) {
-			ret = -ERANGE;
-			goto out_free;
-		}
-		_copy_from_pages(buf, pages, res.acl_data_offset, res.acl_len);
-	}
-out_ok:
-	ret = res.acl_len;
+	if (ret == 0)
+		ret = res.acl_len;
 out_free:
 	for (i = 0; i < ARRAY_SIZE(pages) && pages[i]; i++)
 		__free_page(pages[i]);
@@ -5140,6 +5111,38 @@  static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
 	return ret;
 }
 
+/*
+ * The getxattr API returns the required buffer length when called with a
+ * NULL buf. The NFSv4 acl tool then calls getxattr again after allocating
+ * the required buf. Cache the result from the first getxattr call to avoid
+ * sending another RPC.
+ */
+static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t buflen)
+{
+	void *priv_buf = NULL;
+	void *our_buf = buf;
+	int our_buflen = buflen;
+	static ssize_t ret = -ENOMEM;
+
+	if (!buf) {
+		priv_buf = kmalloc(NFS4_MAX_CACHED_ACL, GFP_KERNEL);
+		if (!priv_buf)
+			goto out;
+		our_buf = priv_buf;
+		our_buflen = NFS4_MAX_CACHED_ACL;
+	}
+	ret = nfs4_do_get_acl(inode, our_buf, our_buflen);
+	if (ret < 0)
+		goto out;
+	if (ret <= our_buflen)
+		nfs4_write_cached_acl(inode, our_buf, ret);
+	if (buf && ret > buflen)
+		ret = -ERANGE;
+out:
+	kfree(priv_buf);
+	return ret;
+}
+
 static ssize_t nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t buflen)
 {
 	struct nfs4_exception exception = { };
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index bb95dd2edeef..534b377084fb 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -5353,39 +5353,24 @@  static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
 	uint32_t attrlen,
 		 bitmap[3] = {0};
 	int status;
-	unsigned int pg_offset;
 
-	res->acl_len = 0;
 	if ((status = decode_op_hdr(xdr, OP_GETATTR)) != 0)
 		goto out;
-
-	xdr_enter_page(xdr, xdr->buf->page_len);
-
-	/* Calculate the offset of the page data */
-	pg_offset = xdr->buf->head[0].iov_len;
-
 	if ((status = decode_attr_bitmap(xdr, bitmap)) != 0)
 		goto out;
 	if ((status = decode_attr_length(xdr, &attrlen, &savep)) != 0)
 		goto out;
-
+	if (unlikely(attrlen > (xdr->nwords << 2)))
+		return -EIO;
 	if (unlikely(bitmap[0] & (FATTR4_WORD0_ACL - 1U)))
 		return -EIO;
 	if (likely(bitmap[0] & FATTR4_WORD0_ACL)) {
+		unsigned int offset = xdr_stream_pos(xdr);
 
-		/* The bitmap (xdr len + bitmaps) and the attr xdr len words
-		 * are stored with the acl data to handle the problem of
-		 * variable length bitmaps.*/
-		res->acl_data_offset = xdr_stream_pos(xdr) - pg_offset;
+		if (attrlen <= res->acl_len)
+			read_bytes_from_xdr_buf(xdr->buf, offset,
+						res->buf, attrlen);
 		res->acl_len = attrlen;
-
-		/* Check for receive buffer overflow */
-		if (res->acl_len > (xdr->nwords << 2) ||
-		    res->acl_len + res->acl_data_offset > xdr->buf->page_len) {
-			res->acl_flags |= NFS4_ACL_TRUNC;
-			dprintk("NFS: acl reply: attrlen %u > page_len %u\n",
-					attrlen, xdr->nwords << 2);
-		}
 	} else
 		status = -EOPNOTSUPP;
 
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index 348f7c158084..418116d62740 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -747,13 +747,11 @@  struct nfs_getaclargs {
 	struct page **			acl_pages;
 };
 
-/* getxattr ACL interface flags */
-#define NFS4_ACL_TRUNC		0x0001	/* ACL was truncated */
 struct nfs_getaclres {
 	struct nfs4_sequence_res	seq_res;
+	void *				buf;
 	size_t				acl_len;
 	size_t				acl_data_offset;
-	int				acl_flags;
 	struct page *			acl_scratch;
 };