[PATCH 0/3] NFS: Disable READ_PLUS by default

[PATCH 0/3] NFS: Disable READ_PLUS by default

[schumaker.anna]

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

I've been scratching my head about what's going on with xfstests generic/091
and generic/263, but I'm not sure what else to look at at this point.
This patchset disables READ_PLUS by default by marking it as a
developer-only kconfig option.

I also included a couple of patches fixing some other issues that were
noticed while inspecting the code. These patches don't help the tests
pass, but they do fail later on after applying so it does feel like
progress.

I'm hopeful the remaning issues can be worked out in the future.

Thanks,
Anna


Anna Schumaker (3):
  NFS: Disable READ_PLUS by default
  NFS: Allocate a scratch page for READ_PLUS
  SUNRPC: Keep buf->len in sync with xdr->nwords when expanding holes

 fs/nfs/Kconfig          |  9 +++++++++
 fs/nfs/nfs42xdr.c       |  2 ++
 fs/nfs/nfs4proc.c       |  2 +-
 fs/nfs/read.c           | 13 +++++++++++--
 include/linux/nfs_xdr.h |  1 +
 net/sunrpc/xdr.c        |  3 ++-
 6 files changed, 26 insertions(+), 4 deletions(-)

-- 
2.29.2

[PATCH 1/3] NFS: Disable READ_PLUS by default

[schumaker.anna]

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

We've been seeing failures with xfstests generic/091 and generic/263
when using READ_PLUS. I've made some progress on these issues, and the
tests fail later on but still don't pass. Let's disable READ_PLUS by
default until we can work out what is going on.

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
 fs/nfs/Kconfig    | 9 +++++++++
 fs/nfs/nfs4proc.c | 2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/Kconfig b/fs/nfs/Kconfig
index 88e1763e02f3..e2a488d403a6 100644
--- a/fs/nfs/Kconfig
+++ b/fs/nfs/Kconfig
@@ -205,3 +205,12 @@ config NFS_DISABLE_UDP_SUPPORT
 	 Choose Y here to disable the use of NFS over UDP. NFS over UDP
 	 on modern networks (1Gb+) can lead to data corruption caused by
 	 fragmentation during high loads.
+
+config NFS_V4_2_READ_PLUS
+	bool "NFS: Enable support for the NFSv4.2 READ_PLUS operation"
+	depends on NFS_V4_2
+	default n
+	help
+	 This is intended for developers only. The READ_PLUS operation has
+	 been shown to have issues under specific conditions and should not
+	 be used in production.
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 9e0ca9b2b210..e89468678ae1 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5309,7 +5309,7 @@ static int nfs4_read_done(struct rpc_task *task, struct nfs_pgio_header *hdr)
 				    nfs4_read_done_cb(task, hdr);
 }
 
-#ifdef CONFIG_NFS_V4_2
+#if defined CONFIG_NFS_V4_2 && defined CONFIG_NFS_V4_2_READ_PLUS
 static void nfs42_read_plus_support(struct nfs_server *server, struct rpc_message *msg)
 {
 	if (server->caps & NFS_CAP_READ_PLUS)
-- 
2.29.2

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

[schumaker.anna]

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);
+
 	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

[PATCH 3/3] SUNRPC: Keep buf->len in sync with xdr->nwords when expanding holes

[schumaker.anna]

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

Otherwise we could end up placing data a few bytes off from where we
actually want to put it.

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
 net/sunrpc/xdr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 71e03b930b70..5b848fe65c81 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1314,6 +1314,7 @@ uint64_t xdr_expand_hole(struct xdr_stream *xdr, uint64_t offset, uint64_t lengt
 		unsigned int res = _shift_data_right_tail(buf, from + bytes - shift, shift);
 		truncated = shift - res;
 		xdr->nwords -= XDR_QUADLEN(truncated);
+		buf->len -= 4 * XDR_QUADLEN(truncated);
 		bytes -= shift;
 	}
 
@@ -1325,7 +1326,7 @@ uint64_t xdr_expand_hole(struct xdr_stream *xdr, uint64_t offset, uint64_t lengt
 					bytes);
 	_zero_pages(buf->pages, buf->page_base + offset, length);
 
-	buf->len += length - (from - offset) - truncated;
+	buf->len += length - (from - offset);
 	xdr_set_page(xdr, offset + length, PAGE_SIZE);
 	return length;
 }
-- 
2.29.2

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

[Chuck Lever]

> 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

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

[Anna Schumaker]

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
>
>
>

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

[Trond Myklebust]

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?

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

[Mkrtchyan, Tigran]

Hi Anna,

I see problems with gedeviceinfo and bisected it to c567552612ece787b178e3b147b5854ad422a836.
The commit itself doesn't look that can break it, but might
be can help you find the problem.

What I see, that after xdr_read_pages call the xdr stream points
to a some random point (or wrong page)

Regards,
   Tigran.


----- Original Message -----
> From: "schumaker anna" <schumaker.anna@gmail.com>
> To: "linux-nfs" <linux-nfs@vger.kernel.org>
> Cc: "Anna Schumaker" <Anna.Schumaker@Netapp.com>
> Sent: Thursday, 3 December, 2020 21:18:38
> Subject: [PATCH 0/3] NFS: Disable READ_PLUS by default

> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> 
> I've been scratching my head about what's going on with xfstests generic/091
> and generic/263, but I'm not sure what else to look at at this point.
> This patchset disables READ_PLUS by default by marking it as a
> developer-only kconfig option.
> 
> I also included a couple of patches fixing some other issues that were
> noticed while inspecting the code. These patches don't help the tests
> pass, but they do fail later on after applying so it does feel like
> progress.
> 
> I'm hopeful the remaning issues can be worked out in the future.
> 
> Thanks,
> Anna
> 
> 
> Anna Schumaker (3):
>  NFS: Disable READ_PLUS by default
>  NFS: Allocate a scratch page for READ_PLUS
>  SUNRPC: Keep buf->len in sync with xdr->nwords when expanding holes
> 
> fs/nfs/Kconfig          |  9 +++++++++
> fs/nfs/nfs42xdr.c       |  2 ++
> fs/nfs/nfs4proc.c       |  2 +-
> fs/nfs/read.c           | 13 +++++++++++--
> include/linux/nfs_xdr.h |  1 +
> net/sunrpc/xdr.c        |  3 ++-
> 6 files changed, 26 insertions(+), 4 deletions(-)
> 
> --
> 2.29.2

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

[Chuck Lever]

> 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

Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

[Olga Kornievskaia]

I object to putting the disable patch in, I think we need to fix the problem.

The current problem is generic/263 is failing because the end of the
buffer is corrupted since we lost the bytes when hole was expanded. I
don't know what the solution is: (1) hole expanding handling needs to
be fixed to not lose data or (2) we shouldnt be reporting that we read
the bytes we lost.

On Fri, Dec 4, 2020 at 10:49 AM Mkrtchyan, Tigran
<tigran.mkrtchyan@desy.de> wrote:
>
> Hi Anna,
>
> I see problems with gedeviceinfo and bisected it to c567552612ece787b178e3b147b5854ad422a836.
> The commit itself doesn't look that can break it, but might
> be can help you find the problem.
>
> What I see, that after xdr_read_pages call the xdr stream points
> to a some random point (or wrong page)
>
> Regards,
>    Tigran.
>
>
> ----- Original Message -----
> > From: "schumaker anna" <schumaker.anna@gmail.com>
> > To: "linux-nfs" <linux-nfs@vger.kernel.org>
> > Cc: "Anna Schumaker" <Anna.Schumaker@Netapp.com>
> > Sent: Thursday, 3 December, 2020 21:18:38
> > Subject: [PATCH 0/3] NFS: Disable READ_PLUS by default
>
> > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> >
> > I've been scratching my head about what's going on with xfstests generic/091
> > and generic/263, but I'm not sure what else to look at at this point.
> > This patchset disables READ_PLUS by default by marking it as a
> > developer-only kconfig option.
> >
> > I also included a couple of patches fixing some other issues that were
> > noticed while inspecting the code. These patches don't help the tests
> > pass, but they do fail later on after applying so it does feel like
> > progress.
> >
> > I'm hopeful the remaning issues can be worked out in the future.
> >
> > Thanks,
> > Anna
> >
> >
> > Anna Schumaker (3):
> >  NFS: Disable READ_PLUS by default
> >  NFS: Allocate a scratch page for READ_PLUS
> >  SUNRPC: Keep buf->len in sync with xdr->nwords when expanding holes
> >
> > fs/nfs/Kconfig          |  9 +++++++++
> > fs/nfs/nfs42xdr.c       |  2 ++
> > fs/nfs/nfs4proc.c       |  2 +-
> > fs/nfs/read.c           | 13 +++++++++++--
> > include/linux/nfs_xdr.h |  1 +
> > net/sunrpc/xdr.c        |  3 ++-
> > 6 files changed, 26 insertions(+), 4 deletions(-)
> >
> > --
> > 2.29.2

Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

