* [PATCH] nfsd: Don't reset the write verifier on a commit EAGAIN @ 2023-09-11 18:43 trondmy 2023-09-11 20:14 ` Chuck Lever 2023-09-12 10:54 ` Jeff Layton 0 siblings, 2 replies; 9+ messages in thread From: trondmy @ 2023-09-11 18:43 UTC (permalink / raw) To: Chuck Lever; +Cc: linux-nfs From: Trond Myklebust <trond.myklebust@hammerspace.com> 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. Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> --- fs/nfsd/vfs.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) 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; } +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, &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 = nfserrno(status); } } @@ -1170,8 +1183,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf, host_err = 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); goto out_nfserr; } *cnt = host_err; @@ -1183,10 +1195,8 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf, if (stable && use_wgather) { host_err = 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); } out_nfserr: @@ -1329,8 +1339,7 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf, err = nfserr_notsupp; break; default: - nfsd_reset_write_verifier(nn); - trace_nfsd_writeverf_reset(nn, rqstp, err2); + commit_reset_write_verifier(nn, rqstp, err2); err = nfserrno(err2); } } else -- 2.41.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] nfsd: Don't reset the write verifier on a commit EAGAIN 2023-09-11 18:43 [PATCH] nfsd: Don't reset the write verifier on a commit EAGAIN trondmy @ 2023-09-11 20:14 ` Chuck Lever 2023-09-11 20:54 ` Trond Myklebust 2023-09-12 10:54 ` Jeff Layton 1 sibling, 1 reply; 9+ messages in thread From: Chuck Lever @ 2023-09-11 20:14 UTC (permalink / raw) To: trondmy; +Cc: linux-nfs On Mon, Sep 11, 2023 at 02:43:57PM -0400, trondmy@gmail.com wrote: > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > 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. 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? One more below. > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > --- > fs/nfsd/vfs.c | 29 +++++++++++++++++++---------- > 1 file changed, 19 insertions(+), 10 deletions(-) > > 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; > } > > +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, > &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 = nfserrno(status); > } > } > @@ -1170,8 +1183,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf, > host_err = 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); Can generic_file_write_iter() or its brethren return STALE or AGAIN before they get to the generic_write_sync() call ? > goto out_nfserr; > } > *cnt = host_err; > @@ -1183,10 +1195,8 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf, > > if (stable && use_wgather) { > host_err = 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); > } > > out_nfserr: > @@ -1329,8 +1339,7 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf, > err = nfserr_notsupp; > break; > default: > - nfsd_reset_write_verifier(nn); > - trace_nfsd_writeverf_reset(nn, rqstp, err2); > + commit_reset_write_verifier(nn, rqstp, err2); > err = nfserrno(err2); > } > } else > -- > 2.41.0 > -- Chuck Lever ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfsd: Don't reset the write verifier on a commit EAGAIN 2023-09-11 20:14 ` Chuck Lever @ 2023-09-11 20:54 ` Trond Myklebust 2023-09-11 22:10 ` Chuck Lever III 0 siblings, 1 reply; 9+ messages in thread From: Trond Myklebust @ 2023-09-11 20:54 UTC (permalink / raw) To: chuck.lever@oracle.com; +Cc: linux-nfs@vger.kernel.org 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> > > > > 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. > > 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? 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. 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'. 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. > > One more below. > > > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > > --- > > fs/nfsd/vfs.c | 29 +++++++++++++++++++---------- > > 1 file changed, 19 insertions(+), 10 deletions(-) > > > > 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; > > } > > > > +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, > > &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 = nfserrno(status); > > } > > } > > @@ -1170,8 +1183,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct > > svc_fh *fhp, struct nfsd_file *nf, > > host_err = 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); > > Can generic_file_write_iter() or its brethren return STALE or AGAIN > before they get to the generic_write_sync() call ? 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. > > > > goto out_nfserr; > > } > > *cnt = host_err; > > @@ -1183,10 +1195,8 @@ nfsd_vfs_write(struct svc_rqst *rqstp, > > struct svc_fh *fhp, struct nfsd_file *nf, > > > > if (stable && use_wgather) { > > host_err = 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); > > } > > > > out_nfserr: > > @@ -1329,8 +1339,7 @@ nfsd_commit(struct svc_rqst *rqstp, struct > > svc_fh *fhp, struct nfsd_file *nf, > > err = nfserr_notsupp; > > break; > > default: > > - nfsd_reset_write_verifier(nn); > > - trace_nfsd_writeverf_reset(nn, rqstp, > > err2); > > + commit_reset_write_verifier(nn, rqstp, > > err2); > > err = nfserrno(err2); > > } > > } else > > -- > > 2.41.0 > > > -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@hammerspace.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfsd: Don't reset the write verifier on a commit EAGAIN 2023-09-11 20:54 ` Trond Myklebust @ 2023-09-11 22:10 ` Chuck Lever III 2023-09-11 23:42 ` Trond Myklebust 0 siblings, 1 reply; 9+ messages in thread From: Chuck Lever III @ 2023-09-11 22:10 UTC (permalink / raw) To: Trond Myklebust; +Cc: Linux NFS Mailing List > On Sep 11, 2023, at 4:54 PM, Trond Myklebust <trondmy@hammerspace.com> wrote: > > 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> >>> >>> 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. >> >> 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? > > 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. > > 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'. > > 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. Good info. I've added some of this to the patch description. >> One more below. >> >> >>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> >>> --- >>> fs/nfsd/vfs.c | 29 +++++++++++++++++++---------- >>> 1 file changed, 19 insertions(+), 10 deletions(-) >>> >>> 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; >>> } >>> >>> +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, >>> &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 = nfserrno(status); >>> } >>> } >>> @@ -1170,8 +1183,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct >>> svc_fh *fhp, struct nfsd_file *nf, >>> host_err = 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); >> >> Can generic_file_write_iter() or its brethren return STALE or AGAIN >> before they get to the generic_write_sync() call ? > > 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. Sounds like I should drop this hunk when applying this fix. >>> goto out_nfserr; >>> } >>> *cnt = host_err; >>> @@ -1183,10 +1195,8 @@ nfsd_vfs_write(struct svc_rqst *rqstp, >>> struct svc_fh *fhp, struct nfsd_file *nf, >>> >>> if (stable && use_wgather) { >>> host_err = 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); >>> } >>> >>> out_nfserr: >>> @@ -1329,8 +1339,7 @@ nfsd_commit(struct svc_rqst *rqstp, struct >>> svc_fh *fhp, struct nfsd_file *nf, >>> err = nfserr_notsupp; >>> break; >>> default: >>> - nfsd_reset_write_verifier(nn); >>> - trace_nfsd_writeverf_reset(nn, rqstp, >>> err2); >>> + commit_reset_write_verifier(nn, rqstp, >>> err2); >>> err = nfserrno(err2); >>> } >>> } else >>> -- >>> 2.41.0 >>> >> > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com -- Chuck Lever ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfsd: Don't reset the write verifier on a commit EAGAIN 2023-09-11 22:10 ` Chuck Lever III @ 2023-09-11 23:42 ` Trond Myklebust 2023-09-12 0:45 ` Chuck Lever III 0 siblings, 1 reply; 9+ messages in thread From: Trond Myklebust @ 2023-09-11 23:42 UTC (permalink / raw) To: chuck.lever@oracle.com; +Cc: linux-nfs@vger.kernel.org On Mon, 2023-09-11 at 22:10 +0000, Chuck Lever III wrote: > > > On Sep 11, 2023, at 4:54 PM, Trond Myklebust > > <trondmy@hammerspace.com> wrote: > > > > 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> > > > > > > > > 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. > > > > > > 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? > > > > 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. > > > > 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'. > > > > 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. > > Good info. I've added some of this to the patch description. > > > > > One more below. > > > > > > > > > > Signed-off-by: Trond Myklebust > > > > <trond.myklebust@hammerspace.com> > > > > --- > > > > fs/nfsd/vfs.c | 29 +++++++++++++++++++---------- > > > > 1 file changed, 19 insertions(+), 10 deletions(-) > > > > > > > > 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; > > > > } > > > > > > > > +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, > > > > > > > > &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 = nfserrno(status); > > > > } > > > > } > > > > @@ -1170,8 +1183,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, > > > > struct > > > > svc_fh *fhp, struct nfsd_file *nf, > > > > host_err = 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); > > > > > > Can generic_file_write_iter() or its brethren return STALE or > > > AGAIN > > > before they get to the generic_write_sync() call ? > > > > 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. > > Sounds like I should drop this hunk when applying this fix. I'm not understanding. Why would you not keep it? > > > > > > goto out_nfserr; > > > > } > > > > *cnt = host_err; > > > > @@ -1183,10 +1195,8 @@ nfsd_vfs_write(struct svc_rqst *rqstp, > > > > struct svc_fh *fhp, struct nfsd_file *nf, > > > > > > > > if (stable && use_wgather) { > > > > host_err = 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); > > > > } > > > > > > > > out_nfserr: > > > > @@ -1329,8 +1339,7 @@ nfsd_commit(struct svc_rqst *rqstp, > > > > struct > > > > svc_fh *fhp, struct nfsd_file *nf, > > > > err = nfserr_notsupp; > > > > break; > > > > default: > > > > - nfsd_reset_write_verifier(nn); > > > > - trace_nfsd_writeverf_reset(nn, rqstp, > > > > err2); > > > > + commit_reset_write_verifier(nn, rqstp, > > > > err2); > > > > err = nfserrno(err2); > > > > } > > > > } else > > > > -- > > > > 2.41.0 > > > > > > > > > > > -- > > Trond Myklebust > > Linux NFS client maintainer, Hammerspace > > trond.myklebust@hammerspace.com > > > -- > Chuck Lever > > -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@hammerspace.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfsd: Don't reset the write verifier on a commit EAGAIN 2023-09-11 23:42 ` Trond Myklebust @ 2023-09-12 0:45 ` Chuck Lever III 2023-09-12 1:11 ` Trond Myklebust 0 siblings, 1 reply; 9+ messages in thread From: Chuck Lever III @ 2023-09-12 0:45 UTC (permalink / raw) To: Trond Myklebust; +Cc: Linux NFS Mailing List > On Sep 11, 2023, at 7:42 PM, Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Mon, 2023-09-11 at 22:10 +0000, Chuck Lever III wrote: >> >>> On Sep 11, 2023, at 4:54 PM, Trond Myklebust >>> <trondmy@hammerspace.com> wrote: >>> >>> 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> >>>>> >>>>> 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. >>>> >>>> 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? >>> >>> 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. >>> >>> 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'. >>> >>> 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. >> >> Good info. I've added some of this to the patch description. >> >> >>>> One more below. >>>> >>>> >>>>> Signed-off-by: Trond Myklebust >>>>> <trond.myklebust@hammerspace.com> >>>>> --- >>>>> fs/nfsd/vfs.c | 29 +++++++++++++++++++---------- >>>>> 1 file changed, 19 insertions(+), 10 deletions(-) >>>>> >>>>> 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; >>>>> } >>>>> >>>>> +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, >>>>> >>>>> &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 = nfserrno(status); >>>>> } >>>>> } >>>>> @@ -1170,8 +1183,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, >>>>> struct >>>>> svc_fh *fhp, struct nfsd_file *nf, >>>>> host_err = 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); >>>> >>>> Can generic_file_write_iter() or its brethren return STALE or >>>> AGAIN >>>> before they get to the generic_write_sync() call ? >>> >>> 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. >> >> Sounds like I should drop this hunk when applying this fix. > > 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 = host_err; >>>>> @@ -1183,10 +1195,8 @@ nfsd_vfs_write(struct svc_rqst *rqstp, >>>>> struct svc_fh *fhp, struct nfsd_file *nf, >>>>> >>>>> if (stable && use_wgather) { >>>>> host_err = 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); >>>>> } >>>>> >>>>> out_nfserr: >>>>> @@ -1329,8 +1339,7 @@ nfsd_commit(struct svc_rqst *rqstp, >>>>> struct >>>>> svc_fh *fhp, struct nfsd_file *nf, >>>>> err = nfserr_notsupp; >>>>> break; >>>>> default: >>>>> - nfsd_reset_write_verifier(nn); >>>>> - trace_nfsd_writeverf_reset(nn, rqstp, >>>>> err2); >>>>> + commit_reset_write_verifier(nn, rqstp, >>>>> err2); >>>>> err = nfserrno(err2); >>>>> } >>>>> } else >>>>> -- >>>>> 2.41.0 >>>>> >>>> >>> >>> -- >>> Trond Myklebust >>> Linux NFS client maintainer, Hammerspace >>> trond.myklebust@hammerspace.com >> >> >> -- >> Chuck Lever >> >> > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > > -- Chuck Lever ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfsd: Don't reset the write verifier on a commit EAGAIN 2023-09-12 0:45 ` Chuck Lever III @ 2023-09-12 1:11 ` Trond Myklebust 2023-09-12 13:19 ` Chuck Lever 0 siblings, 1 reply; 9+ messages in thread From: Trond Myklebust @ 2023-09-12 1:11 UTC (permalink / raw) To: chuck.lever@oracle.com; +Cc: linux-nfs@vger.kernel.org On Tue, 2023-09-12 at 00:45 +0000, Chuck Lever III wrote: > > > > On Sep 11, 2023, at 7:42 PM, Trond Myklebust > > <trondmy@hammerspace.com> wrote: > > > > On Mon, 2023-09-11 at 22:10 +0000, Chuck Lever III wrote: > > > > > > > On Sep 11, 2023, at 4:54 PM, Trond Myklebust > > > > <trondmy@hammerspace.com> wrote: > > > > > > > > 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> > > > > > > > > > > > > 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. > > > > > > > > > > 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? > > > > > > > > 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. > > > > > > > > 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'. > > > > > > > > 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. > > > > > > Good info. I've added some of this to the patch description. > > > > > > > > > > > One more below. > > > > > > > > > > > > > > > > Signed-off-by: Trond Myklebust > > > > > > <trond.myklebust@hammerspace.com> > > > > > > --- > > > > > > fs/nfsd/vfs.c | 29 +++++++++++++++++++---------- > > > > > > 1 file changed, 19 insertions(+), 10 deletions(-) > > > > > > > > > > > > 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; > > > > > > } > > > > > > > > > > > > +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, > > > > > > > > > > > > &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 = nfserrno(status); > > > > > > } > > > > > > } > > > > > > @@ -1170,8 +1183,7 @@ nfsd_vfs_write(struct svc_rqst > > > > > > *rqstp, > > > > > > struct > > > > > > svc_fh *fhp, struct nfsd_file *nf, > > > > > > host_err = 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); > > > > > > > > > > Can generic_file_write_iter() or its brethren return STALE or > > > > > AGAIN > > > > > before they get to the generic_write_sync() call ? > > > > > > > > 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. > > > > > > Sounds like I should drop this hunk when applying this fix. > > > > 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. The point of EAGAIN is that just like NFS4ERR_DELAY, it implies no stateful (read "non-idempotent") changes occurred during the operation that returned it. The call to nfs_revalidate_file_size() will happen before you've changed the page cache, so if it fails with EAGAIN, it won't leave the file in a state where unrecorded changes happened, nor will it cause existing page cache changes to be lost. If, OTOH, the nfs_revalidate_file_size() returns ESTALE, then the page cache changes indeed will not be recorded, but we don't care because the point is that the file is no more. > > 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 = host_err; > > > > > > @@ -1183,10 +1195,8 @@ nfsd_vfs_write(struct svc_rqst > > > > > > *rqstp, > > > > > > struct svc_fh *fhp, struct nfsd_file *nf, > > > > > > > > > > > > if (stable && use_wgather) { > > > > > > host_err = > > > > > > 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); > > > > > > } > > > > > > > > > > > > out_nfserr: > > > > > > @@ -1329,8 +1339,7 @@ nfsd_commit(struct svc_rqst *rqstp, > > > > > > struct > > > > > > svc_fh *fhp, struct nfsd_file *nf, > > > > > > err = nfserr_notsupp; > > > > > > break; > > > > > > default: > > > > > > - nfsd_reset_write_verifier(nn); > > > > > > - trace_nfsd_writeverf_reset(nn, > > > > > > rqstp, > > > > > > err2); > > > > > > + commit_reset_write_verifier(nn, > > > > > > rqstp, > > > > > > err2); > > > > > > err = nfserrno(err2); > > > > > > } > > > > > > } else > > > > > > -- > > > > > > 2.41.0 > > > > > > > > > > > > > > > > > > > -- > > > > Trond Myklebust > > > > Linux NFS client maintainer, Hammerspace > > > > trond.myklebust@hammerspace.com > > > > > > > > > -- > > > Chuck Lever > > > > > > > > > > -- > > Trond Myklebust > > Linux NFS client maintainer, Hammerspace > > trond.myklebust@hammerspace.com > > > > > > -- > Chuck Lever > > -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@hammerspace.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfsd: Don't reset the write verifier on a commit EAGAIN 2023-09-12 1:11 ` Trond Myklebust @ 2023-09-12 13:19 ` Chuck Lever 0 siblings, 0 replies; 9+ messages in thread From: Chuck Lever @ 2023-09-12 13:19 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs@vger.kernel.org On Tue, Sep 12, 2023 at 01:11:28AM +0000, Trond Myklebust wrote: > On Tue, 2023-09-12 at 00:45 +0000, Chuck Lever III wrote: > > > > > > > On Sep 11, 2023, at 7:42 PM, Trond Myklebust > > > <trondmy@hammerspace.com> wrote: > > > > > > On Mon, 2023-09-11 at 22:10 +0000, Chuck Lever III wrote: > > > > > > > > > On Sep 11, 2023, at 4:54 PM, Trond Myklebust > > > > > <trondmy@hammerspace.com> wrote: > > > > > > > > > > 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> > > > > > > > > > > > > > > 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. > > > > > > > > > > > > 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? > > > > > > > > > > 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. > > > > > > > > > > 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'. > > > > > > > > > > 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. > > > > > > > > Good info. I've added some of this to the patch description. > > > > > > > > > > > > > > One more below. > > > > > > > > > > > > > > > > > > > Signed-off-by: Trond Myklebust > > > > > > > <trond.myklebust@hammerspace.com> > > > > > > > --- > > > > > > > fs/nfsd/vfs.c | 29 +++++++++++++++++++---------- > > > > > > > 1 file changed, 19 insertions(+), 10 deletions(-) > > > > > > > > > > > > > > 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; > > > > > > > } > > > > > > > > > > > > > > +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, > > > > > > > > > > > > > > &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 = nfserrno(status); > > > > > > > } > > > > > > > } > > > > > > > @@ -1170,8 +1183,7 @@ nfsd_vfs_write(struct svc_rqst > > > > > > > *rqstp, > > > > > > > struct > > > > > > > svc_fh *fhp, struct nfsd_file *nf, > > > > > > > host_err = 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); > > > > > > > > > > > > Can generic_file_write_iter() or its brethren return STALE or > > > > > > AGAIN > > > > > > before they get to the generic_write_sync() call ? > > > > > > > > > > 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. > > > > > > > > Sounds like I should drop this hunk when applying this fix. > > > > > > 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. > > The point of EAGAIN is that just like NFS4ERR_DELAY, it implies no > stateful (read "non-idempotent") changes occurred during the operation > that returned it. > The call to nfs_revalidate_file_size() will happen before you've > changed the page cache, so if it fails with EAGAIN, it won't leave the > file in a state where unrecorded changes happened, nor will it cause > existing page cache changes to be lost. > > If, OTOH, the nfs_revalidate_file_size() returns ESTALE, then the page > cache changes indeed will not be recorded, but we don't care because > the point is that the file is no more. Thanks. Applied to nfsd-next. > > 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 = host_err; > > > > > > > @@ -1183,10 +1195,8 @@ nfsd_vfs_write(struct svc_rqst > > > > > > > *rqstp, > > > > > > > struct svc_fh *fhp, struct nfsd_file *nf, > > > > > > > > > > > > > > if (stable && use_wgather) { > > > > > > > host_err = > > > > > > > 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); > > > > > > > } > > > > > > > > > > > > > > out_nfserr: > > > > > > > @@ -1329,8 +1339,7 @@ nfsd_commit(struct svc_rqst *rqstp, > > > > > > > struct > > > > > > > svc_fh *fhp, struct nfsd_file *nf, > > > > > > > err = nfserr_notsupp; > > > > > > > break; > > > > > > > default: > > > > > > > - nfsd_reset_write_verifier(nn); > > > > > > > - trace_nfsd_writeverf_reset(nn, > > > > > > > rqstp, > > > > > > > err2); > > > > > > > + commit_reset_write_verifier(nn, > > > > > > > rqstp, > > > > > > > err2); > > > > > > > err = nfserrno(err2); > > > > > > > } > > > > > > > } else > > > > > > > -- > > > > > > > 2.41.0 > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Trond Myklebust > > > > > Linux NFS client maintainer, Hammerspace > > > > > trond.myklebust@hammerspace.com > > > > > > > > > > > > -- > > > > Chuck Lever > > > > > > > > > > > > > > -- > > > Trond Myklebust > > > Linux NFS client maintainer, Hammerspace > > > trond.myklebust@hammerspace.com > > > > > > > > > > -- > > Chuck Lever > > > > > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > > -- Chuck Lever ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfsd: Don't reset the write verifier on a commit EAGAIN 2023-09-11 18:43 [PATCH] nfsd: Don't reset the write verifier on a commit EAGAIN trondmy 2023-09-11 20:14 ` Chuck Lever @ 2023-09-12 10:54 ` Jeff Layton 1 sibling, 0 replies; 9+ messages in thread From: Jeff Layton @ 2023-09-12 10:54 UTC (permalink / raw) To: trondmy, Chuck Lever; +Cc: linux-nfs On Mon, 2023-09-11 at 14:43 -0400, trondmy@gmail.com wrote: > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > 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. > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > --- > fs/nfsd/vfs.c | 29 +++++++++++++++++++---------- > 1 file changed, 19 insertions(+), 10 deletions(-) > > 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; > } > > +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, > &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 = nfserrno(status); > } > } > @@ -1170,8 +1183,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf, > host_err = 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); > goto out_nfserr; > } > *cnt = host_err; > @@ -1183,10 +1195,8 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf, > > if (stable && use_wgather) { > host_err = 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); > } > > out_nfserr: > @@ -1329,8 +1339,7 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf, > err = nfserr_notsupp; > break; > default: > - nfsd_reset_write_verifier(nn); > - trace_nfsd_writeverf_reset(nn, rqstp, err2); > + commit_reset_write_verifier(nn, rqstp, err2); > err = nfserrno(err2); > } > } else Reviewed-by: Jeff Layton <jlayton@kernel.org> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-09-12 13:19 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-09-11 18:43 [PATCH] nfsd: Don't reset the write verifier on a commit EAGAIN trondmy 2023-09-11 20:14 ` Chuck Lever 2023-09-11 20:54 ` Trond Myklebust 2023-09-11 22:10 ` Chuck Lever III 2023-09-11 23:42 ` Trond Myklebust 2023-09-12 0:45 ` Chuck Lever III 2023-09-12 1:11 ` Trond Myklebust 2023-09-12 13:19 ` Chuck Lever 2023-09-12 10:54 ` Jeff Layton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox