Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <dd3936feb109857040ad79e7da47d7b7e5732a41.camel@kernel.org>
Subject: Re: [PATCH v2 2/3] nfsd: only keep unused entries on the LRU
From:   Jeff Layton <jlayton@kernel.org>
To:     NeilBrown <neilb@suse.de>
Cc:     chuck.lever@oracle.com, linux-nfs@vger.kernel.org
Date:   Thu, 27 Oct 2022 18:47:21 -0400
In-Reply-To: <166690925944.13915.14734120966513564215@noble.neil.brown.name>
References: <20221027215213.138304-1-jlayton@kernel.org>
        , <20221027215213.138304-3-jlayton@kernel.org>
         <166690925944.13915.14734120966513564215@noble.neil.brown.name>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) 
MIME-Version: 1.0
Precedence: bulk

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 ar=
e
> > unhashed. There's no need to keep ones that are actively in use there.
>=20
> 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?
>=20
> Thanks,
> NeilBrown
>=20

After patch #1, it doesn't call that anymore. That's probably a (minor)
regression then.

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 I just need to squash this patch into #1?

>=20
> >=20
> > 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 remova=
l
> > fails (or if the list_head was empty), then just increment the counter
> > as we normally would.
> >=20
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfsd/filecache.c | 23 ++++++++++++++++++++---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
> >=20
> > 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;
> >  }
> > =20
> > -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;
> >  }
> > =20
> > +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 disp=
ose list
> >   * @nf: nfsd_file to be unhashed and queued
> > @@ -449,7 +466,7 @@ nfsd_file_unhash_and_queue(struct nfsd_file *nf, st=
ruct 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;
> > --=20
> > 2.37.3
> >=20
> >=20

--=20
Jeff Layton <jlayton@kernel.org>