[Mkrtchyan, Tigran]

I agree with Olga, especially that disabling doesn't fixes my issue.
Unfortunately I have no idea how kernel's vm works, but I am
ready to test as many times as needed.

Thanks,
   Tigran.

----- Original Message -----
> From: "Olga Kornievskaia" <aglo@umich.edu>
> To: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
> Cc: "schumaker anna" <schumaker.anna@gmail.com>, "linux-nfs" <linux-nfs@vger.kernel.org>, "Anna Schumaker"
> <Anna.Schumaker@netapp.com>
> Sent: Friday, 4 December, 2020 21:00:32
> Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

> I object to putting the disable patch in, I think we need to fix the problem.
> 
> The current problem is generic/263 is failing because the end of the
> buffer is corrupted since we lost the bytes when hole was expanded. I
> don't know what the solution is: (1) hole expanding handling needs to
> be fixed to not lose data or (2) we shouldnt be reporting that we read
> the bytes we lost.
> 
> On Fri, Dec 4, 2020 at 10:49 AM Mkrtchyan, Tigran
> <tigran.mkrtchyan@desy.de> wrote:
>>
>> Hi Anna,
>>
>> I see problems with gedeviceinfo and bisected it to
>> c567552612ece787b178e3b147b5854ad422a836.
>> The commit itself doesn't look that can break it, but might
>> be can help you find the problem.
>>
>> What I see, that after xdr_read_pages call the xdr stream points
>> to a some random point (or wrong page)
>>
>> Regards,
>>    Tigran.
>>
>>
>> ----- Original Message -----
>> > From: "schumaker anna" <schumaker.anna@gmail.com>
>> > To: "linux-nfs" <linux-nfs@vger.kernel.org>
>> > Cc: "Anna Schumaker" <Anna.Schumaker@Netapp.com>
>> > Sent: Thursday, 3 December, 2020 21:18:38
>> > Subject: [PATCH 0/3] NFS: Disable READ_PLUS by default
>>
>> > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
>> >
>> > I've been scratching my head about what's going on with xfstests generic/091
>> > and generic/263, but I'm not sure what else to look at at this point.
>> > This patchset disables READ_PLUS by default by marking it as a
>> > developer-only kconfig option.
>> >
>> > I also included a couple of patches fixing some other issues that were
>> > noticed while inspecting the code. These patches don't help the tests
>> > pass, but they do fail later on after applying so it does feel like
>> > progress.
>> >
>> > I'm hopeful the remaning issues can be worked out in the future.
>> >
>> > Thanks,
>> > Anna
>> >
>> >
>> > Anna Schumaker (3):
>> >  NFS: Disable READ_PLUS by default
>> >  NFS: Allocate a scratch page for READ_PLUS
>> >  SUNRPC: Keep buf->len in sync with xdr->nwords when expanding holes
>> >
>> > fs/nfs/Kconfig          |  9 +++++++++
>> > fs/nfs/nfs42xdr.c       |  2 ++
>> > fs/nfs/nfs4proc.c       |  2 +-
>> > fs/nfs/read.c           | 13 +++++++++++--
>> > include/linux/nfs_xdr.h |  1 +
>> > net/sunrpc/xdr.c        |  3 ++-
>> > 6 files changed, 26 insertions(+), 4 deletions(-)
>> >
>> > --
> > > 2.29.2

Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

[Mkrtchyan, Tigran]

Hi Anna

I re-run bisect on your tree with tag nfs-for-5.10-1 as bad and 5.9.0 as good.

The bisect point to commit a14a63594cc2e5bdcbb1543d29df945da71e380f, which makes more sense.

[centos@os-46-nfs-devel linux-nfs]$ git bisect good
c567552612ece787b178e3b147b5854ad422a836 is the first bad commit
commit c567552612ece787b178e3b147b5854ad422a836
Author: Anna Schumaker <Anna.Schumaker@Netapp.com>
Date:   Wed May 28 13:41:22 2014 -0400

    NFS: Add READ_PLUS data segment support

    This patch adds client support for decoding a single NFS4_CONTENT_DATA
    segment returned by the server. This is the simplest implementation
    possible, since it does not account for any hole segments in the reply.

    Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>

 fs/nfs/nfs42xdr.c         | 141 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/nfs/nfs4client.c       |   2 +
 fs/nfs/nfs4proc.c         |  43 +++++++++++++-
 fs/nfs/nfs4xdr.c          |   1 +
 include/linux/nfs4.h      |   2 +-
 include/linux/nfs_fs_sb.h |   1 +
 include/linux/nfs_xdr.h   |   2 +-
 7 files changed, 187 insertions(+), 5 deletions(-)


Best regards,
   Tigran

----- Original Message -----
> From: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
> To: "Olga Kornievskaia" <aglo@umich.edu>
> Cc: "schumaker anna" <schumaker.anna@gmail.com>, "linux-nfs" <linux-nfs@vger.kernel.org>, "Anna Schumaker"
> <Anna.Schumaker@netapp.com>
> Sent: Friday, 4 December, 2020 23:50:27
> Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

> I agree with Olga, especially that disabling doesn't fixes my issue.
> Unfortunately I have no idea how kernel's vm works, but I am
> ready to test as many times as needed.
> 
> Thanks,
>   Tigran.
> 
> ----- Original Message -----
>> From: "Olga Kornievskaia" <aglo@umich.edu>
>> To: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
>> Cc: "schumaker anna" <schumaker.anna@gmail.com>, "linux-nfs"
>> <linux-nfs@vger.kernel.org>, "Anna Schumaker"
>> <Anna.Schumaker@netapp.com>
>> Sent: Friday, 4 December, 2020 21:00:32
>> Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default
> 
>> I object to putting the disable patch in, I think we need to fix the problem.
>> 
>> The current problem is generic/263 is failing because the end of the
>> buffer is corrupted since we lost the bytes when hole was expanded. I
>> don't know what the solution is: (1) hole expanding handling needs to
>> be fixed to not lose data or (2) we shouldnt be reporting that we read
>> the bytes we lost.
>> 
>> On Fri, Dec 4, 2020 at 10:49 AM Mkrtchyan, Tigran
>> <tigran.mkrtchyan@desy.de> wrote:
>>>
>>> Hi Anna,
>>>
>>> I see problems with gedeviceinfo and bisected it to
>>> c567552612ece787b178e3b147b5854ad422a836.
>>> The commit itself doesn't look that can break it, but might
>>> be can help you find the problem.
>>>
>>> What I see, that after xdr_read_pages call the xdr stream points
>>> to a some random point (or wrong page)
>>>
>>> Regards,
>>>    Tigran.
>>>
>>>
>>> ----- Original Message -----
>>> > From: "schumaker anna" <schumaker.anna@gmail.com>
>>> > To: "linux-nfs" <linux-nfs@vger.kernel.org>
>>> > Cc: "Anna Schumaker" <Anna.Schumaker@Netapp.com>
>>> > Sent: Thursday, 3 December, 2020 21:18:38
>>> > Subject: [PATCH 0/3] NFS: Disable READ_PLUS by default
>>>
>>> > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
>>> >
>>> > I've been scratching my head about what's going on with xfstests generic/091
>>> > and generic/263, but I'm not sure what else to look at at this point.
>>> > This patchset disables READ_PLUS by default by marking it as a
>>> > developer-only kconfig option.
>>> >
>>> > I also included a couple of patches fixing some other issues that were
>>> > noticed while inspecting the code. These patches don't help the tests
>>> > pass, but they do fail later on after applying so it does feel like
>>> > progress.
>>> >
>>> > I'm hopeful the remaning issues can be worked out in the future.
>>> >
>>> > Thanks,
>>> > Anna
>>> >
>>> >
>>> > Anna Schumaker (3):
>>> >  NFS: Disable READ_PLUS by default
>>> >  NFS: Allocate a scratch page for READ_PLUS
>>> >  SUNRPC: Keep buf->len in sync with xdr->nwords when expanding holes
>>> >
>>> > fs/nfs/Kconfig          |  9 +++++++++
>>> > fs/nfs/nfs42xdr.c       |  2 ++
>>> > fs/nfs/nfs4proc.c       |  2 +-
>>> > fs/nfs/read.c           | 13 +++++++++++--
>>> > include/linux/nfs_xdr.h |  1 +
>>> > net/sunrpc/xdr.c        |  3 ++-
>>> > 6 files changed, 26 insertions(+), 4 deletions(-)
>>> >
>>> > --
> > > > 2.29.2

Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

