On Fri, 28 Oct 2022, Jeff Layton wrote:
> On Fri, 2022-10-28 at 09:20 +1100, NeilBrown wrote:
> > On Fri, 28 Oct 2022, Jeff Layton wrote:
> > > Currently, nfsd_files live on the LRU once they are added until they are
> > > unhashed. There's no need to keep ones that are actively in use there.
> >
> > Is that true?
> > nfsd_file_do_acquire() calls nfsd_file_lru_remove()
> > Isn't that enough to keep the file off the lru while it is active?
> >
> > Thanks,
> > NeilBrown
> >
>
> After patch #1, it doesn't call that anymore. That's probably a (minor)
> regression then.
Yes, I eventually found that - thanks.
>
> After patch #1, the LRU holds a reference. If you successfully remove it
> from the LRU, you need to transfer or put that reference. Doing the LRU
> handling in the get and put routines seems more natural, I think.
Maybe. But then you need a __get as well as a get.
Though it might seem asymmetric, I would prefer removing from the lru in
'acquire' and adding to the lru in put.
>
> Maybe I just need to squash this patch into #1?
Or do the "put" if lru_remove succeeds in the first patch. Then revise
it all in the second.
Thanks,
NeilBrown
>
> >
> > >
> > > Before incrementing the refcount, do a lockless check for nf_lru being
> > > empty. If it's not then attempt to remove the entry from the LRU. If
> > > that's successful, claim the LRU reference and return it. If the removal
> > > fails (or if the list_head was empty), then just increment the counter
> > > as we normally would.
> > >
> > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > ---
> > > fs/nfsd/filecache.c | 23 ++++++++++++++++++++---
> > > 1 file changed, 20 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > > index e63534f4b9f8..d2bbded805d4 100644
> > > --- a/fs/nfsd/filecache.c
> > > +++ b/fs/nfsd/filecache.c
> > > @@ -420,14 +420,31 @@ nfsd_file_unhash(struct nfsd_file *nf)
> > > return false;
> > > }
> > >
> > > -struct nfsd_file *
> > > -nfsd_file_get(struct nfsd_file *nf)
> > > +static struct nfsd_file *
> > > +__nfsd_file_get(struct nfsd_file *nf)
> > > {
> > > if (likely(refcount_inc_not_zero(&nf->nf_ref)))
> > > return nf;
> > > return NULL;
> > > }
> > >
> > > +struct nfsd_file *
> > > +nfsd_file_get(struct nfsd_file *nf)
> > > +{
> > > + /*
> > > + * Do a lockless list_empty check first, before attempting to
> > > + * remove it, so we can avoid the spinlock when it's not on the
> > > + * list.
> > > + *
> > > + * If we successfully remove it from the LRU, then we can just
> > > + * claim the LRU reference and return it. Otherwise, we need to
> > > + * bump the counter the old-fashioned way.
> > > + */
> > > + if (!list_empty(&nf->nf_lru) && nfsd_file_lru_remove(nf))
> > > + return nf;
> > > + return __nfsd_file_get(nf);
> > > +}
> > > +
> > > /**
> > > * nfsd_file_unhash_and_queue - unhash a file and queue it to the dispose list
> > > * @nf: nfsd_file to be unhashed and queued
> > > @@ -449,7 +466,7 @@ nfsd_file_unhash_and_queue(struct nfsd_file *nf, struct list_head *dispose)
> > > * to take a reference. If that fails, just ignore
> > > * the file altogether.
> > > */
> > > - if (!nfsd_file_lru_remove(nf) && !nfsd_file_get(nf))
> > > + if (!nfsd_file_lru_remove(nf) && !__nfsd_file_get(nf))
> > > return false;
> > > list_add(&nf->nf_lru, dispose);
> > > return true;
> > > --
> > > 2.37.3
> > >
> > >
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
>
