Re: [PATCH v2] NFSD: Implement NFSD_IO_DIRECT for NFS WRITE

[Mike Snitzer <snitzer@kernel.org>]

Hi Chuck,

On Thu, Oct 09, 2025 at 01:46:34PM -0400, Chuck Lever wrote:
> On 10/8/25 2:59 PM, Mike Snitzer wrote:
> > +
> > +static noinline_for_stack int
> > +nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > +		  struct nfsd_file *nf, loff_t offset, unsigned int nvecs,
> > +		  unsigned long *cnt, struct kiocb *kiocb)
> > +{
> > +	struct nfsd_write_dio write_dio;
> > +
> > +	/* Any buffered IO issued here will be misaligned, use
> > +	 * IOCB_SYNC to ensure it has completed before returning.
> > +	 */
> > +	kiocb->ki_flags |= IOCB_SYNC;
> > +	/* Check if IOCB_DONTCACHE should be used when issuing buffered IO;
> > +	 * if so, it will be ignored for any DIO issued here.
> > +	 */
> > +	if (nf->nf_file->f_op->fop_flags & FOP_DONTCACHE)
> > +		kiocb->ki_flags |= IOCB_DONTCACHE;
> > +
> > +	if (nfsd_is_write_dio_possible(offset, *cnt, nf, &write_dio)) {
> > +		trace_nfsd_write_direct(rqstp, fhp, offset, *cnt);
> > +		return nfsd_issue_write_dio(rqstp, fhp, nf, offset, nvecs,
> > +					    cnt, kiocb, &write_dio);
> > +	}
> > +
> > +	return nfsd_buffered_write(rqstp, nf->nf_file, nvecs, cnt, kiocb);
> > +}
> 
> Handful of initial comments:
> 
> The current NFSv3 code path, and perhaps NFSv4 too, doesn't support
> changing a WRITE marked as UNSTABLE to FILE_SYNC. I'm not seeing that
> rectified in this patch. I have a patch that enables changing the
> "stable_how" setting, but it's on a system at home. I'll post it in
> a couple of days and we can build on that.

Just a quick follow-up to see if you might have time to either share
your patch and/or rebase NFSD Direct WRITE ontop of it?

> In nfsd_direct_write we're setting IOCB_SYNC early. When falling back to
> BUFFERED_IO, that setting is preserved, and the fallback buffered path
> is now always FILE_SYNC. We should discuss whether the fallback in this
> case should be always FILE_SYNC or should allow UNSTABLE.

Probably makes sense to only set IOCB_SYNC _and_ IOCB_DONTCACHE if
nfsd_is_write_dio_possible() returns true.  So inverting
nfsd_direct_write()'s flow as such:

   if (!nfsd_is_write_dio_possible())
      return nfsd_buffered_write()

   // could push the setting these flags into nfsd_issue_write_dio?
   kiocb->ki_flags |= IOCB_SYNC;
   if (nf->nf_file->f_op->fop_flags & FOP_DONTCACHE)
      kiocb->ki_flags |= IOCB_DONTCACHE;

   trace_nfsd_write_direct(rqstp, fhp, offset, *cnt);
   return nfsd_issue_write_dio()

But the devil is in the details in conjunction with needing to rebase
to deal with your change to pass stable_how by reference, etc.

> Nit: I'd rather use the normal kernel comment style here, where an
> initial "/*" appears on a line by itself.

OK, will use that in general moving forward.

Thanks,
Mike