2021-03-31 19:29:45

by Olga Kornievskaia

[permalink] [raw]
Subject: [PATCH 1/1] NFSD fix handling of NFSv4.2 SEEK for data within the last hole

From: Olga Kornievskaia <[email protected]>

According to the RFC 7862, "if the server cannot find a
corresponding sa_what, then the status will still be NFS4_OK,
but sr_eof would be TRUE". If there is a file that ends with
a hole and a SEEK request made for sa_what=SEEK_DATA with
an offset in the middle of the last hole, then the server
has to return OK and set the eof. Currently the linux server
returns ERR_NXIO.

Fixes: 24bab491220fa ("NFSD: Implement SEEK")
Signed-off-by: Olga Kornievskaia <[email protected]>
---
fs/nfsd/nfs4proc.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index e13c4c81fb89..2e7ceb9f1d5d 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1737,9 +1737,13 @@ nfsd4_seek(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
* should ever file->f_pos.
*/
seek->seek_pos = vfs_llseek(nf->nf_file, seek->seek_offset, whence);
- if (seek->seek_pos < 0)
- status = nfserrno(seek->seek_pos);
- else if (seek->seek_pos >= i_size_read(file_inode(nf->nf_file)))
+ if (seek->seek_pos < 0) {
+ if (whence == SEEK_DATA &&
+ seek->seek_offset < i_size_read(file_inode(nf->nf_file)))
+ seek->seek_eof = true;
+ else
+ status = nfserrno(seek->seek_pos);
+ } else if (seek->seek_pos >= i_size_read(file_inode(nf->nf_file)))
seek->seek_eof = true;

out:
--
2.18.2


2021-04-01 01:52:08

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 1/1] NFSD fix handling of NFSv4.2 SEEK for data within the last hole

On Wed, Mar 31, 2021 at 03:28:19PM -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <[email protected]>
>
> According to the RFC 7862, "if the server cannot find a
> corresponding sa_what, then the status will still be NFS4_OK,
> but sr_eof would be TRUE". If there is a file that ends with
> a hole and a SEEK request made for sa_what=SEEK_DATA with
> an offset in the middle of the last hole, then the server
> has to return OK and set the eof. Currently the linux server
> returns ERR_NXIO.

Makes sense, but I think you can use the return value from vfs_llseek
instead of checking the file size again. E.g.:

seek->seek_pos = vfs_llseek(nfs->nf_file, seek->seek_offset, whence);
if (seek->seek_pos == -ENXIO)
seek->seek_eof = true;
else if (seek->seek_pos < 0)
status = nfserrno(seek->seek_pos);

--b.

