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

[Mike Snitzer <snitzer@kernel.org>]

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/

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 ?

(I took to setting both for NFSD Direct, and NFS LOCALIO sets
both.. that was done by original LOCALIO author)

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?  I tried to review that
but wasn't able to spend enough time on it to be convinced that to be
the case.

Mike