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

[Chuck Lever III <chuck.lever@oracle.com>]

> On Apr 9, 2021, at 2:39 PM, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote:
> 
> On Fri, Apr 2, 2021 at 10:32 AM J. Bruce Fields <bfields@redhat.com> 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@redhat.com> wrote:
>>>> 
>>>> On Wed, Mar 31, 2021 at 03:28:19PM -0400, Olga Kornievskaia wrote:
>>>>> From: Olga Kornievskaia <kolga@netapp.com>
>>>>> 
>>>>> 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.


>> --b.
>> 
>>> 
>>>>        else if (seek->seek_pos < 0)
>>>>                status = nfserrno(seek->seek_pos);
>>>> 
>>>> --b.
>>>> 
>>>>> 
>>>>> Fixes: 24bab491220fa ("NFSD: Implement SEEK")
>>>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>>>>> ---
>>>>> 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