LinuxLists.cc - [PATCH 0/3] NFS: Disable READ

[permalink] [raw]

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

From: Anna Schumaker <[email protected]>

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 <[email protected]>
---
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

2020-12-03 20:20:59

[permalink] [raw]

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

From: Anna Schumaker <[email protected]>

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

Signed-off-by: Anna Schumaker <[email protected]>
---
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

2020-12-04 15:49:46

[permalink] [raw]

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

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" <[email protected]>
> To: "linux-nfs" <[email protected]>
> Cc: "Anna Schumaker" <[email protected]>
> Sent: Thursday, 3 December, 2020 21:18:38
> Subject: [PATCH 0/3] NFS: Disable READ_PLUS by default

> From: Anna Schumaker <[email protected]>
>
> 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

2020-12-04 20:03:37

[permalink] [raw]

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
<[email protected]> 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" <[email protected]>
> > To: "linux-nfs" <[email protected]>
> > Cc: "Anna Schumaker" <[email protected]>
> > Sent: Thursday, 3 December, 2020 21:18:38
> > Subject: [PATCH 0/3] NFS: Disable READ_PLUS by default
>
> > From: Anna Schumaker <[email protected]>
> >
> > 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

2020-12-04 22:52:47

[permalink] [raw]

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" <[email protected]>
> To: "Tigran Mkrtchyan" <[email protected]>
> Cc: "schumaker anna" <[email protected]>, "linux-nfs" <[email protected]>, "Anna Schumaker"
> <[email protected]>
> 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
> <[email protected]> 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" <[email protected]>
>> > To: "linux-nfs" <[email protected]>
>> > Cc: "Anna Schumaker" <[email protected]>
>> > Sent: Thursday, 3 December, 2020 21:18:38
>> > Subject: [PATCH 0/3] NFS: Disable READ_PLUS by default
>>
>> > From: Anna Schumaker <[email protected]>
>> >
>> > 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

2020-12-07 15:27:37

[permalink] [raw]

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 <[email protected]>
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 <[email protected]>

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" <[email protected]>
> To: "Olga Kornievskaia" <[email protected]>
> Cc: "schumaker anna" <[email protected]>, "linux-nfs" <[email protected]>, "Anna Schumaker"
> <[email protected]>
> 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" <[email protected]>
>> To: "Tigran Mkrtchyan" <[email protected]>
>> Cc: "schumaker anna" <[email protected]>, "linux-nfs"
>> <[email protected]>, "Anna Schumaker"
>> <[email protected]>
>> 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
>> <[email protected]> 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" <[email protected]>
>>> > To: "linux-nfs" <[email protected]>
>>> > Cc: "Anna Schumaker" <[email protected]>
>>> > Sent: Thursday, 3 December, 2020 21:18:38
>>> > Subject: [PATCH 0/3] NFS: Disable READ_PLUS by default
>>>
>>> > From: Anna Schumaker <[email protected]>
>>> >
>>> > 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

2020-12-07 15:56:45

[permalink] [raw]

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" <[email protected]>
> To: "Anna Schumaker" <[email protected]>
> Cc: "schumaker anna" <[email protected]>, "linux-nfs" <[email protected]>, "Olga Kornievskaia"
> <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
>
> 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" <[email protected]>
>> To: "Olga Kornievskaia" <[email protected]>
>> Cc: "schumaker anna" <[email protected]>, "linux-nfs"
>> <[email protected]>, "Anna Schumaker"
>> <[email protected]>
>> 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" <[email protected]>
>>> To: "Tigran Mkrtchyan" <[email protected]>
>>> Cc: "schumaker anna" <[email protected]>, "linux-nfs"
>>> <[email protected]>, "Anna Schumaker"
>>> <[email protected]>
>>> 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
>>> <[email protected]> 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" <[email protected]>
>>>> > To: "linux-nfs" <[email protected]>
>>>> > Cc: "Anna Schumaker" <[email protected]>
>>>> > Sent: Thursday, 3 December, 2020 21:18:38
>>>> > Subject: [PATCH 0/3] NFS: Disable READ_PLUS by default
>>>>
>>>> > From: Anna Schumaker <[email protected]>
>>>> >
>>>> > 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

2020-12-08 12:57:34

[permalink] [raw]

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

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" <[email protected]>
> To: "Anna Schumaker" <[email protected]>
> Cc: "schumaker anna" <[email protected]>, "linux-nfs" <[email protected]>, "Olga Kornievskaia"
> <[email protected]>
> 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" <[email protected]>
>> To: "Anna Schumaker" <[email protected]>
>> Cc: "schumaker anna" <[email protected]>, "linux-nfs"
>> <[email protected]>, "Olga Kornievskaia"
>> <[email protected]>
>> 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 <[email protected]>
>> 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 <[email protected]>
>>
>> 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" <[email protected]>
>>> To: "Olga Kornievskaia" <[email protected]>
>>> Cc: "schumaker anna" <[email protected]>, "linux-nfs"
>>> <[email protected]>, "Anna Schumaker"
>>> <[email protected]>
>>> 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" <[email protected]>
>>>> To: "Tigran Mkrtchyan" <[email protected]>
>>>> Cc: "schumaker anna" <[email protected]>, "linux-nfs"
>>>> <[email protected]>, "Anna Schumaker"
>>>> <[email protected]>
>>>> 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
>>>> <[email protected]> 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" <[email protected]>
>>>>> > To: "linux-nfs" <[email protected]>
>>>>> > Cc: "Anna Schumaker" <[email protected]>
>>>>> > Sent: Thursday, 3 December, 2020 21:18:38
>>>>> > Subject: [PATCH 0/3] NFS: Disable READ_PLUS by default
>>>>>
>>>>> > From: Anna Schumaker <[email protected]>
>>>>> >
>>>>> > 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

