On Sun, Feb 19, 2017 at 3:07 AM, J. Bruce Fields <bfields@xxxxxxxxxx> wrote:
> From: "J. Bruce Fields" <bfields@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
Reviewed-by: Andreas Gruenbacher <agruenba@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