[Mkrtchyan, Tigran]

Sorry, completely confused. It the same commit as before c567552612ece787b178e3b147b5854ad422a836

Tigran.


----- Original Message -----
> From: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
> To: "Anna Schumaker" <Anna.Schumaker@netapp.com>
> Cc: "schumaker anna" <schumaker.anna@gmail.com>, "linux-nfs" <linux-nfs@vger.kernel.org>, "Olga Kornievskaia"
> <aglo@umich.edu>
> Sent: Monday, 7 December, 2020 16:26:05
> Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

> Hi Anna
> 
> I re-run bisect on your tree with tag nfs-for-5.10-1 as bad and 5.9.0 as good.
> 
> The bisect point to commit a14a63594cc2e5bdcbb1543d29df945da71e380f, which makes
> more sense.
> 
> [centos@os-46-nfs-devel linux-nfs]$ git bisect good
> c567552612ece787b178e3b147b5854ad422a836 is the first bad commit
> commit c567552612ece787b178e3b147b5854ad422a836
> Author: Anna Schumaker <Anna.Schumaker@Netapp.com>
> Date:   Wed May 28 13:41:22 2014 -0400
> 
>    NFS: Add READ_PLUS data segment support
> 
>    This patch adds client support for decoding a single NFS4_CONTENT_DATA
>    segment returned by the server. This is the simplest implementation
>    possible, since it does not account for any hole segments in the reply.
> 
>    Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> 
> fs/nfs/nfs42xdr.c         | 141 ++++++++++++++++++++++++++++++++++++++++++++++
> fs/nfs/nfs4client.c       |   2 +
> fs/nfs/nfs4proc.c         |  43 +++++++++++++-
> fs/nfs/nfs4xdr.c          |   1 +
> include/linux/nfs4.h      |   2 +-
> include/linux/nfs_fs_sb.h |   1 +
> include/linux/nfs_xdr.h   |   2 +-
> 7 files changed, 187 insertions(+), 5 deletions(-)
> 
> 
> Best regards,
>   Tigran
> 
> ----- Original Message -----
>> From: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
>> To: "Olga Kornievskaia" <aglo@umich.edu>
>> Cc: "schumaker anna" <schumaker.anna@gmail.com>, "linux-nfs"
>> <linux-nfs@vger.kernel.org>, "Anna Schumaker"
>> <Anna.Schumaker@netapp.com>
>> Sent: Friday, 4 December, 2020 23:50:27
>> Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default
> 
>> I agree with Olga, especially that disabling doesn't fixes my issue.
>> Unfortunately I have no idea how kernel's vm works, but I am
>> ready to test as many times as needed.
>> 
>> Thanks,
>>   Tigran.
>> 
>> ----- Original Message -----
>>> From: "Olga Kornievskaia" <aglo@umich.edu>
>>> To: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
>>> Cc: "schumaker anna" <schumaker.anna@gmail.com>, "linux-nfs"
>>> <linux-nfs@vger.kernel.org>, "Anna Schumaker"
>>> <Anna.Schumaker@netapp.com>
>>> Sent: Friday, 4 December, 2020 21:00:32
>>> Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default
>> 
>>> I object to putting the disable patch in, I think we need to fix the problem.
>>> 
>>> The current problem is generic/263 is failing because the end of the
>>> buffer is corrupted since we lost the bytes when hole was expanded. I
>>> don't know what the solution is: (1) hole expanding handling needs to
>>> be fixed to not lose data or (2) we shouldnt be reporting that we read
>>> the bytes we lost.
>>> 
>>> On Fri, Dec 4, 2020 at 10:49 AM Mkrtchyan, Tigran
>>> <tigran.mkrtchyan@desy.de> wrote:
>>>>
>>>> Hi Anna,
>>>>
>>>> I see problems with gedeviceinfo and bisected it to
>>>> c567552612ece787b178e3b147b5854ad422a836.
>>>> The commit itself doesn't look that can break it, but might
>>>> be can help you find the problem.
>>>>
>>>> What I see, that after xdr_read_pages call the xdr stream points
>>>> to a some random point (or wrong page)
>>>>
>>>> Regards,
>>>>    Tigran.
>>>>
>>>>
>>>> ----- Original Message -----
>>>> > From: "schumaker anna" <schumaker.anna@gmail.com>
>>>> > To: "linux-nfs" <linux-nfs@vger.kernel.org>
>>>> > Cc: "Anna Schumaker" <Anna.Schumaker@Netapp.com>
>>>> > Sent: Thursday, 3 December, 2020 21:18:38
>>>> > Subject: [PATCH 0/3] NFS: Disable READ_PLUS by default
>>>>
>>>> > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
>>>> >
>>>> > I've been scratching my head about what's going on with xfstests generic/091
>>>> > and generic/263, but I'm not sure what else to look at at this point.
>>>> > This patchset disables READ_PLUS by default by marking it as a
>>>> > developer-only kconfig option.
>>>> >
>>>> > I also included a couple of patches fixing some other issues that were
>>>> > noticed while inspecting the code. These patches don't help the tests
>>>> > pass, but they do fail later on after applying so it does feel like
>>>> > progress.
>>>> >
>>>> > I'm hopeful the remaning issues can be worked out in the future.
>>>> >
>>>> > Thanks,
>>>> > Anna
>>>> >
>>>> >
>>>> > Anna Schumaker (3):
>>>> >  NFS: Disable READ_PLUS by default
>>>> >  NFS: Allocate a scratch page for READ_PLUS
>>>> >  SUNRPC: Keep buf->len in sync with xdr->nwords when expanding holes
>>>> >
>>>> > fs/nfs/Kconfig          |  9 +++++++++
>>>> > fs/nfs/nfs42xdr.c       |  2 ++
>>>> > fs/nfs/nfs4proc.c       |  2 +-
>>>> > fs/nfs/read.c           | 13 +++++++++++--
>>>> > include/linux/nfs_xdr.h |  1 +
>>>> > net/sunrpc/xdr.c        |  3 ++-
>>>> > 6 files changed, 26 insertions(+), 4 deletions(-)
>>>> >
>>>> > --
> > > > > 2.29.2

Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

[Mkrtchyan, Tigran]

Hi Anna et al.,

I run some more test and found two issues:

1. our server returns on getdeviceinfo notification bitmap of size 2.
   Though, only the first element has non zero values, some how this
   makes client unhappy, probably due to definition on decode_getdeviceinfo_maxsz
   that expects bitmap length to be 1.

2. The client uses READ_PLUS to DS despite the fact that flex file
   layout stated that DS supports nfs v4.1

Network File System, Ops(3): SEQUENCE, PUTFH, READ_PLUS
    [Program Version: 4]
    [V4 Procedure: COMPOUND (1)]
    Tag: <EMPTY>
    minorversion: 1
    Operations (count: 3): SEQUENCE, PUTFH, READ_PLUS
        Opcode: SEQUENCE (53)
        Opcode: PUTFH (22)
        Opcode: READ_PLUS (68)
            StateID
            offset: 0
            count: 10016
    [Main Opcode: READ_PLUS (68)]


I should re-run some tests with Trond's tree as I was checking the
DS errors in during the test I run last weeks.

Regards,
   Tigran.

----- Original Message -----
> From: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
> To: "Anna Schumaker" <Anna.Schumaker@netapp.com>
> Cc: "schumaker anna" <schumaker.anna@gmail.com>, "linux-nfs" <linux-nfs@vger.kernel.org>, "Olga Kornievskaia"
> <aglo@umich.edu>
> Sent: Monday, 7 December, 2020 16:54:37
> Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

