Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Trond Myklebust <trondmy@hammerspace.com>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] nfsd: Don't reset the write verifier on a commit EAGAIN
Thread-Topic: [PATCH] nfsd: Don't reset the write verifier on a commit EAGAIN
Thread-Index: AQHZ5ODKf/tGr/TkjUO+olrPJo1n67AWD0yAgAALTICAABU+gIAAGZGAgAARl4A=
Date:   Tue, 12 Sep 2023 00:45:17 +0000
Message-ID: <0B4074FC-C08A-4233-AD9D-FF7C405ADCF2@oracle.com>
References: <20230911184357.11739-1-trond.myklebust@hammerspace.com>
 <ZP91EwHCt0/c0jvJ@tissot.1015granger.net>
 <f754d8a170b967d1523d103837eaeeb5e9a6c85b.camel@hammerspace.com>
 <684AB86D-ADC7-44B0-BA54-FC23DB0B4670@oracle.com>
 <dfc7823c29b2157290828c360e9dc7c64536904b.camel@hammerspace.com>
In-Reply-To: <dfc7823c29b2157290828c360e9dc7c64536904b.camel@hammerspace.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?AXLhWQ0DqENwyboieLWnLV+cjnhOSiTek7VwdSNpSfiZdSnBYx+/7BpIIT0f?=
 =?us-ascii?Q?2uKno+mcHryMbPZmInBDtNT6/yjnjIBaBHFfrp9UJGiNXEfVPudUb4dvaAiT?=
 =?us-ascii?Q?Qs7z3x4I9BzObUPd0XU4woxxK77KKE+l4rPNF9kj4ZjVlWOwCJ1wJwfpW7tG?=
 =?us-ascii?Q?nAjTsOUKL8rrPr9nktpNCO28ztzA6C3fQkSRprqNh9+xwgEfOLFmHtpQ82yw?=
 =?us-ascii?Q?AYjm/miL2AcflvFDeUnnuRFga+QKRZsIA/KLvaMBmL9UNO4uLfxsah6pkFrv?=
 =?us-ascii?Q?iAJJEEAJD2ybcIJLCVfjzCwVX0iadRUF3ONxqjDGAuJdpWU20812LSU1GYI7?=
 =?us-ascii?Q?ry7ANfBsac31ZFx5rhKIti1c6PgqbKPe7ZoAtEjagij2iu1QJbFsglH146WO?=
 =?us-ascii?Q?Skr6Wz3xDEyA5j1s7Nmu7ECtAFFWqei84IEZmaRNEzw7YbhjayK7vMC9d1nC?=
 =?us-ascii?Q?b4qTaEJ9sjVUVexHRVOpld5BEGmy+qo3A6Ca2pd1Z+OnbNC1yfyMqmJlgaqS?=
 =?us-ascii?Q?Ovid2JbyAXnZr4tS+yBzG8/1bhMTCOzPhJrjHLE6yMsQdQfDpucQxqZ7IinD?=
 =?us-ascii?Q?bmWYhCylBfEmWWG6jezeYnaAFjfeeqkrEa42bd9/M7wETFQyBoy7U7zetfEH?=
 =?us-ascii?Q?ClO6VTqgbMc1myUyVacOWYrMZJHFuec9ZovCNXV7ntsKmcTEJQ+0CBEkjAnz?=
 =?us-ascii?Q?BZhmI1IrbHR5/jKVX8M9IQxwP3Y7rW0wBy6+MGXbm8bZnhqFcSdPKCW4zbm4?=
 =?us-ascii?Q?Jz+S3819bDm1JvfwlB5rbVdUgnFzrItf+ZuA6wLmYtC4v6XqmEILY9l5jUSc?=
 =?us-ascii?Q?+LU0JdA3gwsBDQXyoki8pRjqpSi+q062eECxF2nIn3SziMH/FcQf9s/96Uf0?=
 =?us-ascii?Q?T/LyBBnhlC2PUzkvkFfdlIT+amGc4cQUY1NMtLMDCQZxgYiYX9WY8Fz2bVe1?=
 =?us-ascii?Q?Y2/nZKA7KoVqRBURGT9e+4LOhCKIEDeYB2xIoVExOA2PUTBdLv+wfMpAG4o2?=
 =?us-ascii?Q?RDMLn5tFCAPf73i5hKB1GlL46BN58nf0Z5T0knQunT6G/Kt7fEU5EvPmMMlQ?=
 =?us-ascii?Q?cCsapYFhUtxSYJa1fNz7FvLaoTUBDS2LHQY2mWQDu122u51DYCykPfETaBwC?=
 =?us-ascii?Q?Ghz/WXTOyEdV2wmzxn2B6PnGnkMNGgmTtziJlNnU4QwHVFzx9vb37tPQfUqD?=
 =?us-ascii?Q?c5+mB7rGnSPebycHAYsV0pTUL5ds1+VYFBOBeotkDXtyXjFelrH2xnmwsKEW?=
 =?us-ascii?Q?6+0PLauC6Nlea3mnz6W9rHngPOQhsU8Co6VDpnYwgFv8vb+gHYS46ThIObrv?=
 =?us-ascii?Q?F6ajRmVIZp3i8dBveRZH82k7rMUc6vuNzZv23FrSRMNEDBzfnvXLS6gXQ/Tw?=
 =?us-ascii?Q?2V5X/hArR4nezmiFRkm7dFNVaWOd39WDf8nMZ2c3ZfTXPo+WEEDL53YSLW1R?=
 =?us-ascii?Q?mrcx+KC6bmqHW5pB3ehw4aIsIYuePhyfX5jk1zckwH1GEAKSfGdFgoqzTmmT?=
 =?us-ascii?Q?8Cpuh1sm1m+/hyJEec9eBjhCGc5lLnhRq4SOgJzrtLjSl/q1NcbC9n5akU8H?=
 =?us-ascii?Q?2weMiDeeb/WoBjtjnlda9Mp9AM+hrOl3Hs+PTzz5ACbhrHzBe8JVnO06nF5H?=
 =?us-ascii?Q?VQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <5904A73B037D21499D7A271B86B716B8@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: pfIvWozLZZLBFWoKp2MW2Q2VoosrtlWfqsYbdM7RghaTYQXzfmvgk1VnBbjxRSuWDfnyF4IuplNegsR3v1vEOkFZ12Hh5bXSt7CONZsvlKtb94ZobG7j/PyDVjjUfUSzSi/gkkV7Qjos58H3+sdpqXnrfrWiOllOH3Tx7KIIRGE9+geGo3D8FVoYceIEBiWzSLwQukCdvkdG0XYgzUJwY5yaAX7boInUmPZq+ma9J1GKfiaGBRgpnIgs0mFl+3G2mb2j6HngWh7rGZCuOSIHa9gPPXJe4oSNw6nRLrdTGLjA6VOa3icp4KUaLTosC+8m92HTNA08iKrBoZpd+1ByFCsoH/uw84NLA40+U9Swn9A0A2o1+783DBBrZb7EiD3SEAIUQWVG292rjx6/4dib6UUaGD4FHUnSRLew+r+sI9vDPhhamIuX9FwA8GqLaVTSqkygrA9vm/JZsGJWdZgd7ieRkmNHQ383N3zcDIz37/7Rr3RD8KuGRG/H6B5jZWmSh2OcmL6946Nc2E0DN9XeHLfFmkLLxJ0IJ1o0oFo64fHeUFRt95EKXNR1a2+VUCG8INlE8aeN0gMIvpcIetp0TE3lTlVXZPDq2/K42a1H6cJmqXeTN1KbIMF3jFQuO28LaP0VewTE9u+kIqVngzaGq5UkUqtAjqUbW9ymizDKEqL7BCnLrUeY7NMLbAabXTvQsoMnrzMVzDuaMkzbwH3T7oukaP3VUfOA1EMmwbxLgwdIB2pHBxMu1gZ0yyImucLmevNbEtLeU2dCZhmANhJczJ6qVDLfCVLVUp+cO8v3NWs+sHBonySApPgaCZK2wYpc
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: cdf3de90-fa3b-4c1f-b02d-08dbb329893a
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Sep 2023 00:45:17.9984
 (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: VbczoCsOkN0+o3yPKNPVwkdCWgN5zCMOHb0DuQyHVniPwtP81hJygCnbAN3kF0/9wDcNHT6x6TJY6PtiZoOYjg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ1PR10MB6001
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.267,Aquarius:18.0.957,Hydra:6.0.601,FMLib:17.11.176.26
 definitions=2023-09-11_19,2023-09-05_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 adultscore=0
 mlxlogscore=999 phishscore=0 malwarescore=0 mlxscore=0 spamscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2308100000 definitions=main-2309120005
X-Proofpoint-ORIG-GUID: ALLTn3XyCxHj_nRIVJT_eEhHs2XuHXd8
X-Proofpoint-GUID: ALLTn3XyCxHj_nRIVJT_eEhHs2XuHXd8
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Mon, 11 Sep 2023 21:14:29 -0700 (PDT)


> On Sep 11, 2023, at 7:42 PM, Trond Myklebust <trondmy@hammerspace.com> wr=
ote:
>=20
> On Mon, 2023-09-11 at 22:10 +0000, Chuck Lever III wrote:
>>=20
>>> On Sep 11, 2023, at 4:54 PM, Trond Myklebust
>>> <trondmy@hammerspace.com> wrote:
>>>=20
>>> On Mon, 2023-09-11 at 16:14 -0400, Chuck Lever wrote:
>>>> On Mon, Sep 11, 2023 at 02:43:57PM -0400,
>>>> trondmy@gmail.com wrote:
>>>>> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>>>>>=20
>>>>> If fsync() is returning EAGAIN, then we can assume that the
>>>>> filesystem
>>>>> being exported is something like NFS with the 'softerr' mount
>>>>> option
>>>>> enabled, and that it is just asking us to replay the fsync()
>>>>> operation
>>>>> at a later date.
>>>>> If we see an ESTALE, then ditto: the file is gone, so there is
>>>>> no
>>>>> danger
>>>>> of losing the error.
>>>>> For those cases, do not reset the write verifier.
>>>>=20
>>>> Out of interest, what's the hazard in a write verifier change in
>>>> these cases? There could be a slight performance penalty, I
>>>> imagine,
>>>> but how frequently does this happen?
>>>=20
>>> When re-exporting to NFSv4 clients, it should be less of a problem,
>>> since any REMOVE will result in a sillyrenamed file that only
>>> disappears once the file is closed. However with NFSv3 clients,
>>> that is
>>> circumvented by the fact that the filecache closes the files when
>>> they
>>> are inactive. We've seen this occur frequently with VMware vmdks:
>>> their
>>> lock files appear to generate a lot of these phantom ESTALE writes.
>>>=20
>>> As for EAGAIN, I just pushed out a 2 patch client series that makes
>>> it
>>> a lot more frequent when re-exporting NFSv4 with 'softerr'.
>>>=20
>>> Finally, it is worth noting that a write verifier change has a
>>> global
>>> effect, causing retransmission by all clients of all uncommitted
>>> unstable writes for all files, so is worth mitigating where
>>> possible.
>>=20
>> Good info. I've added some of this to the patch description.
>>=20
>>=20
>>>> One more below.
>>>>=20
>>>>=20
>>>>> Signed-off-by: Trond Myklebust
>>>>> <trond.myklebust@hammerspace.com>
>>>>> ---
>>>>>  fs/nfsd/vfs.c | 29 +++++++++++++++++++----------
>>>>>  1 file changed, 19 insertions(+), 10 deletions(-)
>>>>>=20
>>>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>>>> index 98fa4fd0556d..31daf9f63572 100644
>>>>> --- a/fs/nfsd/vfs.c
>>>>> +++ b/fs/nfsd/vfs.c
>>>>> @@ -337,6 +337,20 @@ nfsd_lookup(struct svc_rqst *rqstp, struct
>>>>> svc_fh *fhp, const char *name,
>>>>>         return err;
>>>>>  }
>>>>> =20
>>>>> +static void
>>>>> +commit_reset_write_verifier(struct nfsd_net *nn, struct
>>>>> svc_rqst
>>>>> *rqstp,
>>>>> +                           int err)
>>>>> +{
>>>>> +       switch (err) {
>>>>> +       case -EAGAIN:
>>>>> +       case -ESTALE:
>>>>> +               break;
>>>>> +       default:
>>>>> +               nfsd_reset_write_verifier(nn);
>>>>> +               trace_nfsd_writeverf_reset(nn, rqstp, err);
>>>>> +       }
>>>>> +}
>>>>> +
>>>>>  /*
>>>>>   * Commit metadata changes to stable storage.
>>>>>   */
>>>>> @@ -647,8 +661,7 @@ __be32 nfsd4_clone_file_range(struct
>>>>> svc_rqst
>>>>> *rqstp,
>>>>>                                       =20
>>>>> &nfsd4_get_cstate(rqstp)-
>>>>>> current_fh,
>>>>>                                         dst_pos,
>>>>>                                         count, status);
>>>>> -                       nfsd_reset_write_verifier(nn);
>>>>> -                       trace_nfsd_writeverf_reset(nn, rqstp,
>>>>> status);
>>>>> +                       commit_reset_write_verifier(nn, rqstp,
>>>>> status);
>>>>>                         ret =3D nfserrno(status);
>>>>>                 }
>>>>>         }
>>>>> @@ -1170,8 +1183,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp,
>>>>> struct
>>>>> svc_fh *fhp, struct nfsd_file *nf,
>>>>>         host_err =3D vfs_iter_write(file, &iter, &pos, flags);
>>>>>         file_end_write(file);
>>>>>         if (host_err < 0) {
>>>>> -               nfsd_reset_write_verifier(nn);
>>>>> -               trace_nfsd_writeverf_reset(nn, rqstp,
>>>>> host_err);
>>>>> +               commit_reset_write_verifier(nn, rqstp,
>>>>> host_err);
>>>>=20
>>>> Can generic_file_write_iter() or its brethren return STALE or
>>>> AGAIN
>>>> before they get to the generic_write_sync() call ?
>>>=20
>>> The call to nfs_revalidate_file_size(), which can occur when you
>>> are
>>> appending to the file (whether or not O_APPEND is set) could indeed
>>> return ESTALE.
>>> With the new patchset mentioned above, it could also return EAGAIN.
>>=20
>> Sounds like I should drop this hunk when applying this fix.
>=20
> I'm not understanding. Why would you not keep it?

generic_file_write_iter() and its brethren are two calls in
one, if I'm following this correctly:

1. write
2. sync

All the other places you change are "sync" only, so it's
fairly obvious that those callers get a return code that
reflects a failure of "sync".

I asked above if it's possible for the "write" part of
generic_file_write_iter() to fail with STALE/AGAIN before the
sync part is even called.

You seemed to be answering "yes, the 'write' part can fail
that way" but I may have misunderstood your response.

If the "write" step can fail, isn't that something that should
be reflected in a write verifier change? If yes, I don't see
how this particular call site can distinguish between a "write"
failure versus a "sync" failure.

Or, if the vfs_iter_write() call here is guaranteed to never
be a sync write request, then again, I think we want to reflect
all failures here with a write verifier change.

However, if STALE and AGAIN have the exact same semantics
for "write" as they do for "sync", those failures can be
thrown away too, and I can keep this hunk. Are you saying
this is the case?

(this is /only/ for the vfs_iter_write() call site. The others
look OK to me).


>>>>>                 goto out_nfserr;
>>>>>         }
>>>>>         *cnt =3D host_err;
>>>>> @@ -1183,10 +1195,8 @@ nfsd_vfs_write(struct svc_rqst *rqstp,
>>>>> struct svc_fh *fhp, struct nfsd_file *nf,
>>>>> =20
>>>>>         if (stable && use_wgather) {
>>>>>                 host_err =3D wait_for_concurrent_writes(file);
>>>>> -               if (host_err < 0) {
>>>>> -                       nfsd_reset_write_verifier(nn);
>>>>> -                       trace_nfsd_writeverf_reset(nn, rqstp,
>>>>> host_err);
>>>>> -               }
>>>>> +               if (host_err < 0)
>>>>> +                       commit_reset_write_verifier(nn, rqstp,
>>>>> host_err);
>>>>>         }
>>>>> =20
>>>>>  out_nfserr:
>>>>> @@ -1329,8 +1339,7 @@ nfsd_commit(struct svc_rqst *rqstp,
>>>>> struct
>>>>> svc_fh *fhp, struct nfsd_file *nf,
>>>>>                         err =3D nfserr_notsupp;
>>>>>                         break;
>>>>>                 default:
>>>>> -                       nfsd_reset_write_verifier(nn);
>>>>> -                       trace_nfsd_writeverf_reset(nn, rqstp,
>>>>> err2);
>>>>> +                       commit_reset_write_verifier(nn, rqstp,
>>>>> err2);
>>>>>                         err =3D nfserrno(err2);
>>>>>                 }
>>>>>         } else
>>>>> --=20
>>>>> 2.41.0
>>>>>=20
>>>>=20
>>>=20
>>> --=20
>>> Trond Myklebust
>>> Linux NFS client maintainer, Hammerspace
>>> trond.myklebust@hammerspace.com
>>=20
>>=20
>> --
>> Chuck Lever
>>=20
>>=20
>=20
> --=20
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>=20
>=20

--
Chuck Lever