On Wed, Sep 03, 2025 at 12:12:10PM -0400, Chuck Lever wrote:
> On 9/3/25 12:02 PM, Mike Snitzer wrote:
> > On Wed, Sep 03, 2025 at 11:07:29AM -0400, Mike Snitzer wrote:
> >> On Wed, Sep 03, 2025 at 10:38:45AM -0400, Chuck Lever wrote:
> >
> >>>> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> >>>> index 1cd0bed57bc2f..6ef799405145f 100644
> >>>> --- a/fs/nfsd/nfsd.h
> >>>> +++ b/fs/nfsd/nfsd.h
> >>>> @@ -153,6 +153,15 @@ static inline void nfsd_debugfs_exit(void) {}
> >>>>
> >>>> extern bool nfsd_disable_splice_read __read_mostly;
> >>>>
> >>>> +enum {
> >>>> + NFSD_IO_UNSPECIFIED = 0,
> >>>> + NFSD_IO_BUFFERED,
> >>>> + NFSD_IO_DONTCACHE,
> >>>> + NFSD_IO_DIRECT,
> >>>> +};
> >>>> +
> >>>> +extern u64 nfsd_io_cache_read __read_mostly;
> >>>
> >>> And then here, initialize nfsd_io_cache_read to reflect the default
> >>> behavior. That would be NFSD_IO_BUFFERED for now... then later we might
> >>> want to change it to NFSD_IO_DIRECT, for instance.
> >>>
> >>> Same suggestion for 4/7.
> >>
> >> Ah ok, I can see the way forward to default to NFSD_IO_BUFFERED but
> >> _not_ default to it when erroring (if the user specified some unknown
> >> value).
> >>
> >> I'll run with that (despite just asking Jeff's opinion above, I'm the
> >> one who came up with the awkward UNSPECIFIED state when honoring
> >> Jeff's early feedback).
> >
> > Here is the incremental diff (these changes will be folded into
> > appropriate patches in v9):
> >
> > diff --git a/fs/nfsd/debugfs.c b/fs/nfsd/debugfs.c
> > index 8878c3519b30c..173032a04cdec 100644
> > --- a/fs/nfsd/debugfs.c
> > +++ b/fs/nfsd/debugfs.c
> > @@ -43,11 +43,10 @@ DEFINE_DEBUGFS_ATTRIBUTE(nfsd_dsr_fops, nfsd_dsr_get, nfsd_dsr_set, "%llu\n");
> > * /sys/kernel/debug/nfsd/io_cache_read
> > *
> > * Contents:
> > - * %1: NFS READ will use buffered IO
> > - * %2: NFS READ will use dontcache (buffered IO w/ dropbehind)
> > - * %3: NFS READ will use direct IO
> > + * %0: NFS READ will use buffered IO
> > + * %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 (UNSPECIFIED).
> > * This setting takes immediate effect for all NFS versions,
> > * all exports, and in all NFSD net namespaces.
> > */
> > @@ -90,11 +89,10 @@ DEFINE_DEBUGFS_ATTRIBUTE(nfsd_io_cache_read_fops, nfsd_io_cache_read_get,
> > * /sys/kernel/debug/nfsd/io_cache_write
> > *
> > * Contents:
> > - * %1: NFS WRITE will use buffered IO
> > - * %2: NFS WRITE will use dontcache (buffered IO w/ dropbehind)
> > - * %3: NFS WRITE will use direct IO
> > + * %0: NFS WRITE will use buffered IO
> > + * %1: NFS WRITE will use dontcache (buffered IO w/ dropbehind)
> > + * %2: NFS WRITE will use direct IO
> > *
> > - * The default value of this setting is zero (UNSPECIFIED).
> > * This setting takes immediate effect for all NFS versions,
> > * all exports, and in all NFSD net namespaces.
> > */
> > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> > index fe935b4cda538..412a1e9a2a876 100644
> > --- a/fs/nfsd/nfsd.h
> > +++ b/fs/nfsd/nfsd.h
> > @@ -154,8 +154,7 @@ static inline void nfsd_debugfs_exit(void) {}
> > extern bool nfsd_disable_splice_read __read_mostly;
> >
> > enum {
> > - NFSD_IO_UNSPECIFIED = 0,
> > - NFSD_IO_BUFFERED,
> > + NFSD_IO_BUFFERED = 0,
>
> Thanks, this LGTM. Two additional remarks:
>
> 1. I think that the "= 0" is unneeded here because C enumerators always
> start at 0.
It does, I flip-flopped on removing the "= 0" and left it (thinking
was it'd help dissuade others from inserting new enum values at the
beginning). But rather than do that I can just add a comment.
> 2. I'm wondering if this enum definition should be moved to a uapi
> header. Thoughts? This is experimental, and not a fixed API. So maybe
> it needs to stay in fs/nfsd/nfsd.h.
>
> (I'm probably going over ground that has already been covered.)
Don't think this aspect was covered, yes my thinking was this is an
experimental interface that wasn't appropriate to expose in a uapi
header.
>
> > NFSD_IO_DONTCACHE,
> > NFSD_IO_DIRECT,
> > };
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 5e700a0d6b12e..403076443573f 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -50,8 +50,8 @@
> > #define NFSDDBG_FACILITY NFSDDBG_FILEOP
> >
> > bool nfsd_disable_splice_read __read_mostly;
> > -u64 nfsd_io_cache_read __read_mostly;
> > -u64 nfsd_io_cache_write __read_mostly;
> > +u64 nfsd_io_cache_read __read_mostly = NFSD_IO_BUFFERED;
> > +u64 nfsd_io_cache_write __read_mostly = NFSD_IO_BUFFERED;
> >
> > /**
> > * nfserrno - Map Linux errnos to NFS errnos
> > @@ -1272,8 +1272,7 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > break;
> > case NFSD_IO_DONTCACHE:
> > kiocb.ki_flags = IOCB_DONTCACHE;
> > - fallthrough;
> > - case NFSD_IO_UNSPECIFIED:
> > + break;
> > case NFSD_IO_BUFFERED:
> > break;
> > }
> > @@ -1605,8 +1604,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > break;
> > case NFSD_IO_DONTCACHE:
> > kiocb.ki_flags |= IOCB_DONTCACHE;
> > - fallthrough;
> > - case NFSD_IO_UNSPECIFIED:
> > + fallthrough; /* must call nfsd_issue_write_buffered */
>
> Right. In this case, the NFSD_IO_BUFFERED arm is more than just a
> "break;" so, not as brittle as the nfsd_iter_read() switch statement.
> The comment is helpful, though; I'm not suggesting a change, just
> observing.
Sure.
Thanks,
Mike