> Sorry, completely confused. It the same commit as before
> c567552612ece787b178e3b147b5854ad422a836
> 
> Tigran.
> 
> 
> ----- Original Message -----
>> From: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
>> To: "Anna Schumaker" <Anna.Schumaker@netapp.com>
>> Cc: "schumaker anna" <schumaker.anna@gmail.com>, "linux-nfs"
>> <linux-nfs@vger.kernel.org>, "Olga Kornievskaia"
>> <aglo@umich.edu>
>> Sent: Monday, 7 December, 2020 16:26:05
>> Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default
> 
>> Hi Anna
>> 
>> I re-run bisect on your tree with tag nfs-for-5.10-1 as bad and 5.9.0 as good.
>> 
>> The bisect point to commit a14a63594cc2e5bdcbb1543d29df945da71e380f, which makes
>> more sense.
>> 
>> [centos@os-46-nfs-devel linux-nfs]$ git bisect good
>> c567552612ece787b178e3b147b5854ad422a836 is the first bad commit
>> commit c567552612ece787b178e3b147b5854ad422a836
>> Author: Anna Schumaker <Anna.Schumaker@Netapp.com>
>> Date:   Wed May 28 13:41:22 2014 -0400
>> 
>>    NFS: Add READ_PLUS data segment support
>> 
>>    This patch adds client support for decoding a single NFS4_CONTENT_DATA
>>    segment returned by the server. This is the simplest implementation
>>    possible, since it does not account for any hole segments in the reply.
>> 
>>    Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
>> 
>> fs/nfs/nfs42xdr.c         | 141 ++++++++++++++++++++++++++++++++++++++++++++++
>> fs/nfs/nfs4client.c       |   2 +
>> fs/nfs/nfs4proc.c         |  43 +++++++++++++-
>> fs/nfs/nfs4xdr.c          |   1 +
>> include/linux/nfs4.h      |   2 +-
>> include/linux/nfs_fs_sb.h |   1 +
>> include/linux/nfs_xdr.h   |   2 +-
>> 7 files changed, 187 insertions(+), 5 deletions(-)
>> 
>> 
>> Best regards,
>>   Tigran
>> 
>> ----- Original Message -----
>>> From: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
>>> To: "Olga Kornievskaia" <aglo@umich.edu>
>>> Cc: "schumaker anna" <schumaker.anna@gmail.com>, "linux-nfs"
>>> <linux-nfs@vger.kernel.org>, "Anna Schumaker"
>>> <Anna.Schumaker@netapp.com>
>>> Sent: Friday, 4 December, 2020 23:50:27
>>> Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default
>> 
>>> I agree with Olga, especially that disabling doesn't fixes my issue.
>>> Unfortunately I have no idea how kernel's vm works, but I am
>>> ready to test as many times as needed.
>>> 
>>> Thanks,
>>>   Tigran.
>>> 
>>> ----- Original Message -----
>>>> From: "Olga Kornievskaia" <aglo@umich.edu>
>>>> To: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
>>>> Cc: "schumaker anna" <schumaker.anna@gmail.com>, "linux-nfs"
>>>> <linux-nfs@vger.kernel.org>, "Anna Schumaker"
>>>> <Anna.Schumaker@netapp.com>
>>>> Sent: Friday, 4 December, 2020 21:00:32
>>>> Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default
>>> 
>>>> I object to putting the disable patch in, I think we need to fix the problem.
>>>> 
>>>> The current problem is generic/263 is failing because the end of the
>>>> buffer is corrupted since we lost the bytes when hole was expanded. I
>>>> don't know what the solution is: (1) hole expanding handling needs to
>>>> be fixed to not lose data or (2) we shouldnt be reporting that we read
>>>> the bytes we lost.
>>>> 
>>>> On Fri, Dec 4, 2020 at 10:49 AM Mkrtchyan, Tigran
>>>> <tigran.mkrtchyan@desy.de> wrote:
>>>>>
>>>>> Hi Anna,
>>>>>
>>>>> I see problems with gedeviceinfo and bisected it to
>>>>> c567552612ece787b178e3b147b5854ad422a836.
>>>>> The commit itself doesn't look that can break it, but might
>>>>> be can help you find the problem.
>>>>>
>>>>> What I see, that after xdr_read_pages call the xdr stream points
>>>>> to a some random point (or wrong page)
>>>>>
>>>>> Regards,
>>>>>    Tigran.
>>>>>
>>>>>
>>>>> ----- Original Message -----
>>>>> > From: "schumaker anna" <schumaker.anna@gmail.com>
>>>>> > To: "linux-nfs" <linux-nfs@vger.kernel.org>
>>>>> > Cc: "Anna Schumaker" <Anna.Schumaker@Netapp.com>
>>>>> > Sent: Thursday, 3 December, 2020 21:18:38
>>>>> > Subject: [PATCH 0/3] NFS: Disable READ_PLUS by default
>>>>>
>>>>> > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
>>>>> >
>>>>> > I've been scratching my head about what's going on with xfstests generic/091
>>>>> > and generic/263, but I'm not sure what else to look at at this point.
>>>>> > This patchset disables READ_PLUS by default by marking it as a
>>>>> > developer-only kconfig option.
>>>>> >
>>>>> > I also included a couple of patches fixing some other issues that were
>>>>> > noticed while inspecting the code. These patches don't help the tests
>>>>> > pass, but they do fail later on after applying so it does feel like
>>>>> > progress.
>>>>> >
>>>>> > I'm hopeful the remaning issues can be worked out in the future.
>>>>> >
>>>>> > Thanks,
>>>>> > Anna
>>>>> >
>>>>> >
>>>>> > Anna Schumaker (3):
>>>>> >  NFS: Disable READ_PLUS by default
>>>>> >  NFS: Allocate a scratch page for READ_PLUS
>>>>> >  SUNRPC: Keep buf->len in sync with xdr->nwords when expanding holes
>>>>> >
>>>>> > fs/nfs/Kconfig          |  9 +++++++++
>>>>> > fs/nfs/nfs42xdr.c       |  2 ++
>>>>> > fs/nfs/nfs4proc.c       |  2 +-
>>>>> > fs/nfs/read.c           | 13 +++++++++++--
>>>>> > include/linux/nfs_xdr.h |  1 +
>>>>> > net/sunrpc/xdr.c        |  3 ++-
>>>>> > 6 files changed, 26 insertions(+), 4 deletions(-)
>>>>> >
>>>>> > --
> > > > > > 2.29.2

Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

[Anna Schumaker]

Hi Tigran,

On Tue, Dec 8, 2020 at 7:39 AM Mkrtchyan, Tigran
<tigran.mkrtchyan@desy.de> wrote:
>
>
>
> Hi Anna et al.,
>
> I run some more test and found two issues:
>
> 1. our server returns on getdeviceinfo notification bitmap of size 2.
>    Though, only the first element has non zero values, some how this
>    makes client unhappy, probably due to definition on decode_getdeviceinfo_maxsz
>    that expects bitmap length to be 1.
>
> 2. The client uses READ_PLUS to DS despite the fact that flex file
>    layout stated that DS supports nfs v4.1
>
> Network File System, Ops(3): SEQUENCE, PUTFH, READ_PLUS
>     [Program Version: 4]
>     [V4 Procedure: COMPOUND (1)]
>     Tag: <EMPTY>
>     minorversion: 1
>     Operations (count: 3): SEQUENCE, PUTFH, READ_PLUS
>         Opcode: SEQUENCE (53)
>         Opcode: PUTFH (22)
>         Opcode: READ_PLUS (68)
>             StateID
>             offset: 0
>             count: 10016
>     [Main Opcode: READ_PLUS (68)]

Thanks for the update! You're right, READ_PLUS to the DS should not be
happening. I'll look into why it is.

Anna

