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:   Chuck Lever III <chuck.lever@oracle.com>
To:     Olga Kornievskaia <olga.kornievskaia@gmail.com>,
        Bruce Fields <bfields@redhat.com>
CC:     Linux NFS Mailing List <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: AQHXJmQFTcgUtIhOdkWOHgA/x/kHY6qe5g2AgADC0QCAAaRLgIALRWIAgAMEPYA=
Date:   Sun, 11 Apr 2021 16:43:22 +0000
Message-ID: <2ACF7423-7162-4880-B8B7-4580208925E5@oracle.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>
 <YGcq7T2wZHJvONHu@pick.fieldses.org>
 <CAN-5tyHBr-H3UjAMHqAQTUPSe_w-wwd4Pqb=WuvkCFfvxnOo_Q@mail.gmail.com>
In-Reply-To: <CAN-5tyHBr-H3UjAMHqAQTUPSe_w-wwd4Pqb=WuvkCFfvxnOo_Q@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <39382824BEEA7046A170B25B97882533@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: e7cca4bf-2f0b-4164-d5d6-08d8fd08eb47
X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Apr 2021 16:43:22.1147
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: HIrr6742mUjG4z0nKXNuSlVj+f8qRarkEgpRIw1Lz+TGnTBLkkGKkeG5vRA3F+Dl4aqPkQUI0e0PHeJzM+GRQQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB3511
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9951 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 mlxlogscore=999
 adultscore=0 phishscore=0 malwarescore=0 mlxscore=0 bulkscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2104060000 definitions=main-2104110130
X-Proofpoint-GUID: KRPFYnQopLF0VUPmCTuUKK43tYY6qFJ2
X-Proofpoint-ORIG-GUID: KRPFYnQopLF0VUPmCTuUKK43tYY6qFJ2
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9951 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 priorityscore=1501
 clxscore=1015 adultscore=0 mlxlogscore=999 impostorscore=0 malwarescore=0
 lowpriorityscore=0 spamscore=0 phishscore=0 bulkscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104060000
 definitions=main-2104110130
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Apr 9, 2021, at 2:39 PM, Olga Kornievskaia <olga.kornievskaia@gmail.co=
m> wrote:
>=20
> On Fri, Apr 2, 2021 at 10:32 AM J. Bruce Fields <bfields@redhat.com> wrot=
e:
>>=20
>> 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> wr=
ote:
>>>>=20
>>>> On Wed, Mar 31, 2021 at 03:28:19PM -0400, Olga Kornievskaia wrote:
>>>>> From: Olga Kornievskaia <kolga@netapp.com>
>>>>>=20
>>>>> 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=3DSEEK_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.
>>>>=20
>>>> Makes sense, but I think you can use the return value from vfs_llseek
>>>> instead of checking the file size again.  E.g.:
>>>>=20
>>>>        seek->seek_pos =3D vfs_llseek(nfs->nf_file, seek->seek_offset, =
whence);
>>>>        if (seek->seek_pos =3D=3D -ENXIO)
>>>>                seek->seek_eof =3D true;
>>>=20
>>> 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.
>>=20
>> OK, never mind.
>>=20
>>> 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.
>>>=20
>>> 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 >=3D 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)?
>>=20
>> I agree, I don't get it either.
>=20
> 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.
>>=20
>>>=20
>>>>        else if (seek->seek_pos < 0)
>>>>                status =3D nfserrno(seek->seek_pos);
>>>>=20
>>>> --b.
>>>>=20
>>>>>=20
>>>>> 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(-)
>>>>>=20
>>>>> 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 nfsd=
4_compound_state *cstate,
>>>>>       *        should ever file->f_pos.
>>>>>       */
>>>>>      seek->seek_pos =3D vfs_llseek(nf->nf_file, seek->seek_offset, wh=
ence);
>>>>> -     if (seek->seek_pos < 0)
>>>>> -             status =3D nfserrno(seek->seek_pos);
>>>>> -     else if (seek->seek_pos >=3D i_size_read(file_inode(nf->nf_file=
)))
>>>>> +     if (seek->seek_pos < 0) {
>>>>> +             if (whence =3D=3D SEEK_DATA &&
>>>>> +                 seek->seek_offset < i_size_read(file_inode(nf->nf_f=
ile)))
>>>>> +                     seek->seek_eof =3D true;
>>>>> +             else
>>>>> +                     status =3D nfserrno(seek->seek_pos);
>>>>> +     } else if (seek->seek_pos >=3D i_size_read(file_inode(nf->nf_fi=
le)))
>>>>>              seek->seek_eof =3D true;
>>>>>=20
>>>>> out:
>>>>> --
>>>>> 2.18.2

--
Chuck Lever