Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
IronPort-SDR: PUw4Fpjs8TpfwBL9gxTCkc+VlojwYQ6+8Yvw3lYmTwkAHhjfEbP1P296FAVVTFrjzy8c8SLfDK
 1L/MAX0eI6vw==
Subject: Re: [PATCH 11/14] nfsd: add user xattr RPC XDR encoding/decoding logic
Date:   Fri, 20 Mar 2020 16:47:37 +0000
From:   Frank van der Linden <fllinden@amazon.com>
To:     Chuck Lever <chuck.lever@oracle.com>
CC:     Bruce Fields <bfields@fieldses.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Message-ID: <20200320164737.GA19415@dev-dsk-fllinden-2c-c1893d73.us-west-2.amazon.com>
References: <20200311195954.27117-1-fllinden@amazon.com>
 <20200311195954.27117-12-fllinden@amazon.com>
 <6955728A-CFCC-40FC-9E02-671255EDD45F@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <6955728A-CFCC-40FC-9E02-671255EDD45F@oracle.com>
User-Agent: Mutt/1.11.4 (2019-03-13)
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

Hi Chuck,

On Thu, Mar 12, 2020 at 03:16:37PM -0400, Chuck Lever wrote:
> > +static __be32
> > +nfsd4_decode_setxattr(struct nfsd4_compoundargs *argp,
> > +                   struct nfsd4_setxattr *setxattr)
> > +{
> > +     DECODE_HEAD;
> > +     u32 flags, maxcount, size;
> > +     struct kvec head;
> > +     struct page **pagelist;
> > +
> > +     READ_BUF(4);
> > +     flags = be32_to_cpup(p++);
> > +
> > +     if (flags > SETXATTR4_REPLACE)
> > +             return nfserr_inval;
> > +     setxattr->setxa_flags = flags;
> > +
> > +     status = nfsd4_decode_xattr_name(argp, &setxattr->setxa_name);
> > +     if (status)
> > +             return status;
> > +
> > +     maxcount = svc_max_payload(argp->rqstp);
> > +     maxcount = min_t(u32, XATTR_SIZE_MAX, maxcount);
> > +
> > +     READ_BUF(4);
> > +     size = be32_to_cpup(p++);
> > +     if (size > maxcount)
> > +             return nfserr_xattr2big;
> > +
> > +     setxattr->setxa_len = size;
> > +     if (size > 0) {
> > +             status = svcxdr_construct_vector(argp, &head, &pagelist, size);
> > +             if (status)
> > +                     return status;
> > +
> > +             status = nfsd4_vbuf_from_stream(argp, &head, pagelist,
> > +                 &setxattr->setxa_buf, size);
> > +     }
> 
> Now I'm wondering if read_bytes_from_xdr_buf() might be adequate
> for this purpose, so you can avoid open-coding all of this logic.

This took a little longer, I had to check my notes, but basically the
reasons for doing it this way are:

* The nfsd decode path uses nfsd4_compoundargs, which doesn't have an
  xdr_stream (it has a page array). So read_bytes_from_xdr_buf isn't
  a natural fit.
* READ_BUF/read_buf don't deal with > PAGE_SIZE chunks, but xattrs may
  be larger than that.

The other code that deals with > PAGE_SIZE chunks is the write code. So,
I factored out some code from the write code, used that, and added a function
to process the resulting kvec / pagelist (nfs4_vbuf_from_stream).

There definitely seem to be several copy functions in both the client
and server code that basically do the same, but in slightly different ways,
depending on whether they use an XDR buf or not, whether the pages are
mapped or not, etc. Seems like a good candidate for a cleanup, but I
considered it to be out of scope for these patches.

- Frank