diff mbox series

[1/1] NFSv4.2 fix handling of sr_eof in SEEK's reply

Message ID 20210331193025.25724-1-olga.kornievskaia@gmail.com (mailing list archive)
State New
Headers show
Series [1/1] NFSv4.2 fix handling of sr_eof in SEEK's reply | expand

Commit Message

Olga Kornievskaia March 31, 2021, 7:30 p.m. UTC
From: Olga Kornievskaia <kolga@netapp.com>

Currently the client ignores the value of the sr_eof of the SEEK
operation. According to the spec, if the server didn't find the
requested extent and reached the end of the file, the server
would return sr_eof=true. In case the request for DATA and no
data was found (ie in the middle of the hole), then the lseek
expects that ENXIO would be returned.

Fixes: 1c6dcbe5ceff8 ("NFS: Implement SEEK")
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfs/nfs42proc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Trond Myklebust March 31, 2021, 7:42 p.m. UTC | #1
On Wed, 2021-03-31 at 15:30 -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> Currently the client ignores the value of the sr_eof of the SEEK
> operation. According to the spec, if the server didn't find the
> requested extent and reached the end of the file, the server
> would return sr_eof=true. In case the request for DATA and no
> data was found (ie in the middle of the hole), then the lseek
> expects that ENXIO would be returned.
> 
> Fixes: 1c6dcbe5ceff8 ("NFS: Implement SEEK")
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfs/nfs42proc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> index 094024b0aca1..d359e712c11d 100644
> --- a/fs/nfs/nfs42proc.c
> +++ b/fs/nfs/nfs42proc.c
> @@ -659,7 +659,10 @@ static loff_t _nfs42_proc_llseek(struct file
> *filep,
>         if (status)
>                 return status;
>  
> -       return vfs_setpos(filep, res.sr_offset, inode->i_sb-
> >s_maxbytes);
> +       if (whence == SEEK_DATA && res.sr_eof)
> +               return -NFS4ERR_NXIO;
> +       else
> +               return vfs_setpos(filep, res.sr_offset, inode->i_sb-
> >s_maxbytes);
>  }
>  
>  loff_t nfs42_proc_llseek(struct file *filep, loff_t offset, int
> whence)

Don't we also need to deal with SEEK_HOLE with the offset being greater
than the end-of-file in the same way?
Olga Kornievskaia March 31, 2021, 8:34 p.m. UTC | #2
On Wed, Mar 31, 2021 at 3:42 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> On Wed, 2021-03-31 at 15:30 -0400, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > Currently the client ignores the value of the sr_eof of the SEEK
> > operation. According to the spec, if the server didn't find the
> > requested extent and reached the end of the file, the server
> > would return sr_eof=true. In case the request for DATA and no
> > data was found (ie in the middle of the hole), then the lseek
> > expects that ENXIO would be returned.
> >
> > Fixes: 1c6dcbe5ceff8 ("NFS: Implement SEEK")
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  fs/nfs/nfs42proc.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> > index 094024b0aca1..d359e712c11d 100644
> > --- a/fs/nfs/nfs42proc.c
> > +++ b/fs/nfs/nfs42proc.c
> > @@ -659,7 +659,10 @@ static loff_t _nfs42_proc_llseek(struct file
> > *filep,
> >         if (status)
> >                 return status;
> >
> > -       return vfs_setpos(filep, res.sr_offset, inode->i_sb-
> > >s_maxbytes);
> > +       if (whence == SEEK_DATA && res.sr_eof)
> > +               return -NFS4ERR_NXIO;
> > +       else
> > +               return vfs_setpos(filep, res.sr_offset, inode->i_sb-
> > >s_maxbytes);
> >  }
> >
> >  loff_t nfs42_proc_llseek(struct file *filep, loff_t offset, int
> > whence)
>
> Don't we also need to deal with SEEK_HOLE with the offset being greater
> than the end-of-file in the same way?

We do not because if there is no hole extent after the requested
offset, then there is an implied hole which is at the end of the file.
So if sr_eof is true we still need to pay attention to the returned
offset (ie it should be end of the file) and it's not an error
condition.

>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>
Trond Myklebust March 31, 2021, 9:07 p.m. UTC | #3
On Wed, 2021-03-31 at 16:34 -0400, Olga Kornievskaia wrote:
> On Wed, Mar 31, 2021 at 3:42 PM Trond Myklebust
> <trondmy@hammerspace.com> wrote:
> > 
> > On Wed, 2021-03-31 at 15:30 -0400, Olga Kornievskaia wrote:
> > > From: Olga Kornievskaia <kolga@netapp.com>
> > > 
> > > Currently the client ignores the value of the sr_eof of the SEEK
> > > operation. According to the spec, if the server didn't find the
> > > requested extent and reached the end of the file, the server
> > > would return sr_eof=true. In case the request for DATA and no
> > > data was found (ie in the middle of the hole), then the lseek
> > > expects that ENXIO would be returned.
> > > 
> > > Fixes: 1c6dcbe5ceff8 ("NFS: Implement SEEK")
> > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > > ---
> > >  fs/nfs/nfs42proc.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> > > index 094024b0aca1..d359e712c11d 100644
> > > --- a/fs/nfs/nfs42proc.c
> > > +++ b/fs/nfs/nfs42proc.c
> > > @@ -659,7 +659,10 @@ static loff_t _nfs42_proc_llseek(struct file
> > > *filep,
> > >         if (status)
> > >                 return status;
> > > 
> > > -       return vfs_setpos(filep, res.sr_offset, inode->i_sb-
> > > > s_maxbytes);
> > > +       if (whence == SEEK_DATA && res.sr_eof)
> > > +               return -NFS4ERR_NXIO;
> > > +       else
> > > +               return vfs_setpos(filep, res.sr_offset, inode-
> > > >i_sb-
> > > > s_maxbytes);
> > >  }
> > > 
> > >  loff_t nfs42_proc_llseek(struct file *filep, loff_t offset, int
> > > whence)
> > 
> > Don't we also need to deal with SEEK_HOLE with the offset being
> > greater
> > than the end-of-file in the same way?
> 
> We do not because if there is no hole extent after the requested
> offset, then there is an implied hole which is at the end of the file.
> So if sr_eof is true we still need to pay attention to the returned
> offset (ie it should be end of the file) and it's not an error
> condition.