2020-12-08 13:42:32

[permalink] [raw]

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

Hi Tigran,

On Tue, Dec 8, 2020 at 7:39 AM Mkrtchyan, Tigran
<[email protected]> 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" <[email protected]>
> > To: "Anna Schumaker" <[email protected]>
> > Cc: "schumaker anna" <[email protected]>, "linux-nfs" <[email protected]>, "Olga Kornievskaia"
> > <[email protected]>
> > 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" <[email protected]>
> >> To: "Anna Schumaker" <[email protected]>
> >> Cc: "schumaker anna" <[email protected]>, "linux-nfs"
> >> <[email protected]>, "Olga Kornievskaia"
> >> <[email protected]>
> >> 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 <[email protected]>
> >> 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 <[email protected]>
> >>
> >> 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" <[email protected]>
> >>> To: "Olga Kornievskaia" <[email protected]>
> >>> Cc: "schumaker anna" <[email protected]>, "linux-nfs"
> >>> <[email protected]>, "Anna Schumaker"
> >>> <[email protected]>
> >>> 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" <[email protected]>
> >>>> To: "Tigran Mkrtchyan" <[email protected]>
> >>>> Cc: "schumaker anna" <[email protected]>, "linux-nfs"
> >>>> <[email protected]>, "Anna Schumaker"
> >>>> <[email protected]>
> >>>> 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
> >>>> <[email protected]> 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" <[email protected]>
> >>>>> > To: "linux-nfs" <[email protected]>
> >>>>> > Cc: "Anna Schumaker" <[email protected]>
> >>>>> > Sent: Thursday, 3 December, 2020 21:18:38
> >>>>> > Subject: [PATCH 0/3] NFS: Disable READ_PLUS by default
> >>>>>
> >>>>> > From: Anna Schumaker <[email protected]>
> >>>>> >
> >>>>> > 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

2020-12-09 17:02:54

by Trond Myklebust

[permalink] [raw]

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

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
[email protected]

2020-12-09 17:11:23

[permalink] [raw]

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

On Wed, Dec 9, 2020 at 11:59 AM Trond Myklebust <[email protected]> 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
> [email protected]
>
>

2020-12-09 17:17:33

by Trond Myklebust

[permalink] [raw]

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

On Wed, 2020-12-09 at 12:07 -0500, Olga Kornievskaia wrote:
> On Wed, Dec 9, 2020 at 11:59 AM Trond Myklebust
> <[email protected]> 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
[email protected]

2020-12-09 17:25:24

[permalink] [raw]

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

On Wed, Dec 9, 2020 at 12:12 PM Trond Myklebust <[email protected]> wrote:
>
> On Wed, 2020-12-09 at 12:07 -0500, Olga Kornievskaia wrote:
> > On Wed, Dec 9, 2020 at 11:59 AM Trond Myklebust
> > <[email protected]> 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
> [email protected]
>
>

2020-12-09 17:33:53

[permalink] [raw]

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

On Wed, Dec 9, 2020 at 12:22 PM Olga Kornievskaia <[email protected]> wrote:
>
> On Wed, Dec 9, 2020 at 12:12 PM Trond Myklebust <[email protected]> wrote:
> >
> > On Wed, 2020-12-09 at 12:07 -0500, Olga Kornievskaia wrote:
> > > On Wed, Dec 9, 2020 at 11:59 AM Trond Myklebust
> > > <[email protected]> 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
> > [email protected]
> >
> >

2020-12-09 17:36:26

by Trond Myklebust

[permalink] [raw]

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

On Wed, 2020-12-09 at 12:22 -0500, Olga Kornievskaia wrote:
> On Wed, Dec 9, 2020 at 12:12 PM Trond Myklebust <
> [email protected]> wrote:
> >
> > On Wed, 2020-12-09 at 12:07 -0500, Olga Kornievskaia wrote:
> > > On Wed, Dec 9, 2020 at 11:59 AM Trond Myklebust
> > > <[email protected]> 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
[email protected]

2020-12-09 17:43:23

[permalink] [raw]

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

On Wed, Dec 9, 2020 at 12:29 PM Anna Schumaker <[email protected]> wrote:
>
> On Wed, Dec 9, 2020 at 12:22 PM Olga Kornievskaia <[email protected]> wrote:
> >
> > On Wed, Dec 9, 2020 at 12:12 PM Trond Myklebust <[email protected]> wrote:
> > >
> > > On Wed, 2020-12-09 at 12:07 -0500, Olga Kornievskaia wrote:
> > > > On Wed, Dec 9, 2020 at 11:59 AM Trond Myklebust
> > > > <[email protected]> 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
> > > [email protected]
> > >
> > >

2020-12-09 17:44:26

[permalink] [raw]

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

On Wed, Dec 9, 2020 at 12:32 PM Trond Myklebust <[email protected]> wrote:
>
> On Wed, 2020-12-09 at 12:22 -0500, Olga Kornievskaia wrote:
> > On Wed, Dec 9, 2020 at 12:12 PM Trond Myklebust <
> > [email protected]> wrote:
> > >
> > > On Wed, 2020-12-09 at 12:07 -0500, Olga Kornievskaia wrote:
> > > > On Wed, Dec 9, 2020 at 11:59 AM Trond Myklebust
> > > > <[email protected]> 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
> [email protected]
>
>

2020-12-09 18:25:48