* Re: [RFC PATCH] NFSD: Make FILE_SYNC WRITEs comply with spec
2025-10-22 17:27 ` [RFC PATCH] NFSD: Make FILE_SYNC WRITEs comply with spec Mike Snitzer
@ 2025-10-28 22:28 ` Dave Chinner
0 siblings, 0 replies; 2+ messages in thread
From: Dave Chinner @ 2025-10-28 22:28 UTC (permalink / raw)
To: Mike Snitzer
Cc: Chuck Lever, NeilBrown, Jeff Layton, Olga Kornievskaia, Dai Ngo,
Tom Talpey, linux-nfs, Chuck Lever, linux-xfs, honza
On Wed, Oct 22, 2025 at 01:27:54PM -0400, Mike Snitzer wrote:
> On Wed, Oct 22, 2025 at 12:22:37PM -0400, 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);
> > --
> > 2.51.0
> >
>
> I agree with this change. And as I just replied elsewhere, IOCB_SYNC
> doesn't cause a performance drop (at least not on modern systems with
> NVMe): https://lore.kernel.org/linux-nfs/aPkNvmXsgdNJtK_7@kernel.org/
Well, that depends on the underlying file layout. If the test was
doing IO that required allocation or unwritten extent modification,
the IOCB_SYNC performance is identical to IOCB_DSYNC because they
both have to stabilise metadata needed to access the file data.
However, if it is a pure overwrite (i.e. writing into already
written space) then the only filesystem metadata update that occurs
is timestamps. At this point, IOCB_DSYNC is *much* faster than
IOCB_SYNC because it does not require a journal flush to stabilise
metadata.
So if you didn't see any change in performance between DSYNC and
SYNC writes, then is likely that the the tests did not exercise the
pure overwrite path which many high performance applications
optimise for (e.g. databases).
IOWs, I'd definitely expect performance regressions to be reported
by users from this change further down the line...
> Only question I have:
> does IOCB_SYNC _always_ imply IOCB_DSYNC (for VFS and all
> filesystems)? Or should we be setting IOCB_DSYNC|IOCB_SYNC ?
Yes - "all metadata" (_SYNC) has always been a super set
of "enough metadata to retreive the data" (_DSYNC)...
> (I took to setting both for NFSD Direct, and NFS LOCALIO sets
> both.. that was done by original LOCALIO author)
That's a (harmless) bug.
> Basis for my question, is that there was a recent XFS performance
> improvement made by Dave Chinner, reported by Jan Kara, for
> DIO+DSYNC, see commit c91d38b57f2c4 ("xfs: rework datasync tracking
> and execution"). Will DIO + IOCB_SYNC get the benefit of DIO +
> IOCB_DSYNC relative to this XFS improvement?
Yes, it will. But IOCB_SYNC will still be much slower than
IOCB_DSYNC for pure overwrites....
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 2+ messages in thread