Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Trond Myklebust <trondmy@hammerspace.com>
To:     Olga Kornievskaia <aglo@umich.edu>
CC:     Neil Brown <neilb@suse.de>, Trond Myklebust <trondmy@kernel.org>,
        Anna Schumaker <anna@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] NFS: Handle missing attributes in OPEN reply
Thread-Topic: [PATCH] NFS: Handle missing attributes in OPEN reply
Thread-Index: AQHZH9NUhloWZMaUakmHx9w8Uho3Qa6NbEuAgAAEOoCAAAPtAIAAD/4AgALtsYCAAExXgIAABW8AgAV8BgCAAAPIAA==
Date:   Mon, 9 Jan 2023 15:47:40 +0000
Message-ID: <F3FEC918-730E-415A-AE9B-A713B0718015@hammerspace.com>
References: <167279203612.13974.15063003557908413815@noble.neil.brown.name>
 <7a98c3e70bae70c44418ce8ac4b84f387b4ff850.camel@kernel.org>
 <167279409373.13974.16504090316814568327@noble.neil.brown.name>
 <210f08ae5f0ba47c289293981f490fca848dd2ed.camel@kernel.org>
 <167279837032.13974.12155714770736077636@noble.neil.brown.name>
 <167295936597.13974.7568769884598065471@noble.neil.brown.name>
 <46f047159da42dadaca576e0a87a622539609730.camel@kernel.org>
 <167297692611.13974.5805041718280562542@noble.neil.brown.name>
 <CAN-5tyGRkKB-=9-HFXkDSu+--ghBNEfmfXO8yD7=2bo=aH4fhA@mail.gmail.com>