>
>
> I should re-run some tests with Trond's tree as I was checking the
> DS errors in during the test I run last weeks.
>
> Regards,
>    Tigran.
>
> ----- Original Message -----
> > From: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
> > To: "Anna Schumaker" <Anna.Schumaker@netapp.com>
> > Cc: "schumaker anna" <schumaker.anna@gmail.com>, "linux-nfs" <linux-nfs@vger.kernel.org>, "Olga Kornievskaia"
> > <aglo@umich.edu>
> > Sent: Monday, 7 December, 2020 16:54:37
> > Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default
>
> > Sorry, completely confused. It the same commit as before
> > c567552612ece787b178e3b147b5854ad422a836
> >
> > Tigran.
> >
> >
> > ----- Original Message -----
> >> From: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
> >> To: "Anna Schumaker" <Anna.Schumaker@netapp.com>
> >> Cc: "schumaker anna" <schumaker.anna@gmail.com>, "linux-nfs"
> >> <linux-nfs@vger.kernel.org>, "Olga Kornievskaia"
> >> <aglo@umich.edu>
> >> Sent: Monday, 7 December, 2020 16:26:05
> >> Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default
> >
> >> Hi Anna
> >>
> >> I re-run bisect on your tree with tag nfs-for-5.10-1 as bad and 5.9.0 as good.
> >>
> >> The bisect point to commit a14a63594cc2e5bdcbb1543d29df945da71e380f, which makes
> >> more sense.
> >>
> >> [centos@os-46-nfs-devel linux-nfs]$ git bisect good
> >> c567552612ece787b178e3b147b5854ad422a836 is the first bad commit
> >> commit c567552612ece787b178e3b147b5854ad422a836
> >> Author: Anna Schumaker <Anna.Schumaker@Netapp.com>
> >> Date:   Wed May 28 13:41:22 2014 -0400
> >>
> >>    NFS: Add READ_PLUS data segment support
> >>
> >>    This patch adds client support for decoding a single NFS4_CONTENT_DATA
> >>    segment returned by the server. This is the simplest implementation
> >>    possible, since it does not account for any hole segments in the reply.
> >>
> >>    Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> >>
> >> fs/nfs/nfs42xdr.c         | 141 ++++++++++++++++++++++++++++++++++++++++++++++
> >> fs/nfs/nfs4client.c       |   2 +
> >> fs/nfs/nfs4proc.c         |  43 +++++++++++++-
> >> fs/nfs/nfs4xdr.c          |   1 +
> >> include/linux/nfs4.h      |   2 +-
> >> include/linux/nfs_fs_sb.h |   1 +
> >> include/linux/nfs_xdr.h   |   2 +-
> >> 7 files changed, 187 insertions(+), 5 deletions(-)
> >>
> >>
> >> Best regards,
> >>   Tigran
> >>
> >> ----- Original Message -----
> >>> From: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
> >>> To: "Olga Kornievskaia" <aglo@umich.edu>
> >>> Cc: "schumaker anna" <schumaker.anna@gmail.com>, "linux-nfs"
> >>> <linux-nfs@vger.kernel.org>, "Anna Schumaker"
> >>> <Anna.Schumaker@netapp.com>
> >>> Sent: Friday, 4 December, 2020 23:50:27
> >>> Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default
> >>
> >>> I agree with Olga, especially that disabling doesn't fixes my issue.
> >>> Unfortunately I have no idea how kernel's vm works, but I am
> >>> ready to test as many times as needed.
> >>>
> >>> Thanks,
> >>>   Tigran.
> >>>
> >>> ----- Original Message -----
> >>>> From: "Olga Kornievskaia" <aglo@umich.edu>
> >>>> To: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
> >>>> Cc: "schumaker anna" <schumaker.anna@gmail.com>, "linux-nfs"
> >>>> <linux-nfs@vger.kernel.org>, "Anna Schumaker"
> >>>> <Anna.Schumaker@netapp.com>
> >>>> Sent: Friday, 4 December, 2020 21:00:32
> >>>> Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default
> >>>
> >>>> I object to putting the disable patch in, I think we need to fix the problem.
> >>>>
> >>>> The current problem is generic/263 is failing because the end of the
> >>>> buffer is corrupted since we lost the bytes when hole was expanded. I
> >>>> don't know what the solution is: (1) hole expanding handling needs to
> >>>> be fixed to not lose data or (2) we shouldnt be reporting that we read
> >>>> the bytes we lost.
> >>>>
> >>>> On Fri, Dec 4, 2020 at 10:49 AM Mkrtchyan, Tigran
> >>>> <tigran.mkrtchyan@desy.de> wrote:
> >>>>>
> >>>>> Hi Anna,
> >>>>>
> >>>>> I see problems with gedeviceinfo and bisected it to
> >>>>> c567552612ece787b178e3b147b5854ad422a836.
> >>>>> The commit itself doesn't look that can break it, but might
> >>>>> be can help you find the problem.
> >>>>>
> >>>>> What I see, that after xdr_read_pages call the xdr stream points
> >>>>> to a some random point (or wrong page)
> >>>>>
> >>>>> Regards,
> >>>>>    Tigran.
> >>>>>
> >>>>>
> >>>>> ----- Original Message -----
> >>>>> > From: "schumaker anna" <schumaker.anna@gmail.com>
> >>>>> > To: "linux-nfs" <linux-nfs@vger.kernel.org>
> >>>>> > Cc: "Anna Schumaker" <Anna.Schumaker@Netapp.com>
> >>>>> > Sent: Thursday, 3 December, 2020 21:18:38
> >>>>> > Subject: [PATCH 0/3] NFS: Disable READ_PLUS by default
> >>>>>
> >>>>> > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> >>>>> >
> >>>>> > I've been scratching my head about what's going on with xfstests generic/091
> >>>>> > and generic/263, but I'm not sure what else to look at at this point.
> >>>>> > This patchset disables READ_PLUS by default by marking it as a
> >>>>> > developer-only kconfig option.
> >>>>> >
> >>>>> > I also included a couple of patches fixing some other issues that were
> >>>>> > noticed while inspecting the code. These patches don't help the tests
> >>>>> > pass, but they do fail later on after applying so it does feel like
> >>>>> > progress.
> >>>>> >
> >>>>> > I'm hopeful the remaning issues can be worked out in the future.
> >>>>> >
> >>>>> > Thanks,
> >>>>> > Anna
> >>>>> >
> >>>>> >
> >>>>> > Anna Schumaker (3):
> >>>>> >  NFS: Disable READ_PLUS by default
> >>>>> >  NFS: Allocate a scratch page for READ_PLUS
> >>>>> >  SUNRPC: Keep buf->len in sync with xdr->nwords when expanding holes
> >>>>> >
> >>>>> > fs/nfs/Kconfig          |  9 +++++++++
> >>>>> > fs/nfs/nfs42xdr.c       |  2 ++
> >>>>> > fs/nfs/nfs4proc.c       |  2 +-
> >>>>> > fs/nfs/read.c           | 13 +++++++++++--
> >>>>> > include/linux/nfs_xdr.h |  1 +
> >>>>> > net/sunrpc/xdr.c        |  3 ++-
> >>>>> > 6 files changed, 26 insertions(+), 4 deletions(-)
> >>>>> >
> >>>>> > --
> > > > > > > 2.29.2

Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

[Trond Myklebust]

On Fri, 2020-12-04 at 15:00 -0500, Olga Kornievskaia wrote:
> I object to putting the disable patch in, I think we need to fix the
> problem.

I can't see the problem is fixable in 5.10. There are way too many
changes required, and we're in the middle of the week of the last -rc
for 5.10. Furthermore, there are no regressions introduced by just
disabling the functionality, because READ_PLUS has only just been
merged in this release cycle.

I therefore strongly suggest we just send [PATCH 1/3] NFS: Disable
READ_PLUS by default and then fix the rest in 5.11.


-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

[Olga Kornievskaia]

On Wed, Dec 9, 2020 at 11:59 AM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> On Fri, 2020-12-04 at 15:00 -0500, Olga Kornievskaia wrote:
> > I object to putting the disable patch in, I think we need to fix the
> > problem.
>
> I can't see the problem is fixable in 5.10. There are way too many
> changes required, and we're in the middle of the week of the last -rc
> for 5.10. Furthermore, there are no regressions introduced by just
> disabling the functionality, because READ_PLUS has only just been
> merged in this release cycle.
>
> I therefore strongly suggest we just send [PATCH 1/3] NFS: Disable
> READ_PLUS by default and then fix the rest in 5.11.

Sure, but shouldn't there be more ifdefs inside of the xdr code to
turn it off completely?

>
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>

Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

[Trond Myklebust]

On Wed, 2020-12-09 at 12:07 -0500, Olga Kornievskaia wrote:
> On Wed, Dec 9, 2020 at 11:59 AM Trond Myklebust
> <trondmy@hammerspace.com> wrote:
> > 
> > On Fri, 2020-12-04 at 15:00 -0500, Olga Kornievskaia wrote:
> > > I object to putting the disable patch in, I think we need to fix
> > > the
> > > problem.
> > 
> > I can't see the problem is fixable in 5.10. There are way too many
> > changes required, and we're in the middle of the week of the last -
> > rc
> > for 5.10. Furthermore, there are no regressions introduced by just
> > disabling the functionality, because READ_PLUS has only just been
> > merged in this release cycle.
> > 
> > I therefore strongly suggest we just send [PATCH 1/3] NFS: Disable
> > READ_PLUS by default and then fix the rest in 5.11.
> 
> Sure, but shouldn't there be more ifdefs inside of the xdr code to
> turn it off completely?

AFAICT, those functions are not called by anything else, so as long as
the READ_PLUS client functionality is disabled, they should be
harmless.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

