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: <160676629926.384675.11452129892621714986.stgit@klimt.1015granger.net>
In-Reply-To: <160676629926.384675.11452129892621714986.stgit@klimt.1015granger.net>
From:   Olga Kornievskaia <aglo@umich.edu>
Date:   Mon, 30 Nov 2020 16:59:44 -0500
Message-ID: <CAN-5tyFc3vKMwn6_UcE-oUnr762j_zzP0bp-_FQx1ANbv8zqsg@mail.gmail.com>
Subject: Re: [PATCH v2] SUNRPC: Remove XDRBUF_SPARSE_PAGES flag in gss_proxy upcall
To:     Chuck Lever <chuck.lever@oracle.com>
Cc:     Simo Sorce <simo@redhat.com>,
        "J. Bruce Fields" <bfields@fieldses.org>,
        linux-nfs <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Nov 30, 2020 at 2:59 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> There's no need to defer allocation of pages for the receive buffer.
>
> - This upcall is quite infrequent
> - gssp_alloc_receive_pages() can allocate the pages with GFP_KERNEL,
>   unlike the transport
> - gssp_alloc_receive_pages() knows exactly how many pages are needed

Looks good to me now. Reviewed-by.

> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  net/sunrpc/auth_gss/gss_rpc_upcall.c |   15 ++++++++++-----
>  net/sunrpc/auth_gss/gss_rpc_xdr.c    |    1 -
>  2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/net/sunrpc/auth_gss/gss_rpc_upcall.c b/net/sunrpc/auth_gss/gss_rpc_upcall.c
> index af9c7f43859c..d1c003a25b0f 100644
> --- a/net/sunrpc/auth_gss/gss_rpc_upcall.c
> +++ b/net/sunrpc/auth_gss/gss_rpc_upcall.c
> @@ -200,7 +200,7 @@ static int gssp_call(struct net *net, struct rpc_message *msg)
>
>  static void gssp_free_receive_pages(struct gssx_arg_accept_sec_context *arg)
>  {
> -       int i;
> +       unsigned int i;
>
>         for (i = 0; i < arg->npages && arg->pages[i]; i++)
>                 __free_page(arg->pages[i]);
> @@ -210,14 +210,19 @@ static void gssp_free_receive_pages(struct gssx_arg_accept_sec_context *arg)
>
>  static int gssp_alloc_receive_pages(struct gssx_arg_accept_sec_context *arg)
>  {
> +       unsigned int i;
> +
>         arg->npages = DIV_ROUND_UP(NGROUPS_MAX * 4, PAGE_SIZE);
>         arg->pages = kcalloc(arg->npages, sizeof(struct page *), GFP_KERNEL);
> -       /*
> -        * XXX: actual pages are allocated by xdr layer in
> -        * xdr_partial_copy_from_skb.
> -        */
>         if (!arg->pages)
>                 return -ENOMEM;
> +       for (i = 0; i < arg->npages; i++) {
> +               arg->pages[i] = alloc_page(GFP_KERNEL);
> +               if (!arg->pages[i]) {
> +                       gssp_free_receive_pages(arg);
> +                       return -ENOMEM;
> +               }
> +       }
>         return 0;
>  }
>
> diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c
> index c636c648849b..d79f12c2550a 100644
> --- a/net/sunrpc/auth_gss/gss_rpc_xdr.c
> +++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c
> @@ -771,7 +771,6 @@ void gssx_enc_accept_sec_context(struct rpc_rqst *req,
>         xdr_inline_pages(&req->rq_rcv_buf,
>                 PAGE_SIZE/2 /* pretty arbitrary */,
>                 arg->pages, 0 /* page base */, arg->npages * PAGE_SIZE);
> -       req->rq_rcv_buf.flags |= XDRBUF_SPARSE_PAGES;
>  done:
>         if (err)
>                 dprintk("RPC:       gssx_enc_accept_sec_context: %d\n", err);
>
>