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

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2025-07-14 at 12:47 -0400, Mike Snitzer wrote:
> 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;
> +}


Bummer.

I guess we could just roll our own using the seqfile interfaces and put
it in the same directory. I may take a stab at that before we ship
this. For now, we can stick with the integers.

Thanks for fixing up the default case!

-- 
Jeff Layton <jlayton@kernel.org>