Re: [PATCH v4 2/3] NFSD: Implement NFSD_IO_DIRECT for NFS WRITE

[Christoph Hellwig <hch@infradead.org>]

On Wed, Oct 22, 2025 at 10:37:35AM -0400, Chuck Lever wrote:
> What I'm hearing is that nfsd_vfs_write() should never see -ENOTBLK, so
> this check is essentially dead code.

That's how iomap_dio_rw is designed to work.  If it ever leaks out,
that is a file system bug because no other caller of ->write_iter
handles -ENOTBLK.

> >>>> These changes served as the original starting point for the NFS
> >>>> client's misaligned O_DIRECT support that landed with
> >>>> commit c817248fc831 ("nfs/localio: add proper O_DIRECT support for
> >>>> READ and WRITE"). But NFSD's support is simpler because it currently
> >>>> doesn't use AIO completion.
> >>>
> >>> I don't understand this paragraph.  What does starting point mean
> >>> here?  How does it matter for the patch description?
> >>
> >> This patch's NFSD changes were starting point for NFS commit
> >> c817248fc831
> > 
> > But that commit is already in?  This sentence really confuses me.
> 
> I'm not convinced this paragraph in the patch description is needed.
> Can it be removed?

I guess Mike wants to say the two are based on the concept.  But given
that c817248fc831 is already in I'd expect to word it as "these changes
are similar to the direct I/O writes implemented for the NFS client
local I/O feature in commit c817248fc831 <insert subject line here>.
But I'm not really sure there is much of a point in that.

> (Perhaps to get us started, Christoph, do you know of specific code that
> shows how this code could be reorganized?)
> 

This is what I had in mind:

https://lore.kernel.org/linux-block/20251014150456.2219261-1-kbusch@meta.com/T/#m3588b5824b7fa76f001d57e54664889b73471422

It records the alignment while building (or iterating for another
reason), and then just checking a mask later.  For nfsd this would be a
bit different as you'd also record the boundaries of the aligned
segments, but the idea is the same.

> If NFSD responds with NFS_FILE_SYNC here, then timestamps need to be
> persisted as well as data. As discussed in other threads, currently
> nfsd_vfs_write() seems to miss that mark.
> 
> So either: return NFS_DATA_SYNC (which means we've persisted only the
> data) or make this path persist the timestamps too.
> 
> I haven't seen any existing code that convinces me that setting both
> IOCB_SYNC and IOCB_DSYNC is necessary.

iocb_flags is the canonical place.  IOCB_DSYNC is the main path driving
the syncing, and IOCB_SYNC drivers the additional syncing of the
timestamps.

This is all a bit odd, because Linux historically only supposed the
O_SYNC flag, but used it for O_DSYNC semantics.  When I fixed that,
I had to do a few non-standard things so that we could keep existing
binaries work as expected despite renaming the flags.  See commit
6b2f3d1f769b ("vfs: Implement proper O_SYNC semantics") for details.