diff mbox series

[2/3] NFS: Allocate a scratch page for READ_PLUS

Message ID 20201203201841.103294-3-Anna.Schumaker@Netapp.com (mailing list archive)
State New, archived
Headers show
Series NFS: Disable READ_PLUS by default | expand

Commit Message

Anna Schumaker Dec. 3, 2020, 8:18 p.m. UTC 
From: Anna Schumaker <Anna.Schumaker@Netapp.com>

We might need this to better handle shifting around data in the reply
buffer.

Suggested-by: Trond Myklebust <trond.myklebust@hammerspace.com>
Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
 fs/nfs/nfs42xdr.c       |  2 ++
 fs/nfs/read.c           | 13 +++++++++++--
 include/linux/nfs_xdr.h |  1 +
 3 files changed, 14 insertions(+), 2 deletions(-)

Comments

Chuck Lever Dec. 3, 2020, 8:27 p.m. UTC | #1 
> On Dec 3, 2020, at 3:18 PM, schumaker.anna@gmail.com wrote:
> 
> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> 
> We might need this to better handle shifting around data in the reply
> buffer.
> 
> Suggested-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> ---
> fs/nfs/nfs42xdr.c       |  2 ++
> fs/nfs/read.c           | 13 +++++++++++--
> include/linux/nfs_xdr.h |  1 +
> 3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/nfs42xdr.c b/fs/nfs/nfs42xdr.c
> index 8432bd6b95f0..ef095a5f86f7 100644
> --- a/fs/nfs/nfs42xdr.c
> +++ b/fs/nfs/nfs42xdr.c
> @@ -1297,6 +1297,8 @@ static int nfs4_xdr_dec_read_plus(struct rpc_rqst *rqstp,
> 	struct compound_hdr hdr;
> 	int status;
> 
> +	xdr_set_scratch_buffer(xdr, page_address(res->scratch), PAGE_SIZE);

I intend to submit this for v5.11:

https://git.linux-nfs.org/?p=cel/cel-2.6.git;a=commit;h=0ae4c3e8a64ace1b8d7de033b0751afe43024416

But seems like enough callers need a scratch buffer that the XDR
layer should set up it transparently for all requests.


> +
> 	status = decode_compound_hdr(xdr, &hdr);
> 	if (status)
> 		goto out;
> diff --git a/fs/nfs/read.c b/fs/nfs/read.c
> index eb854f1f86e2..012deb5a136f 100644
> --- a/fs/nfs/read.c
> +++ b/fs/nfs/read.c
> @@ -37,15 +37,24 @@ static struct kmem_cache *nfs_rdata_cachep;
> 
> static struct nfs_pgio_header *nfs_readhdr_alloc(void)
> {
> -	struct nfs_pgio_header *p = kmem_cache_zalloc(nfs_rdata_cachep, GFP_KERNEL);
> +	struct nfs_pgio_header *p;
> +	struct page *page;
> 
> -	if (p)
> +	page = alloc_page(GFP_KERNEL);
> +	if (!page)
> +		return ERR_PTR(-ENOMEM);
> +
> +	p = kmem_cache_zalloc(nfs_rdata_cachep, GFP_KERNEL);
> +	if (p) {
> 		p->rw_mode = FMODE_READ;
> +		p->res.scratch = page;
> +	}
> 	return p;
> }
> 
> static void nfs_readhdr_free(struct nfs_pgio_header *rhdr)
> {
> +	__free_page(rhdr->res.scratch);
> 	kmem_cache_free(nfs_rdata_cachep, rhdr);
> }
> 
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index d63cb862d58e..e0a1c97f11a9 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -659,6 +659,7 @@ struct nfs_pgio_res {
> 	struct nfs_fattr *	fattr;
> 	__u64			count;
> 	__u32			op_status;
> +	struct page *		scratch;
> 	union {
> 		struct {
> 			unsigned int		replen;		/* used by read */
> -- 
> 2.29.2
> 

--
Chuck Lever
Anna Schumaker Dec. 3, 2020, 8:31 p.m. UTC | #2 
On Thu, Dec 3, 2020 at 3:27 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
>
>
> > On Dec 3, 2020, at 3:18 PM, schumaker.anna@gmail.com wrote:
> >
> > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> >
> > We might need this to better handle shifting around data in the reply
> > buffer.
> >
> > Suggested-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > ---
> > fs/nfs/nfs42xdr.c       |  2 ++
> > fs/nfs/read.c           | 13 +++++++++++--
> > include/linux/nfs_xdr.h |  1 +
> > 3 files changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/nfs/nfs42xdr.c b/fs/nfs/nfs42xdr.c
> > index 8432bd6b95f0..ef095a5f86f7 100644
> > --- a/fs/nfs/nfs42xdr.c
> > +++ b/fs/nfs/nfs42xdr.c
> > @@ -1297,6 +1297,8 @@ static int nfs4_xdr_dec_read_plus(struct rpc_rqst *rqstp,
> >       struct compound_hdr hdr;
> >       int status;
> >
> > +     xdr_set_scratch_buffer(xdr, page_address(res->scratch), PAGE_SIZE);
>
> I intend to submit this for v5.11:
>
> https://git.linux-nfs.org/?p=cel/cel-2.6.git;a=commit;h=0ae4c3e8a64ace1b8d7de033b0751afe43024416

Thanks for the heads up! This patch can probably be deferred until
yours goes in.

>
> But seems like enough callers need a scratch buffer that the XDR
> layer should set up it transparently for all requests.

That could work too, and save some headache.

Anna

>
>
> > +
> >       status = decode_compound_hdr(xdr, &hdr);
> >       if (status)
> >               goto out;
> > diff --git a/fs/nfs/read.c b/fs/nfs/read.c
> > index eb854f1f86e2..012deb5a136f 100644
> > --- a/fs/nfs/read.c
> > +++ b/fs/nfs/read.c
> > @@ -37,15 +37,24 @@ static struct kmem_cache *nfs_rdata_cachep;
> >
> > static struct nfs_pgio_header *nfs_readhdr_alloc(void)
> > {
> > -     struct nfs_pgio_header *p = kmem_cache_zalloc(nfs_rdata_cachep, GFP_KERNEL);
> > +     struct nfs_pgio_header *p;
> > +     struct page *page;
> >
> > -     if (p)
> > +     page = alloc_page(GFP_KERNEL);
> > +     if (!page)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     p = kmem_cache_zalloc(nfs_rdata_cachep, GFP_KERNEL);
> > +     if (p) {
> >               p->rw_mode = FMODE_READ;
> > +             p->res.scratch = page;
> > +     }
> >       return p;
> > }
> >
> > static void nfs_readhdr_free(struct nfs_pgio_header *rhdr)
> > {
> > +     __free_page(rhdr->res.scratch);
> >       kmem_cache_free(nfs_rdata_cachep, rhdr);
> > }
> >
> > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> > index d63cb862d58e..e0a1c97f11a9 100644
> > --- a/include/linux/nfs_xdr.h
> > +++ b/include/linux/nfs_xdr.h
> > @@ -659,6 +659,7 @@ struct nfs_pgio_res {
> >       struct nfs_fattr *      fattr;
> >       __u64                   count;
> >       __u32                   op_status;
> > +     struct page *           scratch;
> >       union {
> >               struct {
> >                       unsigned int            replen;         /* used by read */
> > --
> > 2.29.2
> >
>
> --
> Chuck Lever
>
>
>
Trond Myklebust Dec. 3, 2020, 8:39 p.m. UTC | #3 
On Thu, 2020-12-03 at 15:31 -0500, Anna Schumaker wrote:
> On Thu, Dec 3, 2020 at 3:27 PM Chuck Lever <chuck.lever@oracle.com>
> wrote:
> > 
> > 
> > 
> > > On Dec 3, 2020, at 3:18 PM, schumaker.anna@gmail.com wrote:
> > > 
> > > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > > 
> > > We might need this to better handle shifting around data in the
> > > reply
> > > buffer.
> > > 
> > > Suggested-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > > ---
> > > fs/nfs/nfs42xdr.c       |  2 ++
> > > fs/nfs/read.c           | 13 +++++++++++--
> > > include/linux/nfs_xdr.h |  1 +
> > > 3 files changed, 14 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/nfs/nfs42xdr.c b/fs/nfs/nfs42xdr.c
> > > index 8432bd6b95f0..ef095a5f86f7 100644
> > > --- a/fs/nfs/nfs42xdr.c
> > > +++ b/fs/nfs/nfs42xdr.c
> > > @@ -1297,6 +1297,8 @@ static int nfs4_xdr_dec_read_plus(struct
> > > rpc_rqst *rqstp,
> > >       struct compound_hdr hdr;
> > >       int status;
> > > 
> > > +     xdr_set_scratch_buffer(xdr, page_address(res->scratch),
> > > PAGE_SIZE);
> > 
> > I intend to submit this for v5.11:
> > 
> > https://git.linux-nfs.org/?p=cel/cel-2.6.git;a=commit;h=0ae4c3e8a64ace1b8d7de033b0751afe43024416
> 
> Thanks for the heads up! This patch can probably be deferred until
> yours goes in.
> 
> > 
> > But seems like enough callers need a scratch buffer that the XDR
> > layer should set up it transparently for all requests.
> 
> That could work too, and save some headache.
> 

Why not just integrate it into xdr_init_decode_pages(), and then add a
matching xdr_exit_decode_pages() to free up any allocated page?
Chuck Lever Dec. 4, 2020, 4:14 p.m. UTC | #4 
> On Dec 3, 2020, at 3:39 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> On Thu, 2020-12-03 at 15:31 -0500, Anna Schumaker wrote:
>> On Thu, Dec 3, 2020 at 3:27 PM Chuck Lever <chuck.lever@oracle.com>
>> wrote:
>>> 
>>> 
>>> 
>>>> On Dec 3, 2020, at 3:18 PM, schumaker.anna@gmail.com wrote:
>>>> 
>>>> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
>>>> 
>>>> We might need this to better handle shifting around data in the
>>>> reply
>>>> buffer.
>>>> 
>>>> Suggested-by: Trond Myklebust <trond.myklebust@hammerspace.com>
>>>> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
>>>> ---
>>>> fs/nfs/nfs42xdr.c       |  2 ++
>>>> fs/nfs/read.c           | 13 +++++++++++--
>>>> include/linux/nfs_xdr.h |  1 +
>>>> 3 files changed, 14 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/fs/nfs/nfs42xdr.c b/fs/nfs/nfs42xdr.c
>>>> index 8432bd6b95f0..ef095a5f86f7 100644
>>>> --- a/fs/nfs/nfs42xdr.c
>>>> +++ b/fs/nfs/nfs42xdr.c
>>>> @@ -1297,6 +1297,8 @@ static int nfs4_xdr_dec_read_plus(struct
>>>> rpc_rqst *rqstp,
>>>>       struct compound_hdr hdr;
>>>>       int status;
>>>> 
>>>> +     xdr_set_scratch_buffer(xdr, page_address(res->scratch),
>>>> PAGE_SIZE);
>>> 
>>> I intend to submit this for v5.11:
>>> 
>>> https://git.linux-nfs.org/?p=cel/cel-2.6.git;a=commit;h=0ae4c3e8a64ace1b8d7de033b0751afe43024416
>> 
>> Thanks for the heads up! This patch can probably be deferred until
>> yours goes in.
>> 
>>> 
>>> But seems like enough callers need a scratch buffer that the XDR
>>> layer should set up it transparently for all requests.
>> 
>> That could work too, and save some headache.
>> 
> 
> Why not just integrate it into xdr_init_decode_pages(), and then add a
> matching xdr_exit_decode_pages() to free up any allocated page?

Sounds OK.

For comparison, to support xdr_stream decoding on the server, I've
changed svc_rqst_alloc() to grab a page that stays around until the
nfsd thread dies. There is a new svcxdr_init_decode() API that
attaches that page for use as the decoding scratch buffer.

Since it's a new API, the call sites that set up new streams with
xdr_init_decode() are not affected.

See:

https://git.linux-nfs.org/?p=cel/cel-2.6.git;a=commit;h=5191955d6fc65e6d4efe8f4f10a6028298f57281


--
Chuck Lever
diff mbox series

Patch

diff --git a/fs/nfs/nfs42xdr.c b/fs/nfs/nfs42xdr.c
index 8432bd6b95f0..ef095a5f86f7 100644
--- a/fs/nfs/nfs42xdr.c
+++ b/fs/nfs/nfs42xdr.c
@@ -1297,6 +1297,8 @@  static int nfs4_xdr_dec_read_plus(struct rpc_rqst *rqstp,
 	struct compound_hdr hdr;
 	int status;
 
+	xdr_set_scratch_buffer(xdr, page_address(res->scratch), PAGE_SIZE);
+
 	status = decode_compound_hdr(xdr, &hdr);
 	if (status)
 		goto out;
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index eb854f1f86e2..012deb5a136f 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -37,15 +37,24 @@  static struct kmem_cache *nfs_rdata_cachep;
 
 static struct nfs_pgio_header *nfs_readhdr_alloc(void)
 {
-	struct nfs_pgio_header *p = kmem_cache_zalloc(nfs_rdata_cachep, GFP_KERNEL);
+	struct nfs_pgio_header *p;
+	struct page *page;
 
-	if (p)
+	page = alloc_page(GFP_KERNEL);
+	if (!page)
+		return ERR_PTR(-ENOMEM);
+
+	p = kmem_cache_zalloc(nfs_rdata_cachep, GFP_KERNEL);
+	if (p) {
 		p->rw_mode = FMODE_READ;
+		p->res.scratch = page;
+	}
 	return p;
 }
 
 static void nfs_readhdr_free(struct nfs_pgio_header *rhdr)
 {
+	__free_page(rhdr->res.scratch);
 	kmem_cache_free(nfs_rdata_cachep, rhdr);
 }
 
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index d63cb862d58e..e0a1c97f11a9 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -659,6 +659,7 @@  struct nfs_pgio_res {
 	struct nfs_fattr *	fattr;
 	__u64			count;
 	__u32			op_status;
+	struct page *		scratch;
 	union {
 		struct {
 			unsigned int		replen;		/* used by read */