[Olga Kornievskaia]

On Wed, Dec 9, 2020 at 12:12 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> On Wed, 2020-12-09 at 12:07 -0500, Olga Kornievskaia wrote:
> > On Wed, Dec 9, 2020 at 11:59 AM Trond Myklebust
> > <trondmy@hammerspace.com> wrote:
> > >
> > > On Fri, 2020-12-04 at 15:00 -0500, Olga Kornievskaia wrote:
> > > > I object to putting the disable patch in, I think we need to fix
> > > > the
> > > > problem.
> > >
> > > I can't see the problem is fixable in 5.10. There are way too many
> > > changes required, and we're in the middle of the week of the last -
> > > rc
> > > for 5.10. Furthermore, there are no regressions introduced by just
> > > disabling the functionality, because READ_PLUS has only just been
> > > merged in this release cycle.
> > >
> > > I therefore strongly suggest we just send [PATCH 1/3] NFS: Disable
> > > READ_PLUS by default and then fix the rest in 5.11.
> >
> > Sure, but shouldn't there be more ifdefs inside of the xdr code to
> > turn it off completely?
>
> AFAICT, those functions are not called by anything else, so as long as
> the READ_PLUS client functionality is disabled, they should be
> harmless.

Is it benign that in the normal read path sunrpc will be calling a new
function of xdr_realign_pages()? Non readplus code didn't have it.

>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>

Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

[Anna Schumaker]

On Wed, Dec 9, 2020 at 12:22 PM Olga Kornievskaia <aglo@umich.edu> wrote:
>
> On Wed, Dec 9, 2020 at 12:12 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
> >
> > On Wed, 2020-12-09 at 12:07 -0500, Olga Kornievskaia wrote:
> > > On Wed, Dec 9, 2020 at 11:59 AM Trond Myklebust
> > > <trondmy@hammerspace.com> wrote:
> > > >
> > > > On Fri, 2020-12-04 at 15:00 -0500, Olga Kornievskaia wrote:
> > > > > I object to putting the disable patch in, I think we need to fix
> > > > > the
> > > > > problem.
> > > >
> > > > I can't see the problem is fixable in 5.10. There are way too many
> > > > changes required, and we're in the middle of the week of the last -
> > > > rc
> > > > for 5.10. Furthermore, there are no regressions introduced by just
> > > > disabling the functionality, because READ_PLUS has only just been
> > > > merged in this release cycle.
> > > >
> > > > I therefore strongly suggest we just send [PATCH 1/3] NFS: Disable
> > > > READ_PLUS by default and then fix the rest in 5.11.
> > >
> > > Sure, but shouldn't there be more ifdefs inside of the xdr code to
> > > turn it off completely?
> >
> > AFAICT, those functions are not called by anything else, so as long as
> > the READ_PLUS client functionality is disabled, they should be
> > harmless.
>
> Is it benign that in the normal read path sunrpc will be calling a new
> function of xdr_realign_pages()? Non readplus code didn't have it.

It should be. All I did was pull out some code from xdr_align_pages()
and put it into a new function. `git show --diff-algorithm=histogram`
says this is what I did:

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 909920fab93b..d93bcad5ba9f 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -997,10 +997,25 @@ __be32 * xdr_inline_decode(struct xdr_stream
*xdr, size_t nbytes)
 }
 EXPORT_SYMBOL_GPL(xdr_inline_decode);

