On Thu, Jul 10, 2025 at 06:46:37PM -0400, Chuck Lever wrote:
> On 7/10/25 10:06 AM, Jeff Layton wrote:
> > On Tue, 2025-07-08 at 12:06 -0400, Mike Snitzer wrote:
> >> Add 'io_cache_read' to NFSD's debugfs interface so that: Any data
> >> read by NFSD will either be:
> >> - cached using page cache (NFSD_IO_BUFFERED=0)
> >> - cached but removed from the page cache upon completion
> >> (NFSD_IO_DONTCACHE=1).
> >> - not cached (NFSD_IO_DIRECT=2)
> >>
> >> io_cache_read is 0 by default. It may be set by writing to:
> >> /sys/kernel/debug/nfsd/io_cache_read
> >>
> >> If NFSD_IO_DONTCACHE is specified using 1, FOP_DONTCACHE must be
> >> advertised as supported by the underlying filesystem (e.g. XFS),
> >> otherwise all IO flagged with RWF_DONTCACHE will fail with
> >> -EOPNOTSUPP.
> >>
> >> If NFSD_IO_DIRECT is specified using 2, the IO must be aligned
> >> relative to the underlying block device's logical_block_size. Also the
> >> memory buffer used to store the read must be aligned relative to the
> >> underlying block device's dma_alignment.
> >>
> >> Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> >> ---
> >> fs/nfsd/debugfs.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++
> >> fs/nfsd/nfsd.h | 8 +++++++
> >> fs/nfsd/vfs.c | 15 ++++++++++++++
> >> 3 files changed, 76 insertions(+)
> >>
> >> diff --git a/fs/nfsd/debugfs.c b/fs/nfsd/debugfs.c
> >> index 84b0c8b559dc..709646af797a 100644
> >> --- a/fs/nfsd/debugfs.c
> >> +++ b/fs/nfsd/debugfs.c
> >> @@ -27,11 +27,61 @@ static int nfsd_dsr_get(void *data, u64 *val)
> >> static int nfsd_dsr_set(void *data, u64 val)
> >> {
> >> nfsd_disable_splice_read = (val > 0) ? true : false;
> >> + if (!nfsd_disable_splice_read) {
> >> + /*
> >> + * Cannot use NFSD_IO_DONTCACHE or NFSD_IO_DIRECT
> >> + * if splice_read is enabled.
> >> + */
> >> + nfsd_io_cache_read = NFSD_IO_BUFFERED;
> >> + }
> >> return 0;
> >> }
> >>
> >> DEFINE_DEBUGFS_ATTRIBUTE(nfsd_dsr_fops, nfsd_dsr_get, nfsd_dsr_set, "%llu\n");
> >>
> >> +/*
> >> + * /sys/kernel/debug/nfsd/io_cache_read
> >> + *
> >> + * Contents:
> >> + * %0: NFS READ will use buffered IO (default)
> >> + * %1: NFS READ will use dontcache (buffered IO w/ dropbehind)
> >> + * %2: NFS READ will use direct IO
> >> + *
> >> + * The default value of this setting is zero (buffered IO is
> >> + * used). This setting takes immediate effect for all NFS
> >> + * versions, all exports, and in all NFSD net namespaces.
> >> + */
> >> +
> >
> > Could we switch this to use a string instead? Maybe
> > buffered/dontcache/direct ?
>
> That thought occurred to me too, since it would make the API a little
> more self-documenting, and might be a harbinger of what a future
> export option might look like.
>
>
> >> +static int nfsd_io_cache_read_get(void *data, u64 *val)
> >> +{
> >> + *val = nfsd_io_cache_read;
> >> + return 0;
> >> +}
> >> +
> >> +static int nfsd_io_cache_read_set(void *data, u64 val)
> >> +{
> >> + switch (val) {
> >> + case NFSD_IO_DONTCACHE:
> >> + case NFSD_IO_DIRECT:
> >> + /*
> >> + * Must disable splice_read when enabling
> >> + * NFSD_IO_DONTCACHE or NFSD_IO_DIRECT.
> >> + */
> >> + nfsd_disable_splice_read = true;
> >> + nfsd_io_cache_read = val;
> >> + break;
> >> + case NFSD_IO_BUFFERED:
> >> + default:
> >> + nfsd_io_cache_read = NFSD_IO_BUFFERED;
> >> + break;
> >
> > I think the default case should leave nfsd_io_cache_read alone and
> > return an error. If we add new values later, and someone tries to use
> > them on an old kernel, it's better to make that attempt error out.
> >
> > Ditto for the write side controls.
>
> +1 on both accounts.
I started to implement this just now (so that I can kick v3 of this
patchset out of the nest today) but soon found that debugfs doesn't
provide string-based interface controls.
See simple_attr_open() (which is used by DEFINE_DEBUGFS_ATTRIBUTE).
It only allows u64 to be set/get.
I'll fix the default case to return an error for now though.
Once we graduate from debugfs to a proper per-export control we can
impose string controls/mapping, e.g.:
+static u64 nfsd_io_cache_string_to_mode(const char *nfsd_io_cache_string)
+{
+ u64 val = NFSD_IO_UNKNOWN;
+
+ if (!strncmp(nfsd_io_cache_string, NFSD_IO_BUFFERED_string,
+ strlen(NFSD_IO_BUFFERED_string)))
+ val = NFSD_IO_BUFFERED;
+ else if (!strncmp(nfsd_io_cache_string, NFSD_IO_DONTCACHE_string,
+ strlen(NFSD_IO_DONTCACHE_string)))
+ val = NFSD_IO_DONTCACHE;
+ else if (!strncmp(nfsd_io_cache_string, NFSD_IO_DIRECT_string,
+ strlen(NFSD_IO_DIRECT_string)))
+ val = NFSD_IO_DIRECT;
+
+ return val;
+}
+
+static const char *
+nfsd_io_cache_mode_to_string(const char *nfsd_io_cache_string)
+{
+ char *nfsd_io_cache_string;
+
+ switch (val) {
+ case NFSD_IO_BUFFERED:
+ nfsd_io_cache_string = NFSD_IO_BUFFERED_string;
+ break;
+ case NFSD_IO_DONTCACHE:
+ nfsd_io_cache_string = NFSD_IO_DONTCACHE_string;
+ break;
+ case NFSD_IO_DIRECT:
+ nfsd_io_cache_string = NFSD_IO_DIRECT_string;
+ break;
+ case NFSD_IO_UNKNOWN:
+ nfsd_io_cache_string = NFSD_IO_UNKNOWN_string;
+ break;
+ }
+
+ return nfsd_io_cache_string;
+}
