Re: [RFC PATCH] NFSD: Make FILE_SYNC WRITEs comply with spec

[Mike Snitzer <snitzer@kernel.org>]

On Wed, Oct 22, 2025 at 01:22:52PM -0400, Tom Talpey wrote:
> There needs to be some statement of the performance consequences of
> removing this "optimization". I'm going to predict it's significant
> on rotating media, and should not go unmeasured.

I agree that rotational storage will likely see an impact.  But
Linux's more recent (now like 14 years ago) FLUSH+FUA rework really
helped improve things -- that was a major undertaking where Christoph,
Jens and others really did a fantastic job of breathing new life into
Linux performance on modern rotational storage.

Related blast from the past: https://lwn.net/Articles/457667/

My point: may not be as grim as we think...
but there is a difference between SATA rotational storage and
"enterprise" rotational storage (e.g. NetApp or EMC fronted by
elaborate caching that is configured to report wbc=0 because they have
battery backed cache)

Mike

ps: I don't have any rotational storage to test, not it!

> The commit message needs to more bluntly state the fact that the
> server has been violating the quoted MUST. See previous paragraph.
> 
> Tom.
> 
> On 10/22/2025 12:22 PM, Chuck Lever wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > Mike noted that when NFSD responds to an NFS_FILE_SYNC WRITE, it
> > does not also persist file time stamps. To wit, Section 18.32.3
> > of RFC 8881 mandates:
> > 
> > > The client specifies with the stable parameter the method of how
> > > the data is to be processed by the server. If stable is
> > > FILE_SYNC4, the server MUST commit the data written plus all file
> > > system metadata to stable storage before returning results. This
> > > corresponds to the NFSv2 protocol semantics. Any other behavior
> > > constitutes a protocol violation. If stable is DATA_SYNC4, then
> > > the server MUST commit all of the data to stable storage and
> > > enough of the metadata to retrieve the data before returning.
> > 
> > For many years, NFSD has used a "data sync only" optimization for
> > FILE_SYNC WRITEs, so file time stamps haven't been persisted as the
> > mandate above requires.
> > 
> > Reported-by: Mike Snitzer <snitzer@kernel.org>
> > Closes: https://lore.kernel.org/linux-nfs/20251018005431.3403-1-cel@kernel.org/T/#t
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >   fs/nfsd/vfs.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > This would need to be applied to nfsd-testing before the DIRECT
> > WRITE patches. I'm guessing a Cc: stable would be needed as well.
> > 
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index f537a7b4ee01..2c5d38f38454 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -1315,7 +1315,8 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >   	init_sync_kiocb(&kiocb, file);
> >   	kiocb.ki_pos = offset;
> >   	if (stable && !fhp->fh_use_wgather)
> > -		kiocb.ki_flags |= IOCB_DSYNC;
> > +		kiocb.ki_flags |=
> > +			(stable == NFS_FILE_SYNC ? IOCB_SYNC : IOCB_DSYNC);
> >   	nvecs = xdr_buf_to_bvec(rqstp->rq_bvec, rqstp->rq_maxpages, payload);
> >   	iov_iter_bvec(&iter, ITER_SOURCE, rqstp->rq_bvec, nvecs, *cnt);
>