On Thu, Feb 20, 2025 at 01:17:42PM -0500, Chuck Lever wrote:
> [ Adding NFSD reviewers ... ]
>
> On 2/20/25 12:12 PM, Mike Snitzer wrote:
> > Add nfsd 'nfsd_dontcache' modparam so that "Any data read or written
> > by nfsd will be removed from the page cache upon completion."
> >
> > nfsd_dontcache is disabled by default. It may be enabled with:
> > echo Y > /sys/module/nfsd/parameters/nfsd_dontcache
>
> A per-export setting like an export option would be nicer. Also, does
> it make sense to make it a separate control for READ and one for WRITE?
> My trick knee suggests caching read results is still going to add
> significant value, but write, not so much.
My intent was to make 6.14's DONTCACHE feature able to be tested in
the context of nfsd in a no-frills way. I realize adding the
nfsd_dontcache knob skews toward too raw, lacks polish. But I'm
inclined to expose such course-grained opt-in knobs to encourage
others' discovery (and answers to some of the questions you pose
below). I also hope to enlist all NFSD reviewers' help in
categorizing/documenting where DONTCACHE helps/hurts. ;)
And I agree that ultimately per-export control is needed. I'll take
the time to implement that, hopeful to have something more suitable in
time for LSF.
> However, to add any such administrative control, I'd like to see some
> performance numbers. I think we need to enumerate the cases (I/O types)
> that are most interesting to examine: small memory NFS servers; lots of
> small unaligned I/O; server-side CPU per byte; storage interrupt rates;
> any others?
>
> And let's see some user/admin documentation (eg when should this setting
> be enabled? when would it be contra-indicated?)
>
> The same arguments that applied to Cedric's request to make maximum RPC
> size a tunable setting apply here. Do we want to carry a manual setting
> for this mechanism for a long time, or do we expect that the setting can
> become automatic/uninteresting after a period of experimentation?
>
> * It might be argued that putting these experimental tunables under /sys
> eliminates the support longevity question, since there aren't strict
> rules about removing files under /sys.
Right, I do think a sysfs knob (that defaults to disabled, requires
user opt-in) is a pretty useful and benign means to expose
experimental functionality.
And I agree with all you said needed above, I haven't had the time to
focus on DONTCACHE since ~Decemeber, I just picked up my old patches
from that time and decided to send the NFSD one since DONTCACHE has
been merged for 6.14.
> > FOP_DONTCACHE must be advertised as supported by the underlying
> > filesystem (e.g. XFS), otherwise if/when 'nfsd_dontcache' is enabled
> > all IO will fail with -EOPNOTSUPP.
>
> It would be better all around if NFSD simply ignored the setting in the
> cases where the underlying file system doesn't implement DONTCACHE.
I'll work on making it so.
>
>
> > Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> > ---
> > fs/nfsd/vfs.c | 17 ++++++++++++++++-
> > 1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 29cb7b812d71..d7e49004e93d 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -955,6 +955,11 @@ nfsd_open_verified(struct svc_fh *fhp, int may_flags, struct file **filp)
> > return __nfsd_open(fhp, S_IFREG, may_flags, filp);
> > }
> >
> > +static bool nfsd_dontcache __read_mostly = false;
> > +module_param(nfsd_dontcache, bool, 0644);
> > +MODULE_PARM_DESC(nfsd_dontcache,
> > + "Any data read or written by nfsd will be removed from the page cache upon completion.");
> > +
> > /*
> > * Grab and keep cached pages associated with a file in the svc_rqst
> > * so that they can be passed to the network sendmsg routines
> > @@ -1084,6 +1089,7 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > loff_t ppos = offset;
> > struct page *page;
> > ssize_t host_err;
> > + rwf_t flags = 0;
> >
> > v = 0;
> > total = *count;
> > @@ -1097,9 +1103,12 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > }
> > WARN_ON_ONCE(v > ARRAY_SIZE(rqstp->rq_vec));
> >
> > + if (nfsd_dontcache)
> > + flags |= RWF_DONTCACHE;
> > +
> > trace_nfsd_read_vector(rqstp, fhp, offset, *count);
> > iov_iter_kvec(&iter, ITER_DEST, rqstp->rq_vec, v, *count);
> > - host_err = vfs_iter_read(file, &iter, &ppos, 0);
> > + host_err = vfs_iter_read(file, &iter, &ppos, flags);
> > return nfsd_finish_read(rqstp, fhp, file, offset, count, eof, host_err);
> > }
> >
> > @@ -1186,6 +1195,9 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf,
> > if (stable && !fhp->fh_use_wgather)
> > flags |= RWF_SYNC;
> >
> > + if (nfsd_dontcache)
> > + flags |= RWF_DONTCACHE;
> > +
> > iov_iter_kvec(&iter, ITER_SOURCE, vec, vlen, *cnt);
> > since = READ_ONCE(file->f_wb_err);
> > if (verf)
> > @@ -1237,6 +1249,9 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf,
> > */
> > bool nfsd_read_splice_ok(struct svc_rqst *rqstp)
> > {
> > + if (nfsd_dontcache) /* force the use of vfs_iter_read for reads */
> > + return false;
> > +
>
> Urgh.
Heh, yeah, bypassing splice was needed given dontcache hooks off vfs_iter_read.
> So I've been mulling over simply removing the splice read path.
>
> - Less code, less complexity, smaller test matrix
>
> - How much of a performance loss would result?
>
> - Would such a change make it easier to pass whole folios from
> the file system directly to the network layer?
Good to know.
Thanks,
Mike