>
> Fixes: 24bab491220fa ("NFSD: Implement SEEK")
> Signed-off-by: Olga Kornievskaia <[email protected]>
> ---
> fs/nfsd/nfs4proc.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index e13c4c81fb89..2e7ceb9f1d5d 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1737,9 +1737,13 @@ nfsd4_seek(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> * should ever file->f_pos.
> */
> seek->seek_pos = vfs_llseek(nf->nf_file, seek->seek_offset, whence);
> - if (seek->seek_pos < 0)
> - status = nfserrno(seek->seek_pos);
> - else if (seek->seek_pos >= i_size_read(file_inode(nf->nf_file)))
> + if (seek->seek_pos < 0) {
> + if (whence == SEEK_DATA &&
> + seek->seek_offset < i_size_read(file_inode(nf->nf_file)))
> + seek->seek_eof = true;
> + else
> + status = nfserrno(seek->seek_pos);
> + } else if (seek->seek_pos >= i_size_read(file_inode(nf->nf_file)))
> seek->seek_eof = true;
>
> out:
> --
> 2.18.2
>

2021-04-01 18:28:23

by Olga Kornievskaia

[permalink] [raw]
Subject: Re: [PATCH 1/1] NFSD fix handling of NFSv4.2 SEEK for data within the last hole

On Wed, Mar 31, 2021 at 9:50 PM J. Bruce Fields <[email protected]> wrote:
>
> On Wed, Mar 31, 2021 at 03:28:19PM -0400, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <[email protected]>
> >
> > According to the RFC 7862, "if the server cannot find a
> > corresponding sa_what, then the status will still be NFS4_OK,
> > but sr_eof would be TRUE". If there is a file that ends with
> > a hole and a SEEK request made for sa_what=SEEK_DATA with
> > an offset in the middle of the last hole, then the server
> > has to return OK and set the eof. Currently the linux server
> > returns ERR_NXIO.
>
> Makes sense, but I think you can use the return value from vfs_llseek
> instead of checking the file size again. E.g.:
>
> seek->seek_pos = vfs_llseek(nfs->nf_file, seek->seek_offset, whence);
> if (seek->seek_pos == -ENXIO)
> seek->seek_eof = true;

I don't believe this is correct. (1) ENXIO doesn't imply eof. If the
specified seek_offset was beyond the end of the file the server must
return ERR_NXIO and not OK. and (2) for the same reason I need to
check if the requested type was looking for data but didn't find it
because the offset is in the middle of the hole but still within the
file size (thus the need to check if the seek_offset is within the
file size). But I'm happy to check specifically if the seek_pos was
ENXIO (and not the generic negative error) and then also check if
request was for data and request was within file size.

Also while I'm fixing this and have your attention, Can you tell if
the "else if" condition in the original code makes sense to you. I
didn't touch it but I don't think it's correct. "else if
(seek->seek_pos >= i_size_read(file_inode(nf->nf_file)))" I don't
believe this can ever happen. How can vfs_llseek() ever return a
position that is greater than the size of the file (or actually even
equal to it)?

> else if (seek->seek_pos < 0)
> status = nfserrno(seek->seek_pos);
>
> --b.
>
> >
> > Fixes: 24bab491220fa ("NFSD: Implement SEEK")
> > Signed-off-by: Olga Kornievskaia <[email protected]>
> > ---
> > fs/nfsd/nfs4proc.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index e13c4c81fb89..2e7ceb9f1d5d 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -1737,9 +1737,13 @@ nfsd4_seek(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > * should ever file->f_pos.
> > */
> > seek->seek_pos = vfs_llseek(nf->nf_file, seek->seek_offset, whence);
> > - if (seek->seek_pos < 0)
> > - status = nfserrno(seek->seek_pos);
> > - else if (seek->seek_pos >= i_size_read(file_inode(nf->nf_file)))
> > + if (seek->seek_pos < 0) {
> > + if (whence == SEEK_DATA &&
> > + seek->seek_offset < i_size_read(file_inode(nf->nf_file)))
> > + seek->seek_eof = true;
> > + else
> > + status = nfserrno(seek->seek_pos);
> > + } else if (seek->seek_pos >= i_size_read(file_inode(nf->nf_file)))
> > seek->seek_eof = true;
> >
> > out:
> > --
> > 2.18.2
> >
>

2021-04-01 21:33:59

by Rick Macklem

[permalink] [raw]
Subject: Re: [PATCH 1/1] NFSD fix handling of NFSv4.2 SEEK for data within the last hole

I discussed this on [email protected] some time ago.
The problem with "fixing" the server is that it breaks
the unpatched Linux client.

If the client is careful, it can work correctly for both a Linux
and RFC5661 conformant server. That's what the FreeBSD
client tries to do.
--> I'd suggest that be your main goal.

The FreeBSD server ships "Linux compatible", but there is
a switch to make it RFC5661 compatible.
--> I wouldn't make it default the RFC compatible for
quite a while after the client that handles RFC compatible
ships.

I tried to convince folks to "errata" the RFC to Linux
compatible (since Linux was the only shipping 4.2 at
the time, but they did not consider it an "errata").

Just mho, rick

________________________________________
From: Olga Kornievskaia <[email protected]>
Sent: Thursday, April 1, 2021 9:27 AM
To: J. Bruce Fields
Cc: Chuck Lever; linux-nfs
Subject: Re: [PATCH 1/1] NFSD fix handling of NFSv4.2 SEEK for data within the last hole

CAUTION: This email originated from outside of the University of Guelph. Do not click links or open attachments unless you recognize the sender and know the content is safe. If in doubt, forward suspicious emails to [email protected]


On Wed, Mar 31, 2021 at 9:50 PM J. Bruce Fields <[email protected]> wrote:
>
> On Wed, Mar 31, 2021 at 03:28:19PM -0400, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <[email protected]>
> >
> > According to the RFC 7862, "if the server cannot find a
> > corresponding sa_what, then the status will still be NFS4_OK,
> > but sr_eof would be TRUE". If there is a file that ends with
> > a hole and a SEEK request made for sa_what=SEEK_DATA with
> > an offset in the middle of the last hole, then the server
> > has to return OK and set the eof. Currently the linux server
> > returns ERR_NXIO.
>
> Makes sense, but I think you can use the return value from vfs_llseek
> instead of checking the file size again. E.g.:
>
> seek->seek_pos = vfs_llseek(nfs->nf_file, seek->seek_offset, whence);
> if (seek->seek_pos == -ENXIO)
> seek->seek_eof = true;

I don't believe this is correct. (1) ENXIO doesn't imply eof. If the
specified seek_offset was beyond the end of the file the server must
return ERR_NXIO and not OK. and (2) for the same reason I need to
check if the requested type was looking for data but didn't find it
because the offset is in the middle of the hole but still within the
file size (thus the need to check if the seek_offset is within the
file size). But I'm happy to check specifically if the seek_pos was
ENXIO (and not the generic negative error) and then also check if
request was for data and request was within file size.

Also while I'm fixing this and have your attention, Can you tell if
the "else if" condition in the original code makes sense to you. I
didn't touch it but I don't think it's correct. "else if
(seek->seek_pos >= i_size_read(file_inode(nf->nf_file)))" I don't
believe this can ever happen. How can vfs_llseek() ever return a
position that is greater than the size of the file (or actually even
equal to it)?

> else if (seek->seek_pos < 0)
> status = nfserrno(seek->seek_pos);
>
> --b.
>
> >
> > Fixes: 24bab491220fa ("NFSD: Implement SEEK")
> > Signed-off-by: Olga Kornievskaia <[email protected]>
> > ---
> > fs/nfsd/nfs4proc.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index e13c4c81fb89..2e7ceb9f1d5d 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -1737,9 +1737,13 @@ nfsd4_seek(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > * should ever file->f_pos.
> > */
> > seek->seek_pos = vfs_llseek(nf->nf_file, seek->seek_offset, whence);
> > - if (seek->seek_pos < 0)
> > - status = nfserrno(seek->seek_pos);
> > - else if (seek->seek_pos >= i_size_read(file_inode(nf->nf_file)))
> > + if (seek->seek_pos < 0) {
> > + if (whence == SEEK_DATA &&
> > + seek->seek_offset < i_size_read(file_inode(nf->nf_file)))
> > + seek->seek_eof = true;
> > + else
> > + status = nfserrno(seek->seek_pos);
> > + } else if (seek->seek_pos >= i_size_read(file_inode(nf->nf_file)))
> > seek->seek_eof = true;
> >
> > out:
> > --
> > 2.18.2
> >
>

2021-04-02 14:34:21

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 1/1] NFSD fix handling of NFSv4.2 SEEK for data within the last hole

On Thu, Apr 01, 2021 at 09:27:56AM -0400, Olga Kornievskaia wrote:
> On Wed, Mar 31, 2021 at 9:50 PM J. Bruce Fields <[email protected]> wrote:
> >
> > On Wed, Mar 31, 2021 at 03:28:19PM -0400, Olga Kornievskaia wrote:
> > > From: Olga Kornievskaia <[email protected]>
> > >
> > > According to the RFC 7862, "if the server cannot find a
> > > corresponding sa_what, then the status will still be NFS4_OK,
> > > but sr_eof would be TRUE". If there is a file that ends with
> > > a hole and a SEEK request made for sa_what=SEEK_DATA with
> > > an offset in the middle of the last hole, then the server
> > > has to return OK and set the eof. Currently the linux server
> > > returns ERR_NXIO.
> >
> > Makes sense, but I think you can use the return value from vfs_llseek
> > instead of checking the file size again. E.g.:
> >
> > seek->seek_pos = vfs_llseek(nfs->nf_file, seek->seek_offset, whence);
> > if (seek->seek_pos == -ENXIO)
> > seek->seek_eof = true;
>
> I don't believe this is correct. (1) ENXIO doesn't imply eof. If the
> specified seek_offset was beyond the end of the file the server must
> return ERR_NXIO and not OK.

OK, never mind.

> and (2) for the same reason I need to
> check if the requested type was looking for data but didn't find it
> because the offset is in the middle of the hole but still within the
> file size (thus the need to check if the seek_offset is within the
> file size). But I'm happy to check specifically if the seek_pos was
> ENXIO (and not the generic negative error) and then also check if
> request was for data and request was within file size.
>
> Also while I'm fixing this and have your attention, Can you tell if
> the "else if" condition in the original code makes sense to you. I
> didn't touch it but I don't think it's correct. "else if
> (seek->seek_pos >= i_size_read(file_inode(nf->nf_file)))" I don't
> believe this can ever happen. How can vfs_llseek() ever return a
> position that is greater than the size of the file (or actually even
> equal to it)?

I agree, I don't get it either.

--b.

>
> > else if (seek->seek_pos < 0)
> > status = nfserrno(seek->seek_pos);
> >
> > --b.
> >
> > >
> > > Fixes: 24bab491220fa ("NFSD: Implement SEEK")
> > > Signed-off-by: Olga Kornievskaia <[email protected]>
> > > ---
> > > fs/nfsd/nfs4proc.c | 10 +++++++---
> > > 1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > index e13c4c81fb89..2e7ceb9f1d5d 100644
> > > --- a/fs/nfsd/nfs4proc.c
> > > +++ b/fs/nfsd/nfs4proc.c
> > > @@ -1737,9 +1737,13 @@ nfsd4_seek(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > > * should ever file->f_pos.
> > > */
> > > seek->seek_pos = vfs_llseek(nf->nf_file, seek->seek_offset, whence);
> > > - if (seek->seek_pos < 0)
> > > - status = nfserrno(seek->seek_pos);
> > > - else if (seek->seek_pos >= i_size_read(file_inode(nf->nf_file)))
> > > + if (seek->seek_pos < 0) {
> > > + if (whence == SEEK_DATA &&
> > > + seek->seek_offset < i_size_read(file_inode(nf->nf_file)))
> > > + seek->seek_eof = true;
> > > + else
> > > + status = nfserrno(seek->seek_pos);
> > > + } else if (seek->seek_pos >= i_size_read(file_inode(nf->nf_file)))
> > > seek->seek_eof = true;
> > >
> > > out:
> > > --
> > > 2.18.2
> > >
> >
>

2021-04-02 21:03:59

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 1/1] NFSD fix handling of NFSv4.2 SEEK for data within the last hole

Adding [email protected]:

On Thu, Apr 01, 2021 at 09:32:05PM +0000, Rick Macklem wrote:
> I discussed this on [email protected] some time ago.
> The problem with "fixing" the server is that it breaks
> the unpatched Linux client.
>
> If the client is careful, it can work correctly for both a Linux
> and RFC5661 conformant server. That's what the FreeBSD
> client tries to do.
> --> I'd suggest that be your main goal.
>
> The FreeBSD server ships "Linux compatible", but there is
> a switch to make it RFC5661 compatible.
> --> I wouldn't make it default the RFC compatible for
> quite a while after the client that handles RFC compatible
> ships.
>
> I tried to convince folks to "errata" the RFC to Linux
> compatible (since Linux was the only shipping 4.2 at
> the time, but they did not consider it an "errata").

Previous discussion here:

https://mailarchive.ietf.org/arch/msg/nfsv4/bPLFnywt1wZ4kolMkzeSYca0qIM/

There's no rejection on that thread, was it elsewhere?

--b.

2021-04-09 18:40:09

by Olga Kornievskaia

[permalink] [raw]
Subject: Re: [PATCH 1/1] NFSD fix handling of NFSv4.2 SEEK for data within the last hole

On Fri, Apr 2, 2021 at 10:32 AM J. Bruce Fields <[email protected]> wrote:
>
> On Thu, Apr 01, 2021 at 09:27:56AM -0400, Olga Kornievskaia wrote:
> > On Wed, Mar 31, 2021 at 9:50 PM J. Bruce Fields <[email protected]> wrote:
> > >
> > > On Wed, Mar 31, 2021 at 03:28:19PM -0400, Olga Kornievskaia wrote:
> > > > From: Olga Kornievskaia <[email protected]>
> > > >
> > > > According to the RFC 7862, "if the server cannot find a
> > > > corresponding sa_what, then the status will still be NFS4_OK,
> > > > but sr_eof would be TRUE". If there is a file that ends with
> > > > a hole and a SEEK request made for sa_what=SEEK_DATA with
> > > > an offset in the middle of the last hole, then the server
> > > > has to return OK and set the eof. Currently the linux server
> > > > returns ERR_NXIO.
> > >
> > > Makes sense, but I think you can use the return value from vfs_llseek
> > > instead of checking the file size again. E.g.:
> > >
> > > seek->seek_pos = vfs_llseek(nfs->nf_file, seek->seek_offset, whence);
> > > if (seek->seek_pos == -ENXIO)
> > > seek->seek_eof = true;
> >
> > I don't believe this is correct. (1) ENXIO doesn't imply eof. If the
> > specified seek_offset was beyond the end of the file the server must
> > return ERR_NXIO and not OK.
>
> OK, never mind.
>
> > and (2) for the same reason I need to
> > check if the requested type was looking for data but didn't find it
> > because the offset is in the middle of the hole but still within the
> > file size (thus the need to check if the seek_offset is within the
> > file size). But I'm happy to check specifically if the seek_pos was
> > ENXIO (and not the generic negative error) and then also check if
> > request was for data and request was within file size.
> >
> > Also while I'm fixing this and have your attention, Can you tell if
> > the "else if" condition in the original code makes sense to you. I
> > didn't touch it but I don't think it's correct. "else if
> > (seek->seek_pos >= i_size_read(file_inode(nf->nf_file)))" I don't
> > believe this can ever happen. How can vfs_llseek() ever return a
> > position that is greater than the size of the file (or actually even
> > equal to it)?
>
> I agree, I don't get it either.

Any more thoughts about the forward progress of this patch? Are you
interested in taking it?

>
> --b.
>
> >
> > > else if (seek->seek_pos < 0)
> > > status = nfserrno(seek->seek_pos);
> > >
> > > --b.
> > >
> > > >
> > > > Fixes: 24bab491220fa ("NFSD: Implement SEEK")
> > > > Signed-off-by: Olga Kornievskaia <[email protected]>
> > > > ---
> > > > fs/nfsd/nfs4proc.c | 10 +++++++---
> > > > 1 file changed, 7 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > > index e13c4c81fb89..2e7ceb9f1d5d 100644
> > > > --- a/fs/nfsd/nfs4proc.c
> > > > +++ b/fs/nfsd/nfs4proc.c
> > > > @@ -1737,9 +1737,13 @@ nfsd4_seek(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > > > * should ever file->f_pos.
> > > > */
> > > > seek->seek_pos = vfs_llseek(nf->nf_file, seek->seek_offset, whence);
> > > > - if (seek->seek_pos < 0)
> > > > - status = nfserrno(seek->seek_pos);
> > > > - else if (seek->seek_pos >= i_size_read(file_inode(nf->nf_file)))
> > > > + if (seek->seek_pos < 0) {
> > > > + if (whence == SEEK_DATA &&
> > > > + seek->seek_offset < i_size_read(file_inode(nf->nf_file)))
> > > > + seek->seek_eof = true;
> > > > + else
> > > > + status = nfserrno(seek->seek_pos);
> > > > + } else if (seek->seek_pos >= i_size_read(file_inode(nf->nf_file)))
> > > > seek->seek_eof = true;
> > > >
> > > > out:
> > > > --
> > > > 2.18.2
> > > >
> > >
> >
>

2021-04-09 22:26:40

by Frank Filz

[permalink] [raw]
Subject: RE: [PATCH 1/1] NFSD fix handling of NFSv4.2 SEEK for data within the last hole

> Adding [email protected]:
>
> On Thu, Apr 01, 2021 at 09:32:05PM +0000, Rick Macklem wrote:
> > I discussed this on [email protected] some time ago.
> > The problem with "fixing" the server is that it breaks the unpatched
> > Linux client.
> >
> > If the client is careful, it can work correctly for both a Linux and
> > RFC5661 conformant server. That's what the FreeBSD client tries to do.
> > --> I'd suggest that be your main goal.
> >
> > The FreeBSD server ships "Linux compatible", but there is a switch to
> > make it RFC5661 compatible.
> > --> I wouldn't make it default the RFC compatible for
> > quite a while after the client that handles RFC compatible
> > ships.
> >
> > I tried to convince folks to "errata" the RFC to Linux compatible
> > (since Linux was the only shipping 4.2 at the time, but they did not
> > consider it an "errata").
>
> Previous discussion here:
>
> https://mailarchive.ietf.org/arch/msg/nfsv4/bPLFnywt1wZ4kolMkzeSYc
> a0qIM/
>
> There's no rejection on that thread, was it elsewhere?

Yea, it would be nice to resolve this. I remember having to tweak Ganesha's
implementation to not break the Linux client.

I'm still not sure I got it right...

Frank