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

[Christoph Hellwig <hch@infradead.org>]

On Wed, Oct 22, 2025 at 01:59:04PM -0400, Chuck Lever wrote:
> On 10/20/25 3:19 AM, Christoph Hellwig wrote:
> >> +	/*
> >> +	 * Any buffered IO issued here will be misaligned, use
> >> +	 * sync IO to ensure it has completed before returning.
> >> +	 * Also update @stable_how to avoid need for COMMIT.
> >> +	 */
> >> +	kiocb->ki_flags |= (IOCB_DSYNC | IOCB_SYNC);
> > What do you mean with completed before returning?  I guess you
> > mean writeback actually happening, right?  Why do you need that,
> > why do you also force it for the direct I/O?
> 
> This is the only comment where I'm not clear on what corrective
> action to take.

I have multiple issues with the comment.  One is that it really should
talk about commiting data to stable storage instead of completing,
as that seems to be the main point (and also what IOCB_DSYNC /
IOCB_SYNC) does.  If we stick to using IOCB_SYNC it should also
mention metadata or more specifically timestamps.

But I also really don't get the "Any buffered IO issued here will
be misaligned, ..." part.  What does buffered I/O and/or misalignment
have to do with not wanting to do a COMMIT?

> I think IOCB_SYNC would be needed with O_DIRECT to force timestamp
> updates. Otherwise, IOCB_SYNC is relevant only when the function is
> forced to fall back to some form of write through the page cache.

Well, IOCB_SYNC is only needed to commit timestamps.  O_DSYNC is
always required if you want to commit to stable storage.  As said
above I don't really understand from the patch why we want to do
that, but IFF we want to do that, we need IOCB_DSYNC bother for
direct and buffered I/O.