Re: [PATCH RFC 2/2] nfsd: call generic_fadvise after v3 READ, stable WRITE or COMMIT

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2025-07-04 at 09:44 +1000, NeilBrown wrote:
> On Fri, 04 Jul 2025, Jeff Layton wrote:
> > Recent testing has shown that keeping pagecache pages around for too
> > long can be detrimental to performance with nfsd. Clients only rarely
> > revisit the same data, so the pages tend to just hang around.
> > 
> > This patch changes the pc_release callbacks for NFSv3 READ, WRITE and
> > COMMIT to call generic_fadvise(..., POSIX_FADV_DONTNEED) on the accessed
> > range.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfsd/debugfs.c  |  2 ++
> >  fs/nfsd/nfs3proc.c | 59 +++++++++++++++++++++++++++++++++++++++++++++---------
> >  fs/nfsd/nfsd.h     |  1 +
> >  fs/nfsd/nfsproc.c  |  4 ++--
> >  fs/nfsd/vfs.c      | 21 ++++++++++++++-----
> >  fs/nfsd/vfs.h      |  5 +++--
> >  fs/nfsd/xdr3.h     |  3 +++
> >  7 files changed, 77 insertions(+), 18 deletions(-)
> > 
> > diff --git a/fs/nfsd/debugfs.c b/fs/nfsd/debugfs.c
> > index 84b0c8b559dc90bd5c2d9d5e15c8e0682c0d610c..b007718dd959bc081166ec84e06f577a8fc2b46b 100644
> > --- a/fs/nfsd/debugfs.c
> > +++ b/fs/nfsd/debugfs.c
> > @@ -44,4 +44,6 @@ void nfsd_debugfs_init(void)
> >  
> >  	debugfs_create_file("disable-splice-read", S_IWUSR | S_IRUGO,
> >  			    nfsd_top_dir, NULL, &nfsd_dsr_fops);
> > +	debugfs_create_bool("enable-fadvise-dontneed", 0644,
> > +			    nfsd_top_dir, &nfsd_enable_fadvise_dontneed);
> >  }
> > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> > index b6d03e1ef5f7a5e8dd111b0d56c061f1e91abff7..11261cf67ea817ec566626f08b733e09c9e121de 100644
> > --- a/fs/nfsd/nfs3proc.c
> > +++ b/fs/nfsd/nfs3proc.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/ext2_fs.h>
> >  #include <linux/magic.h>
> >  #include <linux/namei.h>
> > +#include <linux/fadvise.h>
> >  
> >  #include "cache.h"
> >  #include "xdr3.h"
> > @@ -206,11 +207,25 @@ nfsd3_proc_read(struct svc_rqst *rqstp)
> >  
> >  	fh_copy(&resp->fh, &argp->fh);
> >  	resp->status = nfsd_read(rqstp, &resp->fh, argp->offset,
> > -				 &resp->count, &resp->eof);
> > +				 &resp->count, &resp->eof, &resp->nf);
> >  	resp->status = nfsd3_map_status(resp->status);
> >  	return rpc_success;
> >  }
> >  
> > +static void
> > +nfsd3_release_read(struct svc_rqst *rqstp)
> > +{
> > +	struct nfsd3_readargs *argp = rqstp->rq_argp;
> > +	struct nfsd3_readres *resp = rqstp->rq_resp;
> > +
> > +	if (nfsd_enable_fadvise_dontneed && resp->status == nfs_ok)
> > +		generic_fadvise(nfsd_file_file(resp->nf), argp->offset, resp->count,
> > +				POSIX_FADV_DONTNEED);
> > +	if (resp->nf)
> > +		nfsd_file_put(resp->nf);
> > +	fh_put(&resp->fh);
> 
> This looks wrong - testing resp->nf after assuming it was non-NULL.
> I don't think it *is* wrong because ->state == nfs_ok ensures
> ->nf is valid. But still....
> 

That was my thinking, but I agree that it's a bit fragile.

> How about:
> 
>     fh_put(resp->fh);
>     if (!resp->nf)
>          return;
>     if (nfsd_enable_fadvise_dontneed)
> 	generic_fadvise(nfsd_file_file(resp->nf), argp->offset, resp->count,
> 		POSIX_FADV_DONTNEED);
>     nfsd_file_put(resp->nf);
> 
> ??
> Note that we don't test ->status because that is identical to testing ->nf.
> Ditto for other release functions.
> 

That looks good. I'll plan to do that in the next respin.

> Otherwise it makes sense for exploring how to optimise IO.
> 
> Reviewed-by: NeilBrown <neil@brown.name>
> 
> NeilBrown


Thanks for the reviews!
-- 
Jeff Layton <jlayton@kernel.org>