On Fri, 2025-02-21 at 10:02 -0500, Mike Snitzer wrote:
> 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.
>
Would it make more sense to hook DONTCACHE up to the IO_ADVISE
operation in RFC7862? IO_ADVISE4_NOREUSE sounds like it has similar
meaning? That would give the clients a way to do this on a per-open
basis.
> > 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.
Isn't /sys covered by the same ABI guarantees? I know debugfs isn't,
but I'm not sure about /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.
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