I'm asking because according to the manpage for lseek():

       ENXIO  whence is SEEK_DATA or SEEK_HOLE, and offset is beyond  the  end
              of  the file, or whence is SEEK_DATA and offset is within a hole
              at the end of the file.

i.e. the manpage implies that we should also be returning an error for
SEEK_HOLE if the supplied offset is beyond eof. Are you saying that we
currently do so?
Olga Kornievskaia March 31, 2021, 9:25 p.m. UTC | #4
On Wed, Mar 31, 2021 at 5:07 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> On Wed, 2021-03-31 at 16:34 -0400, Olga Kornievskaia wrote:
> > On Wed, Mar 31, 2021 at 3:42 PM Trond Myklebust
> > <trondmy@hammerspace.com> wrote:
> > >
> > > On Wed, 2021-03-31 at 15:30 -0400, Olga Kornievskaia wrote:
> > > > From: Olga Kornievskaia <kolga@netapp.com>
> > > >
> > > > Currently the client ignores the value of the sr_eof of the SEEK
> > > > operation. According to the spec, if the server didn't find the
> > > > requested extent and reached the end of the file, the server
> > > > would return sr_eof=true. In case the request for DATA and no
> > > > data was found (ie in the middle of the hole), then the lseek
> > > > expects that ENXIO would be returned.
> > > >
> > > > Fixes: 1c6dcbe5ceff8 ("NFS: Implement SEEK")
> > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > > > ---
> > > >  fs/nfs/nfs42proc.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> > > > index 094024b0aca1..d359e712c11d 100644
> > > > --- a/fs/nfs/nfs42proc.c
> > > > +++ b/fs/nfs/nfs42proc.c
> > > > @@ -659,7 +659,10 @@ static loff_t _nfs42_proc_llseek(struct file
> > > > *filep,
> > > >         if (status)
> > > >                 return status;
> > > >
> > > > -       return vfs_setpos(filep, res.sr_offset, inode->i_sb-
> > > > > s_maxbytes);
> > > > +       if (whence == SEEK_DATA && res.sr_eof)
> > > > +               return -NFS4ERR_NXIO;
> > > > +       else
> > > > +               return vfs_setpos(filep, res.sr_offset, inode-
> > > > >i_sb-
> > > > > s_maxbytes);
> > > >  }
> > > >
> > > >  loff_t nfs42_proc_llseek(struct file *filep, loff_t offset, int
> > > > whence)
> > >
> > > Don't we also need to deal with SEEK_HOLE with the offset being
> > > greater
> > > than the end-of-file in the same way?
> >
> > We do not because if there is no hole extent after the requested
> > offset, then there is an implied hole which is at the end of the file.
> > So if sr_eof is true we still need to pay attention to the returned
> > offset (ie it should be end of the file) and it's not an error
> > condition.
>
> I'm asking because according to the manpage for lseek():
>
>        ENXIO  whence is SEEK_DATA or SEEK_HOLE, and offset is beyond  the  end
>               of  the file, or whence is SEEK_DATA and offset is within a hole
>               at the end of the file.
>
> i.e. the manpage implies that we should also be returning an error for
> SEEK_HOLE if the supplied offset is beyond eof. Are you saying that we
> currently do so?

Yes the server should do that. Client doesn't need to do anything
extra. This patch is to handle the discrepancy between what the
NFSv4.2 spec says and what the linux lseek expects (specifically the
last condition is what linux expects but spec says it has to be a
non-error condition).

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

Patch

diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
index 094024b0aca1..d359e712c11d 100644
--- a/fs/nfs/nfs42proc.c
+++ b/fs/nfs/nfs42proc.c
@@ -659,7 +659,10 @@  static loff_t _nfs42_proc_llseek(struct file *filep,
 	if (status)
 		return status;
 
-	return vfs_setpos(filep, res.sr_offset, inode->i_sb->s_maxbytes);
+	if (whence == SEEK_DATA && res.sr_eof)
+		return -NFS4ERR_NXIO;
+	else
+		return vfs_setpos(filep, res.sr_offset, inode->i_sb->s_maxbytes);
 }
 
 loff_t nfs42_proc_llseek(struct file *filep, loff_t offset, int whence)