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@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;
> +}
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@xxxxxxxxxx>
