On Sun, Apr 11, 2021 at 04:43:22PM +0000, Chuck Lever III wrote:
>
>
> > On Apr 9, 2021, at 2:39 PM, Olga Kornievskaia <olga.kornievskaia@xxxxxxxxx> wrote:
> >
> > On Fri, Apr 2, 2021 at 10:32 AM J. Bruce Fields <bfields@xxxxxxxxxx> 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 <bfields@xxxxxxxxxx> wrote:
> >>>>
> >>>> On Wed, Mar 31, 2021 at 03:28:19PM -0400, Olga Kornievskaia wrote:
> >>>>> From: Olga Kornievskaia <kolga@xxxxxxxxxx>
> >>>>>
> >>>>> 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?
>
> Bruce and I discussed this privately a few days back. He asked
> that I not merge it until the client compatibility issue is
> resolved. Bruce, please chime in if I misunderstood you.
Honestly, I haven't even looked at what the issue is. I think Olga
agreed with Rick that there was one, though?
--b.
>
>
> >> --b.
> >>
> >>>
> >>>> else if (seek->seek_pos < 0)
> >>>> status = nfserrno(seek->seek_pos);
> >>>>
> >>>> --b.
> >>>>
> >>>>>
> >>>>> Fixes: 24bab491220fa ("NFSD: Implement SEEK")
> >>>>> Signed-off-by: Olga Kornievskaia <kolga@xxxxxxxxxx>
> >>>>> ---
> >>>>> 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
>
> --
> Chuck Lever
>
>
>
