Re: [PATCH 5/6] NFSD: leverage DIO alignment to selectively issue O_DIRECT reads and writes

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2025-06-11 at 11:11 -0400, Chuck Lever wrote:
> On 6/11/25 11:07 AM, Jeff Layton wrote:
> > On Wed, 2025-06-11 at 10:42 -0400, Chuck Lever wrote:
> > > On 6/10/25 4:57 PM, Mike Snitzer wrote:
> > > > IO must be aligned, otherwise it falls back to using buffered IO.
> > > > 
> > > > RWF_DONTCACHE is _not_ currently used for misaligned IO (even when
> > > > nfsd/enable-dontcache=1) because it works against us (due to RMW
> > > > needing to read without benefit of cache), whereas buffered IO enables
> > > > misaligned IO to be more performant.
> > > > 
> > > > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > > > ---
> > > >  fs/nfsd/vfs.c | 40 ++++++++++++++++++++++++++++++++++++----
> > > >  1 file changed, 36 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > > > index e7cc8c6dfbad..a942609e3ab9 100644
> > > > --- a/fs/nfsd/vfs.c
> > > > +++ b/fs/nfsd/vfs.c
> > > > @@ -1064,6 +1064,22 @@ __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > > >  	return nfsd_finish_read(rqstp, fhp, file, offset, count, eof, host_err);
> > > >  }
> > > >  
> > > > +static bool is_dio_aligned(const struct iov_iter *iter, loff_t offset,
> > > > +			   const u32 blocksize)
> > > > +{
> > > > +	u32 blocksize_mask;
> > > > +
> > > > +	if (!blocksize)
> > > > +		return false;
> > > > +
> > > > +	blocksize_mask = blocksize - 1;
> > > > +	if ((offset & blocksize_mask) ||
> > > > +	    (iov_iter_alignment(iter) & blocksize_mask))
> > > > +		return false;
> > > > +
> > > > +	return true;
> > > > +}
> > > > +
> > > >  /**
> > > >   * nfsd_iter_read - Perform a VFS read using an iterator
> > > >   * @rqstp: RPC transaction context
> > > > @@ -1107,8 +1123,16 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > > >  	trace_nfsd_read_vector(rqstp, fhp, offset, *count);
> > > >  	iov_iter_bvec(&iter, ITER_DEST, rqstp->rq_bvec, v, *count);
> > > >  
> > > > -	if (nfsd_enable_dontcache)
> > > > -		flags |= RWF_DONTCACHE;
> > > > +	if (nfsd_enable_dontcache) {
> > > > +		if (is_dio_aligned(&iter, offset, nf->nf_dio_read_offset_align))
> > > > +			flags |= RWF_DIRECT;
> > > > +		/* FIXME: not using RWF_DONTCACHE for misaligned IO because it works
> > > > +		 * against us (due to RMW needing to read without benefit of cache),
> > > > +		 * whereas buffered IO enables misaligned IO to be more performant.
> > > > +		 */
> > > > +		//else
> > > > +		//	flags |= RWF_DONTCACHE;
> > > > +	}
> > > >  
> > > >  	host_err = vfs_iter_read(file, &iter, &ppos, flags);
> > > >  	return nfsd_finish_read(rqstp, fhp, file, offset, count, eof, host_err);
> > > > @@ -1217,8 +1241,16 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > > >  	nvecs = xdr_buf_to_bvec(rqstp->rq_bvec, rqstp->rq_maxpages, payload);
> > > >  	iov_iter_bvec(&iter, ITER_SOURCE, rqstp->rq_bvec, nvecs, *cnt);
> > > >  
> > > > -	if (nfsd_enable_dontcache)
> > > > -		flags |= RWF_DONTCACHE;
> > > > +	if (nfsd_enable_dontcache) {
> > > > +		if (is_dio_aligned(&iter, offset, nf->nf_dio_offset_align))
> > > > +			flags |= RWF_DIRECT;
> > > > +		/* FIXME: not using RWF_DONTCACHE for misaligned IO because it works
> > > > +		 * against us (due to RMW needing to read without benefit of cache),
> > > > +		 * whereas buffered IO enables misaligned IO to be more performant.
> > > > +		 */
> > > > +		//else
> > > > +		//	flags |= RWF_DONTCACHE;
> > > > +	}
> > > 
> > > IMO adding RWF_DONTCACHE first then replacing it later in the series
> > > with a form of O_DIRECT is confusing. Also, why add RWF_DONTCACHE here
> > > and then take it away "because it doesn't work"?
> > > 
> > > But OK, your series is really a proof-of-concept. Something to work out
> > > before it is merge-ready, I guess.
> > > 
> > > It is much more likely for NFS READ requests to be properly aligned.
> > > Clients are generally good about that. NFS WRITE request alignment
> > > is going to be arbitrary. Fwiw.
> > > 
> > > However, one thing we discussed at bake-a-thon was what to do about
> > > unstable WRITEs. For unstable WRITEs, the server has to cache the
> > > write data at least until the client sends a COMMIT. Otherwise the
> > > server will have to convert all UNSTABLE writes to FILE_SYNC writes,
> > > and that can have performance implications.
> > > 
> > 
> > If we're doing synchronous, direct I/O writes then why not just respond
> > with FILE_SYNC? The write should be on the platter by the time it
> > returns.
> 
> Because "platter". On some devices, writes are slow.
> 
> For some workloads, unstable is faster. I have an experimental series
> that makes NFSD convert all NFS WRITEs to FILE_SYNC. It was not an
> across the board win, even with an NVMe-backed file system.
> 

Presumably, those devices wouldn't be exported in this mode. That's
probably a good argument for making this settable on a per-export
basis.

> 
> > > One thing you might consider is to continue using the page cache for
> > > unstable WRITEs, and then use fadvise DONTNEED after a successful
> > > COMMIT operation to reduce page cache footprint. Unstable writes to
> > > the same range of the file might be a problem, however.
> > 
> > Since the client sends almost everything UNSTABLE, that would probably
> > erase most of the performance win. The only reason I can see to use
> > buffered I/O in this mode would be because we had to deal with an
> > unaligned write and need to do a RMW cycle on a block.
> > 
> > The big question is whether mixing buffered and direct I/O writes like
> > this is safe across all exportable filesystems. I'm not yet convinced
> > of that.
> 
> Agreed, that deserves careful scrutiny.
> 

Like Mike is asking though, I need a better understanding of the
potential races here:

XFS, for instance, takes the i_rwsem shared around dio writes and
exclusive around buffered, so they should exclude each other. If we did
all the buffered writes as RWF_SYNC, would that prevent corruption?

In any case, for now at least, unless you're using RDMA, it's going to
end up falling back to buffered writes everywhere. The data is almost
never going to be properly aligned coming in off the wire. That might
be fixable though.
-- 
Jeff Layton <jlayton@kernel.org>