Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Rick Macklem <rmacklem@uoguelph.ca>
To:     Olga Kornievskaia <olga.kornievskaia@gmail.com>,
        "J. Bruce Fields" <bfields@redhat.com>
CC:     Chuck Lever <chuck.lever@oracle.com>,
        linux-nfs <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 1/1] NFSD fix handling of NFSv4.2 SEEK for data within the
 last hole
Thread-Topic: [PATCH 1/1] NFSD fix handling of NFSv4.2 SEEK for data within
 the last hole
Thread-Index: AQHXJyTCyThckhC0u0WVjmsFImLJ2qqgLJWI
Date:   Thu, 1 Apr 2021 21:32:05 +0000
Message-ID: <YQXPR0101MB0968C9AB372DC12408F496D8DD7B9@YQXPR0101MB0968.CANPRD01.PROD.OUTLOOK.COM>
References: <20210331192819.25637-1-olga.kornievskaia@gmail.com>
 <YGUm7/HE3HqVJik2@pick.fieldses.org>,<CAN-5tyETvKvUq_X7+2E0o=9GjJ628DC=QJW5xKA2-X7UHc_DOw@mail.gmail.com>
In-Reply-To: <CAN-5tyETvKvUq_X7+2E0o=9GjJ628DC=QJW5xKA2-X7UHc_DOw@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 36e5f340-54a7-4262-c4dd-08d8f55598d9
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Apr 2021 21:32:05.7789
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: be62a12b-2cad-49a1-a5fa-85f4f3156a7d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: K6QpGBcHLe9mi8hzeTm+LtcePWGuvVHvbtE5qDyUHq9BG9OflMudkBbOk67mtGZS7Gh9q9sp0xK8U85jfMTn9A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: YQXPR01MB3911
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org

I discussed this on nfsv4@ietf.org some time ago.=0A=
The problem with "fixing" the server is that it breaks=0A=
the unpatched Linux client.=0A=
=0A=
If the client is careful, it can work correctly for both a Linux=0A=
and RFC5661 conformant server. That's what the FreeBSD=0A=
client tries to do.=0A=
--> I'd suggest that be your main goal.=0A=
=0A=
The FreeBSD server ships "Linux compatible", but there is=0A=
a switch to make it RFC5661 compatible.=0A=
--> I wouldn't make it default the RFC compatible for=0A=
      quite a while after the client that handles RFC compatible=0A=
      ships.=0A=
=0A=
I tried to convince folks to "errata" the RFC to Linux=0A=
compatible (since Linux was the only shipping 4.2 at=0A=
the time, but they did not consider it an "errata").=0A=
=0A=
Just mho, rick=0A=
=0A=
________________________________________=0A=
From: Olga Kornievskaia <olga.kornievskaia@gmail.com>=0A=
Sent: Thursday, April 1, 2021 9:27 AM=0A=
To: J. Bruce Fields=0A=
Cc: Chuck Lever; linux-nfs=0A=
Subject: Re: [PATCH 1/1] NFSD fix handling of NFSv4.2 SEEK for data within =
the last hole=0A=
=0A=
CAUTION: This email originated from outside of the University of Guelph. Do=
 not click links or open attachments unless you recognize the sender and kn=
ow the content is safe. If in doubt, forward suspicious emails to IThelp@uo=
guelph.ca=0A=
=0A=
=0A=
On Wed, Mar 31, 2021 at 9:50 PM J. Bruce Fields <bfields@redhat.com> wrote:=
=0A=
>=0A=
> On Wed, Mar 31, 2021 at 03:28:19PM -0400, Olga Kornievskaia wrote:=0A=
> > From: Olga Kornievskaia <kolga@netapp.com>=0A=
> >=0A=
> > According to the RFC 7862, "if the server cannot find a=0A=
> > corresponding sa_what, then the status will still be NFS4_OK,=0A=
> > but sr_eof would be TRUE". If there is a file that ends with=0A=
> > a hole and a SEEK request made for sa_what=3DSEEK_DATA with=0A=
> > an offset in the middle of the last hole, then the server=0A=
> > has to return OK and set the eof. Currently the linux server=0A=
> > returns ERR_NXIO.=0A=
>=0A=
> Makes sense, but I think you can use the return value from vfs_llseek=0A=
> instead of checking the file size again.  E.g.:=0A=
>=0A=
>         seek->seek_pos =3D vfs_llseek(nfs->nf_file, seek->seek_offset, wh=
ence);=0A=
>         if (seek->seek_pos =3D=3D -ENXIO)=0A=
>                 seek->seek_eof =3D true;=0A=
=0A=
I don't believe this is correct. (1) ENXIO doesn't imply eof. If the=0A=
specified seek_offset was beyond the end of the file the server must=0A=
return ERR_NXIO and not OK. and (2) for the same reason I need to=0A=
check if the requested type was looking for data but didn't find it=0A=
because the offset is in the middle of the hole but still within the=0A=
file size (thus the need to check if the seek_offset is within the=0A=
file size). But I'm happy to check specifically if the seek_pos was=0A=
ENXIO (and not the generic negative error) and then also check if=0A=
request was for data and request was within file size.=0A=
=0A=
Also while I'm fixing this and have your attention, Can you tell if=0A=
the "else if" condition in the original code makes sense to you. I=0A=
didn't touch it but I don't think it's correct. "else if=0A=
(seek->seek_pos >=3D i_size_read(file_inode(nf->nf_file)))" I don't=0A=
believe this can ever happen. How can vfs_llseek() ever return a=0A=
position that is greater than the size of the file (or actually even=0A=
equal to it)?=0A=
=0A=
>         else if (seek->seek_pos < 0)=0A=
>                 status =3D nfserrno(seek->seek_pos);=0A=
>=0A=
> --b.=0A=
>=0A=
> >=0A=
> > Fixes: 24bab491220fa ("NFSD: Implement SEEK")=0A=
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>=0A=
> > ---=0A=
> >  fs/nfsd/nfs4proc.c | 10 +++++++---=0A=
> >  1 file changed, 7 insertions(+), 3 deletions(-)=0A=
> >=0A=
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c=0A=
> > index e13c4c81fb89..2e7ceb9f1d5d 100644=0A=
> > --- a/fs/nfsd/nfs4proc.c=0A=
> > +++ b/fs/nfsd/nfs4proc.c=0A=
> > @@ -1737,9 +1737,13 @@ nfsd4_seek(struct svc_rqst *rqstp, struct nfsd4_=
compound_state *cstate,=0A=
> >        *        should ever file->f_pos.=0A=
> >        */=0A=
> >       seek->seek_pos =3D vfs_llseek(nf->nf_file, seek->seek_offset, whe=
nce);=0A=
> > -     if (seek->seek_pos < 0)=0A=
> > -             status =3D nfserrno(seek->seek_pos);=0A=
> > -     else if (seek->seek_pos >=3D i_size_read(file_inode(nf->nf_file))=
)=0A=
> > +     if (seek->seek_pos < 0) {=0A=
> > +             if (whence =3D=3D SEEK_DATA &&=0A=
> > +                 seek->seek_offset < i_size_read(file_inode(nf->nf_fil=
e)))=0A=
> > +                     seek->seek_eof =3D true;=0A=
> > +             else=0A=
> > +                     status =3D nfserrno(seek->seek_pos);=0A=
> > +     } else if (seek->seek_pos >=3D i_size_read(file_inode(nf->nf_file=
)))=0A=
> >               seek->seek_eof =3D true;=0A=
> >=0A=
> >  out:=0A=
> > --=0A=
> > 2.18.2=0A=
> >=0A=
>=0A=