Re: [RFC PATCH v2 6/8] NFSD: add io_cache_read controls to debugfs interface

[Mike Snitzer <snitzer@kernel.org>]

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@kernel.org>
> >> ---
> >>  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;
+}