Re: [PATCH 1/3] nfsd: avoid using DONTCACHE for misaligned DIO's buffered IO fallback

Chuck Lever
2025-11-04T19:33:53Z

On 11/4/25 12:35 PM, Mike Snitzer wrote:
> On Tue, Nov 04, 2025 at 12:23:02PM -0500, Chuck Lever wrote:
>> On 11/4/25 11:42 AM, Mike Snitzer wrote:
>>> Also, use buffered IO (without DONTCACHE) if READ is less than 32K.
>>> But do use DONTCACHE if an entire WRITE is misaligned, this preserves
>>> intent of NFSD_IO_DIRECT.
>> These two changes need to be separate patches.
> They are linked, otherwise if READ uses DONTCACHE for the small IO
> it'll kill any benefit to RMW.
>
> Unless I'm misunderstanding which two changes you're referring to?

It's unclear to me why the READ and WRITE parts of this patch are
related to each other. So, if you believe these are inseparable changes,
the patch description must explain why.

I don't understand why performing READs with buffered I/O has any effect
on the RMW behavior of direct WRITEs, for instance. Unless, you mean
that it's not an RMW cycle during the server's write processing, but the
application and client are doing RMW? If so, the use of "RMW" in the
patch description and new comments is misleading... and I'm not sure we
care all that much about optimizing unfortunate application behavior.

> The "But do use DONTCACHE if an entire WRITE is misaligned" just
> amounts to a comment tweak in nfsd_direct_write (last hunk below)

Don't the last two hunks below both modify the direct write path?

The Subject: is misleading, then: it refers to both "misaligned I/O" and
"fallback". I think you mean only the unaligned ends should avoid
DONTCACHE?

> + /* Don't use expanded DIO READ for IO less than 32K */
> + if ((*count < (32 << 10)) &&
> + (((offset - dio_start) > 0) || ((dio_end - (offset + *count)) > 0)))
> + return nfserrno(-ENOTBLK); /* fallback to buffered */

A few more comments on this hunk:

- What caught me up about "nfserrno(-ENOTBLK)" is that generally
nfserrno() is for converting an errno that is not known at compile
time. Here you are passing it a constant. I mean, that is already
what you do in the caller with "if (nfserr != nfserr_eagain)".

- Why cap the buffered READs at exactly 32KB? That feels like you are
using a number that is beneficial to a specific workload and hardware
configuration. I'd like to have a warm, fuzzy feeling that you have
looked at the impact of this change on other common workloads.

- The convention for new code added to fs/nfsd is to use symbolic
constants rather than raw integers for such magic numbers. The
definition of said constant would appear in an NFSD header.

- The new comment is repeating what the code does. A more useful comment
might explain why this cap is necessary. But, such a comment might
instead be placed in front of the definition of the symbolic constant
mentioned above.

> On one capable testbed, this commit improved IOR_HARD WRITE
> performance from 0.3433GB/s to 1.26GB/s.

Can you elaborate a bit on what the benchmark is doing? Why make a code
change rather than simply recognize this is a workload that does not
perform well with NFSD_IO_DIRECT ?

--
Chuck Lever