In-Reply-To: <CAN-5tyGRkKB-=9-HFXkDSu+--ghBNEfmfXO8yD7=2bo=aH4fhA@mail.gmail.com>
Accept-Language: en-US, en-GB
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?VbQyS2/XrCAuzXVmM0mAWRPj5ZDH8Kw8C6nryjVtmymYRZvnEj8zQrU6TdaJ?=
 =?us-ascii?Q?4riC/sh8eYeyUfAmj3yiojDkk99fLdBy35nuOPFIus+XDN0wGEvIn8x9aW7L?=
 =?us-ascii?Q?nGRfcD3eh3RuUrRbW6Gvp4TRXZAKBsbaieMTO+xmZ6jcdvwTSLLltNXhkIW/?=
 =?us-ascii?Q?HTEDNL2HhyidFqmlyJqaTPJk3OqBIcSDoAY+Vb3f0WYY9OS7JR34hQowBmJw?=
 =?us-ascii?Q?wcVMhrCe3UqEFo4SrnIACylkMZ5MLaVCYQNjOxQi+lbBHTA2wbPcNktPQTrV?=
 =?us-ascii?Q?nSJlQ8CNV5Tt77srIUiXBOYEwRlcOtv0TSjk9FWdAUdT7nVTfDfxZpPn/dhy?=
 =?us-ascii?Q?qNupF8z/NSPVvkN8h7B8RUTHdV/btUfIAfZJhzmLY2xx7uLJW6c6dG4rz1pW?=
 =?us-ascii?Q?KMifWTGkGsyossaubl0kgS1HeXEWnfgoF/pOneDOTiTfaVDKquxl3VwjjbPJ?=
 =?us-ascii?Q?6y6XAdzmmxkXhUoCCJVLXjRbnvRhq5AzKlaroEwVjCo+Y718SVMUdaAp5ey5?=
 =?us-ascii?Q?/JdvN7dAfkd6GAnn1y/34orNL6BatKqBnxLwplaZS4/VQ5kwc5zpjiG6lfBS?=
 =?us-ascii?Q?oOVlLCmouk0OO8SyhBPgKGif1+qHULeSj3Dg3zDgdsHeXRosx5/UsTMUaemK?=
 =?us-ascii?Q?8qYnL7ARo27A0bb09OauSgyuyFQGxyI6hLEvqemLC9DqcCFfDCwtu/f6Qfac?=
 =?us-ascii?Q?ZO5qyLi7RGI4ianC/pbYoYsix2Yk9WxHGLhUreFQ+ZneeeYOUzgK+IC/Pjxx?=
 =?us-ascii?Q?DgRN9txjTtrNwROSaicWu4J2uKiUDBMLavYlcc7Bkc3auhfYf2sZoEGVl3yE?=
 =?us-ascii?Q?QduAi30qVFU6avJR6wTwBahuqMUxiSHdGuYmdYim5vGiJio8/nou8G0SZnod?=
 =?us-ascii?Q?ldE6gvRPf7Tvm/AVLr8/kdX6y3sJZDY7Qv5/o9DHLUNiYfEJ/+tdBCkSixO1?=
 =?us-ascii?Q?no0tOxdihnsACER1TEoRR8yZ+VuP+zxUAODUMbYK7kMKRZ+RxovJwQOXp+I+?=
 =?us-ascii?Q?2Mt7WA4SppT3DZB9T7FYX9mPRtVUFedWjVk88gwFZjrD2KOGuSuiQ+a9rA0g?=
 =?us-ascii?Q?iflQbBYSrQdkIFj6FMz4uPdfNxhwpW+cM2jBZF/71Tbc/F+4n5/kPGolNwJ8?=
 =?us-ascii?Q?T7s0ZAiGCyZeSzNd+/ndXcN7r7WcXX1QDyrAIuFFi3QJFdVOot9Hq/0ZtO6E?=
 =?us-ascii?Q?SSqjpCl9S6/eUU70+cctoON/SOEJBLHHtAYWL0dRQAtjeufzLgb9XjbwnmZJ?=
 =?us-ascii?Q?6pGIbtrhUIzYM+Zqs8fl6IAsEe8Bpy2Z/TiDwvy+1G7Ya04nehUcJ8ckqY/T?=
 =?us-ascii?Q?NNNpwR+bg3gw6RbHf0qoIyg4yHG7T52hau1TxKXP82QckSfrEHsF16MWw4ZK?=
 =?us-ascii?Q?hTt2ArSrIeg1XAU+wi4SWcHi3OJY2uEv0Yh/XFCHST4G9M1nUC0/gCc79hBZ?=
 =?us-ascii?Q?oSPX5roDdw5uCX4HlGHwZBMqNkRVex/FzXfwwXQSi4slQrNznEO8U4oLFaLT?=
 =?us-ascii?Q?IsTlf8DAzCdOgt4kREZYokFXpiL7m6xVqrqD9cCAaU/LZFHFVHqgTEG+22AN?=
 =?us-ascii?Q?U6j3VMZ7yxWKxOnyb64Y+jYtK5YncloDgLDyEhE9bbcn5XEXF/quQV+d3O5l?=
 =?us-ascii?Q?/A=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <07636A7A17AE9C43BD716457814C8627@namprd13.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: hammerspace.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CH0PR13MB5084.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 861b6e64-4df9-4015-08c8-08daf258d731
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Jan 2023 15:47:40.7080
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 0d4fed5c-3a70-46fe-9430-ece41741f59e
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: TV9muHluHSbqGBi34lYNMg61iHlfZMfO/lfRBiZZXqaM9+tsPLpaRKxupuNQYVDHZ12QkYsmZXiQrADRIWdf5g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL0PR13MB4417
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Jan 9, 2023, at 10:33, Olga Kornievskaia <aglo@umich.edu> wrote:
>=20
> On Thu, Jan 5, 2023 at 10:48 PM NeilBrown <neilb@suse.de> wrote:
>>=20
>> On Fri, 06 Jan 2023, Trond Myklebust wrote:
>>> On Fri, 2023-01-06 at 09:56 +1100, NeilBrown wrote:
>>>> On Wed, 04 Jan 2023, NeilBrown wrote:
>>>>> On Wed, 04 Jan 2023, Trond Myklebust wrote:
>>>>>> On Wed, 2023-01-04 at 12:01 +1100, NeilBrown wrote:
>>>>>>> On Wed, 04 Jan 2023, Trond Myklebust wrote:
>>>>>>>>=20
>>>>>>>>=20
>>>>>>>> If the server starts to reply NFS4ERR_STALE to GETATTR
>>>>>>>> requests,
>>>>>>>> why do
>>>>>>>> we care about stateid values? Just mark the inode as stale
>>>>>>>> and drop
>>>>>>>> it
>>>>>>>> on the floor.
>>>>>>>=20
>>>>>>> We have a valid state from the server - we really shouldn't
>>>>>>> just
>>>>>>> ignore
>>>>>>> it.
>>>>>>>=20
>>>>>>> Maybe it would be OK to mark the inode stale.  I don't know if
>>>>>>> various
>>>>>>> retry loops abort properly when the inode is stale.
>>>>>>=20
>>>>>> Yes, they are all supposed to do that. Otherwise we would end up
>>>>>> looping forever in close(), for instance, since the PUTFH,
>>>>>> GETATTR and
>>>>>> CLOSE can all return NFS4ERR_STALE as well.
>>>>>=20
>>>>> To mark the inode as STALE we still need to find the inode, and
>>>>> that is
>>>>> the key bit missing in the current code.  Once we find the inode,
>>>>> we
>>>>> could mark it stale, but maybe some other error resulted in the
>>>>> missing
>>>>> GETATTR...
>>>>>=20
>>>>> It might make sense to put the new code in _nfs4_proc_open() after
>>>>> the
>>>>> explicit nfs4_proc_getattr() fails.  We would need to find the
>>>>> inode
>>>>> given only the filehandle.  Is there any easy way to do that?
>>>>>=20
>>>>> Thanks,
>>>>> NeilBrown
>>>>>=20
>>>>=20
>>>> I couldn't see a consistent pattern to follow for when to mark an
>>>> inode
>>>> as stale.  Do this, on top of the previous patch, seem reasonable?
>>>>=20
>>>> Thanks,
>>>> NeilBrown
>>>>=20
>>>>=20
>>>>=20
>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>> index b441b1d14a50..04497cb42154 100644
>>>> --- a/fs/nfs/nfs4proc.c
>>>> +++ b/fs/nfs/nfs4proc.c
>>>> @@ -489,6 +489,8 @@ static int nfs4_do_handle_exception(struct
>>>> nfs_server *server,
>>>>                case -ESTALE:
>>>>                        if (inode !=3D NULL && S_ISREG(inode->i_mode))
>>>>                                pnfs_destroy_layout(NFS_I(inode));
>>>> +                       if (inode)
>>>> +                               nfs_set_inode_stale(inode);
>>>=20
>>> This is normally dealt with in the generic code inside
>>> nfs_revalidate_inode(). There should be no need to duplicate it here.
>>>=20
>>>>                        break;
>>>>                case -NFS4ERR_DELEG_REVOKED:
>>>>                case -NFS4ERR_ADMIN_REVOKED:
>>>> @@ -2713,8 +2715,12 @@ static int _nfs4_proc_open(struct
>>>> nfs4_opendata *data,
>>>>                        return status;
>>>>        }
>>>>        if (!(o_res->f_attr->valid & NFS_ATTR_FATTR)) {
>>>> +               struct inode *inode =3D nfs4_get_inode_by_stateid(
>>>> +                       &data->o_res.stateid,
>>>> +                       data->owner);
>>>=20
>>> There shouldn't be a need to go looking for open descriptors here,
>>> because they will hit ESTALE at some point later anyway.
>>=20
>> The problem is that they don't hit ESTALE later.  Unless we update our
>> stored stateid with the result of the OPEN, we can use the old stateid,
>> and get the corresponding error.
>>=20
>> The only way to avoid the infinite loop is to either mark the inode as
>> stale, or update the state-id.  For either of those we need to find the
>> inode.  We don't have fileid so we cannot use iget.  We do have file
>> handle and state-id.
>>=20
>> Maybe you are saying this is a server bug that the client cannot be
>> expect to cope with at all, and that an infinite loop is a valid client
>> response to this particular server behaviour.  In that case, I guess no
>> patch is needed.
>=20
> I'm not arguing that the server should do something else. But I would
> like to talk about it from the spec perspective. When PUTFH+WRITE is
> sent to the server (with an incorrect stateid) and it's processing the
> WRITE compound (as the spec doesn't require the server to validate a
> filehandle on PUTFH. The spec says PUTFH is to "set" the correct
> filehandle), the server is dealing with 2 errors that it can possibly
> return to the client ERR_STALE and ERR_OLD_STATEID. There is nothing
> in the spec that speaks to the orders of errors to be returned. Of
> course I'd like to say that the server should prioritize ERR_STALE
> over ERR_OLD_STATEID (simply because it's a MUST in the spec and
> ERR_OLD_STATEIDs are written as "rules").
>=20

I disagree for the reason already pointed to in the spec. There is nothing =
in the spec that appears to allow the PUTFH to return anything other than N=
FS4ERR_STALE after the file has been deleted (and yes, RFC5661, Section 15.=
2 does list NFS4ERR_STALE as an error for PUTFH). PUTFH is definitely requi=
red to validate the file handle, since it is the ONLY operation that can re=
turn NFS4ERR_BADHANDLE.

_________________________________
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com