Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201201213128.13624-1-fllinden@amazon.com>
In-Reply-To: <20201201213128.13624-1-fllinden@amazon.com>
From:   Olga Kornievskaia <aglo@umich.edu>
Date:   Tue, 1 Dec 2020 17:03:46 -0500
Message-ID: <CAN-5tyF8P9J8DhHZhFyEq97BpjAe9bFg786QH6eF_1r0jPi-Fg@mail.gmail.com>
Subject: Re: [PATCH] NFSv4.2: improve page handling for GETXATTR
To:     Frank van der Linden <fllinden@amazon.com>
Cc:     linux-nfs <linux-nfs@vger.kernel.org>,
        Anna Schumaker <anna.schumaker@netapp.com>,
        Trond Myklebust <trond.myklebust@hammerspace.com>,
        Chuck Lever <chuck.lever@oracle.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Dec 1, 2020 at 4:44 PM Frank van der Linden <fllinden@amazon.com> wrote:
>
> XDRBUF_SPARSE_PAGES can cause problems for the RDMA transport,
> and it's easy enough to allocate enough pages for the request
> up front, so do that.
>
> Also, since we've allocated the pages anyway, use the full
> page aligned length for the receive buffer. This will allow
> caching of valid replies that are too large for the caller,
> but that still fit in the allocated pages.
>
> Signed-off-by: Frank van der Linden <fllinden@amazon.com>
> ---
>  fs/nfs/nfs42proc.c | 39 ++++++++++++++++++++++++++++++---------
>  fs/nfs/nfs42xdr.c  | 22 +++++++++++++++++-----
>  2 files changed, 47 insertions(+), 14 deletions(-)
>
> diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> index 2b2211d1234e..bfe15ac77bd9 100644
> --- a/fs/nfs/nfs42proc.c
> +++ b/fs/nfs/nfs42proc.c
> @@ -1176,11 +1176,9 @@ static ssize_t _nfs42_proc_getxattr(struct inode *inode, const char *name,
>                                 void *buf, size_t buflen)
>  {
>         struct nfs_server *server = NFS_SERVER(inode);
> -       struct page *pages[NFS4XATTR_MAXPAGES] = {};
> +       struct page **pages;
>         struct nfs42_getxattrargs arg = {
>                 .fh             = NFS_FH(inode),
> -               .xattr_pages    = pages,
> -               .xattr_len      = buflen,
>                 .xattr_name     = name,
>         };
>         struct nfs42_getxattrres res;
> @@ -1189,12 +1187,31 @@ static ssize_t _nfs42_proc_getxattr(struct inode *inode, const char *name,
>                 .rpc_argp       = &arg,
>                 .rpc_resp       = &res,
>         };
> -       int ret, np;
> +       ssize_t ret, np, i;
> +
> +       arg.xattr_len = buflen ?: XATTR_SIZE_MAX;
> +
> +       ret = -ENOMEM;
> +       np = DIV_ROUND_UP(arg.xattr_len, PAGE_SIZE);
> +
> +       pages = kcalloc(np, sizeof(struct page *), GFP_KERNEL);
> +       if (pages == NULL)
> +               return ret;
> +
> +       for (i = 0; i < np; i++) {
> +               pages[i] = alloc_page(GFP_KERNEL);
> +               if (!pages[i])
> +                       goto out_free;
> +       }
> +
> +       arg.xattr_pages = pages;
>
>         ret = nfs4_call_sync(server->client, server, &msg, &arg.seq_args,
>             &res.seq_res, 0);
>         if (ret < 0)
> -               return ret;
> +               goto out_free;
> +
> +       ret = res.xattr_len;
>
>         /*
>          * Normally, the caching is done one layer up, but for successful
> @@ -1209,16 +1226,20 @@ static ssize_t _nfs42_proc_getxattr(struct inode *inode, const char *name,
>         nfs4_xattr_cache_add(inode, name, NULL, pages, res.xattr_len);
>
>         if (buflen) {
> -               if (res.xattr_len > buflen)
> -                       return -ERANGE;
> +               if (res.xattr_len > buflen) {
> +                       ret = -ERANGE;
> +                       goto out_free;
> +               }
>                 _copy_from_pages(buf, pages, 0, res.xattr_len);
>         }
>
> -       np = DIV_ROUND_UP(res.xattr_len, PAGE_SIZE);
> +out_free:
>         while (--np >= 0)
>                 __free_page(pages[np]);

Looking at Chuck's page for listxattr, I think we need to check if
(pages[np) before freeing?

Otherwise, looks fine to me. Reviewed-by.

> -       return res.xattr_len;
> +       kfree(pages);
> +
> +       return ret;
>  }
>
>  static ssize_t _nfs42_proc_listxattrs(struct inode *inode, void *buf,
> diff --git a/fs/nfs/nfs42xdr.c b/fs/nfs/nfs42xdr.c
> index 6e060a88f98c..8dfe674d1301 100644
> --- a/fs/nfs/nfs42xdr.c
> +++ b/fs/nfs/nfs42xdr.c
> @@ -489,6 +489,12 @@ static int decode_getxattr(struct xdr_stream *xdr,
>                 return -EIO;
>
>         len = be32_to_cpup(p);
> +
> +       /*
> +        * Only check against the page length here. The actual
> +        * requested length may be smaller, but that is only
> +        * checked against after possibly caching a valid reply.
> +        */
>         if (len > req->rq_rcv_buf.page_len)
>                 return -ERANGE;
>
> @@ -1483,11 +1489,17 @@ static void nfs4_xdr_enc_getxattr(struct rpc_rqst *req, struct xdr_stream *xdr,
>         encode_putfh(xdr, args->fh, &hdr);
>         encode_getxattr(xdr, args->xattr_name, &hdr);
>
> -       plen = args->xattr_len ? args->xattr_len : XATTR_SIZE_MAX;
> -
> -       rpc_prepare_reply_pages(req, args->xattr_pages, 0, plen,
> -           hdr.replen);
> -       req->rq_rcv_buf.flags |= XDRBUF_SPARSE_PAGES;
> +       /*
> +        * The GETXATTR op has no length field in the call, and the
> +        * xattr data is at the end of the reply.
> +        *
> +        * Since the pages have already been allocated, there is no
> +        * downside in using the page-aligned length. It will allow
> +        * receiving and caching xattrs that are too large for the
> +        * caller but still fit in the page-rounded value.
> +        */
> +       plen = round_up(args->xattr_len, PAGE_SIZE);
> +       rpc_prepare_reply_pages(req, args->xattr_pages, 0, plen, hdr.replen);
>
>         encode_nops(&hdr);
>  }
> --
> 2.23.3
>