+static void xdr_realign_pages(struct xdr_stream *xdr)
+{
+       struct xdr_buf *buf = xdr->buf;
+       struct kvec *iov = buf->head;
+       unsigned int cur = xdr_stream_pos(xdr);
+       unsigned int copied, offset;
+
+       /* Realign pages to current pointer position */
+       if (iov->iov_len > cur) {
+               offset = iov->iov_len - cur;
+               copied = xdr_shrink_bufhead(buf, offset);
+               trace_rpc_xdr_alignment(xdr, offset, copied);
+               xdr->nwords = XDR_QUADLEN(buf->len - cur);
+       }
+}
+
 static unsigned int xdr_align_pages(struct xdr_stream *xdr, unsigned int len)
 {
        struct xdr_buf *buf = xdr->buf;
-       struct kvec *iov;
        unsigned int nwords = XDR_QUADLEN(len);
        unsigned int cur = xdr_stream_pos(xdr);
        unsigned int copied, offset;
@@ -1008,15 +1023,7 @@ static unsigned int xdr_align_pages(struct
xdr_stream *xdr, unsigned int len)
        if (xdr->nwords == 0)
                return 0;

-       /* Realign pages to current pointer position */
-       iov = buf->head;
-       if (iov->iov_len > cur) {
-               offset = iov->iov_len - cur;
-               copied = xdr_shrink_bufhead(buf, offset);
-               trace_rpc_xdr_alignment(xdr, offset, copied);
-               xdr->nwords = XDR_QUADLEN(buf->len - cur);
-       }
-
+       xdr_realign_pages(xdr);
        if (nwords > xdr->nwords) {
                nwords = xdr->nwords;
                len = nwords << 2;


>
> >
> > --
> > Trond Myklebust
> > Linux NFS client maintainer, Hammerspace
> > trond.myklebust@hammerspace.com
> >
> >

Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

[Trond Myklebust]

On Wed, 2020-12-09 at 12:22 -0500, Olga Kornievskaia wrote:
> On Wed, Dec 9, 2020 at 12:12 PM Trond Myklebust <
> trondmy@hammerspace.com> wrote:
> > 
> > On Wed, 2020-12-09 at 12:07 -0500, Olga Kornievskaia wrote:
> > > On Wed, Dec 9, 2020 at 11:59 AM Trond Myklebust
> > > <trondmy@hammerspace.com> wrote:
> > > > 
> > > > On Fri, 2020-12-04 at 15:00 -0500, Olga Kornievskaia wrote:
> > > > > I object to putting the disable patch in, I think we need to
> > > > > fix
> > > > > the
> > > > > problem.
> > > > 
> > > > I can't see the problem is fixable in 5.10. There are way too
> > > > many
> > > > changes required, and we're in the middle of the week of the
> > > > last -
> > > > rc
> > > > for 5.10. Furthermore, there are no regressions introduced by
> > > > just
> > > > disabling the functionality, because READ_PLUS has only just
> > > > been
> > > > merged in this release cycle.
> > > > 
> > > > I therefore strongly suggest we just send [PATCH 1/3] NFS:
> > > > Disable
> > > > READ_PLUS by default and then fix the rest in 5.11.
> > > 
> > > Sure, but shouldn't there be more ifdefs inside of the xdr code
> > > to
> > > turn it off completely?
> > 
> > AFAICT, those functions are not called by anything else, so as long
> > as
> > the READ_PLUS client functionality is disabled, they should be
> > harmless.
> 
> Is it benign that in the normal read path sunrpc will be calling a
> new
> function of xdr_realign_pages()? Non readplus code didn't have it.
> > 

Looking at commit 06216ecbd9368 (
https://git.linux-nfs.org/?p=trondmy/linux-nfs.git;a=commitdiff;h=06216ecbd9368
) it is not actually changing the Linux-5.9 code, but is just
performing a trivial refactoring of that code into a new function. I'm
OK with that.

The rest of the READ code looks unchanged to me.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

[Olga Kornievskaia]

On Wed, Dec 9, 2020 at 12:29 PM Anna Schumaker <schumaker.anna@gmail.com> wrote:
>
> On Wed, Dec 9, 2020 at 12:22 PM Olga Kornievskaia <aglo@umich.edu> wrote:
> >
> > On Wed, Dec 9, 2020 at 12:12 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
> > >
> > > On Wed, 2020-12-09 at 12:07 -0500, Olga Kornievskaia wrote:
> > > > On Wed, Dec 9, 2020 at 11:59 AM Trond Myklebust
> > > > <trondmy@hammerspace.com> wrote:
> > > > >
> > > > > On Fri, 2020-12-04 at 15:00 -0500, Olga Kornievskaia wrote:
> > > > > > I object to putting the disable patch in, I think we need to fix
> > > > > > the
> > > > > > problem.
> > > > >
> > > > > I can't see the problem is fixable in 5.10. There are way too many
> > > > > changes required, and we're in the middle of the week of the last -
> > > > > rc
> > > > > for 5.10. Furthermore, there are no regressions introduced by just
> > > > > disabling the functionality, because READ_PLUS has only just been
> > > > > merged in this release cycle.
> > > > >
> > > > > I therefore strongly suggest we just send [PATCH 1/3] NFS: Disable
> > > > > READ_PLUS by default and then fix the rest in 5.11.
> > > >
> > > > Sure, but shouldn't there be more ifdefs inside of the xdr code to
> > > > turn it off completely?
> > >
> > > AFAICT, those functions are not called by anything else, so as long as
> > > the READ_PLUS client functionality is disabled, they should be
> > > harmless.
> >
> > Is it benign that in the normal read path sunrpc will be calling a new
> > function of xdr_realign_pages()? Non readplus code didn't have it.
>
> It should be. All I did was pull out some code from xdr_align_pages()
> and put it into a new function. `git show --diff-algorithm=histogram`
> says this is what I did:

Ok sounds good then. I just wanted to double check.

> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 909920fab93b..d93bcad5ba9f 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -997,10 +997,25 @@ __be32 * xdr_inline_decode(struct xdr_stream
> *xdr, size_t nbytes)
>  }
>  EXPORT_SYMBOL_GPL(xdr_inline_decode);
>
> +static void xdr_realign_pages(struct xdr_stream *xdr)
> +{
> +       struct xdr_buf *buf = xdr->buf;
> +       struct kvec *iov = buf->head;
> +       unsigned int cur = xdr_stream_pos(xdr);
> +       unsigned int copied, offset;
> +
> +       /* Realign pages to current pointer position */
> +       if (iov->iov_len > cur) {
> +               offset = iov->iov_len - cur;
> +               copied = xdr_shrink_bufhead(buf, offset);
> +               trace_rpc_xdr_alignment(xdr, offset, copied);
> +               xdr->nwords = XDR_QUADLEN(buf->len - cur);
> +       }
> +}
> +
>  static unsigned int xdr_align_pages(struct xdr_stream *xdr, unsigned int len)
>  {
>         struct xdr_buf *buf = xdr->buf;
> -       struct kvec *iov;
>         unsigned int nwords = XDR_QUADLEN(len);
>         unsigned int cur = xdr_stream_pos(xdr);
>         unsigned int copied, offset;
> @@ -1008,15 +1023,7 @@ static unsigned int xdr_align_pages(struct
> xdr_stream *xdr, unsigned int len)
>         if (xdr->nwords == 0)
>                 return 0;
>
> -       /* Realign pages to current pointer position */
> -       iov = buf->head;
> -       if (iov->iov_len > cur) {
> -               offset = iov->iov_len - cur;
> -               copied = xdr_shrink_bufhead(buf, offset);
> -               trace_rpc_xdr_alignment(xdr, offset, copied);
> -               xdr->nwords = XDR_QUADLEN(buf->len - cur);
> -       }
> -
> +       xdr_realign_pages(xdr);
>         if (nwords > xdr->nwords) {
>                 nwords = xdr->nwords;
>                 len = nwords << 2;
>
>
> >
> > >
> > > --
> > > Trond Myklebust
> > > Linux NFS client maintainer, Hammerspace
> > > trond.myklebust@hammerspace.com
> > >
> > >

Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

[Olga Kornievskaia]

On Wed, Dec 9, 2020 at 12:32 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> On Wed, 2020-12-09 at 12:22 -0500, Olga Kornievskaia wrote:
> > On Wed, Dec 9, 2020 at 12:12 PM Trond Myklebust <
> > trondmy@hammerspace.com> wrote:
> > >
> > > On Wed, 2020-12-09 at 12:07 -0500, Olga Kornievskaia wrote:
> > > > On Wed, Dec 9, 2020 at 11:59 AM Trond Myklebust
> > > > <trondmy@hammerspace.com> wrote:
> > > > >
> > > > > On Fri, 2020-12-04 at 15:00 -0500, Olga Kornievskaia wrote:
> > > > > > I object to putting the disable patch in, I think we need to
> > > > > > fix
> > > > > > the
> > > > > > problem.
> > > > >
> > > > > I can't see the problem is fixable in 5.10. There are way too
> > > > > many
> > > > > changes required, and we're in the middle of the week of the
> > > > > last -
> > > > > rc
> > > > > for 5.10. Furthermore, there are no regressions introduced by
> > > > > just
> > > > > disabling the functionality, because READ_PLUS has only just
> > > > > been
> > > > > merged in this release cycle.
> > > > >
> > > > > I therefore strongly suggest we just send [PATCH 1/3] NFS:
> > > > > Disable
> > > > > READ_PLUS by default and then fix the rest in 5.11.
> > > >
> > > > Sure, but shouldn't there be more ifdefs inside of the xdr code
> > > > to
> > > > turn it off completely?
> > >
> > > AFAICT, those functions are not called by anything else, so as long
> > > as
> > > the READ_PLUS client functionality is disabled, they should be
> > > harmless.
> >
> > Is it benign that in the normal read path sunrpc will be calling a
> > new
> > function of xdr_realign_pages()? Non readplus code didn't have it.
> > >
>
> Looking at commit 06216ecbd9368 (
> https://git.linux-nfs.org/?p=trondmy/linux-nfs.git;a=commitdiff;h=06216ecbd9368
> ) it is not actually changing the Linux-5.9 code, but is just
> performing a trivial refactoring of that code into a new function. I'm
> OK with that.
>
> The rest of the READ code looks unchanged to me.

Thank you for checking.

> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>

Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

[Mkrtchyan, Tigran]

Hi Anna,


finally I understand whats going on...

The the flex_file layout calls standard nfs4_proc_read_setup which
uses READ or READ_PLUS based on capabilities of NFS_SERVER(hdr->inode).
Unfortunately, this returns the pointer to MDS, thus READ_PLUS is get used.

The following diff is a brute-force fix.

[os-46-nfs-devel linux-nfs]$ git diff
diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
index a163533446fa..0d820a04ac3b 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.c
+++ b/fs/nfs/flexfilelayout/flexfilelayout.c
@@ -1390,6 +1390,7 @@ static void ff_layout_read_prepare_v4(struct rpc_task *task, void *data)
 {
        struct nfs_pgio_header *hdr = data;

+       nfs4_proc_read41_setup(&task->tk_msg);
        if (nfs4_setup_sequence(hdr->ds_clp,
                                &hdr->args.seq_args,
                                &hdr->res.seq_res,
diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index 065cb04222a1..6b89ebd88838 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -525,7 +525,7 @@ extern int nfs4_setup_sequence(struct nfs_client *client,
                                struct rpc_task *task);
 extern int nfs4_sequence_done(struct rpc_task *task,
                              struct nfs4_sequence_res *res);
-
+extern void nfs4_proc_read41_setup(struct rpc_message *msg);
 extern void nfs4_free_lock_state(struct nfs_server *server, struct nfs4_lock_state *lsp);
 extern int nfs4_proc_commit(struct file *dst, __u64 offset, __u32 count, struct nfs_commitres *res);
 extern const nfs4_stateid zero_stateid;
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 9e0ca9b2b210..bb5373deb586 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5334,6 +5334,12 @@ static void nfs4_proc_read_setup(struct nfs_pgio_header *hdr,
        nfs4_init_sequence(&hdr->args.seq_args, &hdr->res.seq_res, 0, 0);
 }

+void nfs4_proc_read41_setup(struct rpc_message *msg)
+{
+       msg->rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_READ];
+}
+EXPORT_SYMBOL_GPL(nfs4_proc_read41_setup);
+
 static int nfs4_proc_pgio_rpc_prepare(struct rpc_task *task,
                                      struct nfs_pgio_header *hdr)
 {


Something in that direction can be done to get it to work (I am happy to make
required changes).

Regards,
   Tigran.

----- Original Message -----
> From: "Anna Schumaker" <anna.schumaker@netapp.com>
> To: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
> Cc: "linux-nfs" <linux-nfs@vger.kernel.org>, "Olga Kornievskaia" <aglo@umich.edu>
> Sent: Tuesday, 8 December, 2020 14:38:10
> Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

> Hi Tigran,
> 
> On Tue, Dec 8, 2020 at 7:39 AM Mkrtchyan, Tigran
> <tigran.mkrtchyan@desy.de> wrote:
>>
>>
>>
>> Hi Anna et al.,
>>
>> I run some more test and found two issues:
>>
>> 1. our server returns on getdeviceinfo notification bitmap of size 2.
>>    Though, only the first element has non zero values, some how this
>>    makes client unhappy, probably due to definition on decode_getdeviceinfo_maxsz
>>    that expects bitmap length to be 1.
>>
>> 2. The client uses READ_PLUS to DS despite the fact that flex file
>>    layout stated that DS supports nfs v4.1
>>
>> Network File System, Ops(3): SEQUENCE, PUTFH, READ_PLUS
>>     [Program Version: 4]
>>     [V4 Procedure: COMPOUND (1)]
>>     Tag: <EMPTY>
>>     minorversion: 1
>>     Operations (count: 3): SEQUENCE, PUTFH, READ_PLUS
>>         Opcode: SEQUENCE (53)
>>         Opcode: PUTFH (22)
>>         Opcode: READ_PLUS (68)
>>             StateID
>>             offset: 0
>>             count: 10016
>>     [Main Opcode: READ_PLUS (68)]
> 
> Thanks for the update! You're right, READ_PLUS to the DS should not be
> happening. I'll look into why it is.
> 
> Anna
> 
>>
>>
>> I should re-run some tests with Trond's tree as I was checking the
>> DS errors in during the test I run last weeks.
>>
>> Regards,
>>    Tigran.
>>
>> ----- Original Message -----
>> > From: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
>> > To: "Anna Schumaker" <Anna.Schumaker@netapp.com>
>> > Cc: "schumaker anna" <schumaker.anna@gmail.com>, "linux-nfs"
>> > <linux-nfs@vger.kernel.org>, "Olga Kornievskaia"
>> > <aglo@umich.edu>
>> > Sent: Monday, 7 December, 2020 16:54:37
>> > Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default
>>
>> > Sorry, completely confused. It the same commit as before
>> > c567552612ece787b178e3b147b5854ad422a836
>> >
>> > Tigran.
>> >
>> >
>> > ----- Original Message -----
>> >> From: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
>> >> To: "Anna Schumaker" <Anna.Schumaker@netapp.com>
>> >> Cc: "schumaker anna" <schumaker.anna@gmail.com>, "linux-nfs"
>> >> <linux-nfs@vger.kernel.org>, "Olga Kornievskaia"
>> >> <aglo@umich.edu>
>> >> Sent: Monday, 7 December, 2020 16:26:05
>> >> Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default
>> >
>> >> Hi Anna
>> >>
>> >> I re-run bisect on your tree with tag nfs-for-5.10-1 as bad and 5.9.0 as good.
>> >>
>> >> The bisect point to commit a14a63594cc2e5bdcbb1543d29df945da71e380f, which makes
>> >> more sense.
>> >>
>> >> [centos@os-46-nfs-devel linux-nfs]$ git bisect good
>> >> c567552612ece787b178e3b147b5854ad422a836 is the first bad commit
>> >> commit c567552612ece787b178e3b147b5854ad422a836
>> >> Author: Anna Schumaker <Anna.Schumaker@Netapp.com>
>> >> Date:   Wed May 28 13:41:22 2014 -0400
>> >>
>> >>    NFS: Add READ_PLUS data segment support
>> >>
>> >>    This patch adds client support for decoding a single NFS4_CONTENT_DATA
>> >>    segment returned by the server. This is the simplest implementation
>> >>    possible, since it does not account for any hole segments in the reply.
>> >>
>> >>    Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
>> >>
>> >> fs/nfs/nfs42xdr.c         | 141 ++++++++++++++++++++++++++++++++++++++++++++++
>> >> fs/nfs/nfs4client.c       |   2 +
>> >> fs/nfs/nfs4proc.c         |  43 +++++++++++++-
>> >> fs/nfs/nfs4xdr.c          |   1 +
>> >> include/linux/nfs4.h      |   2 +-
>> >> include/linux/nfs_fs_sb.h |   1 +
>> >> include/linux/nfs_xdr.h   |   2 +-
>> >> 7 files changed, 187 insertions(+), 5 deletions(-)
>> >>
>> >>
>> >> Best regards,
>> >>   Tigran
>> >>
>> >> ----- Original Message -----
>> >>> From: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
>> >>> To: "Olga Kornievskaia" <aglo@umich.edu>
>> >>> Cc: "schumaker anna" <schumaker.anna@gmail.com>, "linux-nfs"
>> >>> <linux-nfs@vger.kernel.org>, "Anna Schumaker"
>> >>> <Anna.Schumaker@netapp.com>
>> >>> Sent: Friday, 4 December, 2020 23:50:27
>> >>> Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default
>> >>
>> >>> I agree with Olga, especially that disabling doesn't fixes my issue.
>> >>> Unfortunately I have no idea how kernel's vm works, but I am
>> >>> ready to test as many times as needed.
>> >>>
>> >>> Thanks,
>> >>>   Tigran.
>> >>>
>> >>> ----- Original Message -----
>> >>>> From: "Olga Kornievskaia" <aglo@umich.edu>
>> >>>> To: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
>> >>>> Cc: "schumaker anna" <schumaker.anna@gmail.com>, "linux-nfs"
>> >>>> <linux-nfs@vger.kernel.org>, "Anna Schumaker"
>> >>>> <Anna.Schumaker@netapp.com>
>> >>>> Sent: Friday, 4 December, 2020 21:00:32
>> >>>> Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default
>> >>>
>> >>>> I object to putting the disable patch in, I think we need to fix the problem.
>> >>>>
>> >>>> The current problem is generic/263 is failing because the end of the
>> >>>> buffer is corrupted since we lost the bytes when hole was expanded. I
>> >>>> don't know what the solution is: (1) hole expanding handling needs to
>> >>>> be fixed to not lose data or (2) we shouldnt be reporting that we read
>> >>>> the bytes we lost.
>> >>>>
>> >>>> On Fri, Dec 4, 2020 at 10:49 AM Mkrtchyan, Tigran
>> >>>> <tigran.mkrtchyan@desy.de> wrote:
>> >>>>>
>> >>>>> Hi Anna,
>> >>>>>
>> >>>>> I see problems with gedeviceinfo and bisected it to
>> >>>>> c567552612ece787b178e3b147b5854ad422a836.
>> >>>>> The commit itself doesn't look that can break it, but might
>> >>>>> be can help you find the problem.
>> >>>>>
>> >>>>> What I see, that after xdr_read_pages call the xdr stream points
>> >>>>> to a some random point (or wrong page)
>> >>>>>
>> >>>>> Regards,
>> >>>>>    Tigran.
>> >>>>>
>> >>>>>
>> >>>>> ----- Original Message -----
>> >>>>> > From: "schumaker anna" <schumaker.anna@gmail.com>
>> >>>>> > To: "linux-nfs" <linux-nfs@vger.kernel.org>
>> >>>>> > Cc: "Anna Schumaker" <Anna.Schumaker@Netapp.com>
>> >>>>> > Sent: Thursday, 3 December, 2020 21:18:38
>> >>>>> > Subject: [PATCH 0/3] NFS: Disable READ_PLUS by default
>> >>>>>
>> >>>>> > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
>> >>>>> >
>> >>>>> > I've been scratching my head about what's going on with xfstests generic/091
>> >>>>> > and generic/263, but I'm not sure what else to look at at this point.
>> >>>>> > This patchset disables READ_PLUS by default by marking it as a
>> >>>>> > developer-only kconfig option.
>> >>>>> >
>> >>>>> > I also included a couple of patches fixing some other issues that were
>> >>>>> > noticed while inspecting the code. These patches don't help the tests
>> >>>>> > pass, but they do fail later on after applying so it does feel like
>> >>>>> > progress.
>> >>>>> >
>> >>>>> > I'm hopeful the remaning issues can be worked out in the future.
>> >>>>> >
>> >>>>> > Thanks,
>> >>>>> > Anna
>> >>>>> >
>> >>>>> >
>> >>>>> > Anna Schumaker (3):
>> >>>>> >  NFS: Disable READ_PLUS by default
>> >>>>> >  NFS: Allocate a scratch page for READ_PLUS
>> >>>>> >  SUNRPC: Keep buf->len in sync with xdr->nwords when expanding holes
>> >>>>> >
>> >>>>> > fs/nfs/Kconfig          |  9 +++++++++
>> >>>>> > fs/nfs/nfs42xdr.c       |  2 ++
>> >>>>> > fs/nfs/nfs4proc.c       |  2 +-
>> >>>>> > fs/nfs/read.c           | 13 +++++++++++--
>> >>>>> > include/linux/nfs_xdr.h |  1 +
>> >>>>> > net/sunrpc/xdr.c        |  3 ++-
>> >>>>> > 6 files changed, 26 insertions(+), 4 deletions(-)
>> >>>>> >
>> >>>>> > --
> > > > > > > > 2.29.2