Re: [PATCH] SUNRPC/cache: Allow garbage collection of invalid cache entries

From: Trond Myklebust <trondmy@hammerspace.com>
To: "bfields@fieldses.org" <bfields@fieldses.org>
Cc: "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
	"bfields@redhat.com" <bfields@redhat.com>
Subject: Re: [PATCH] SUNRPC/cache: Allow garbage collection of invalid cache entries
Date: Fri, 7 Feb 2020 14:25:27 +0000	[thread overview]
Message-ID: <8dc1ed17de98e4b59fb9e408692c152456863a20.camel@hammerspace.com> (raw)
In-Reply-To: <20200206163322.GB2244@fieldses.org>

On Thu, 2020-02-06 at 11:33 -0500, J. Bruce Fields wrote:
> On Tue, Jan 14, 2020 at 11:57:38AM -0500, Trond Myklebust wrote:
> > If the cache entry never gets initialised, we want the garbage
> > collector to be able to evict it. Otherwise if the upcall daemon
> > fails to initialise the entry, we end up never expiring it.
> 
> Could you tell us more about what motivated this?
> 
> It's causing failures on pynfs server-reboot tests.  I haven't pinned
> down the cause yet, but it looks like it could be a regression to the
> behavior Kinglong Mee describes in detail in his original patch.
> 

Can you point me to the tests that are failing?

The motivation here is to allow the garbage collector to do its job of
evicting cache entries after they are supposed to have timed out. The
fact that uninitialised cache entries are given an infinite lifetime,
and are never evicted is a de facto memory leak if, for instance, the
mountd daemon ignores the cache request, or the downcall in
expkey_parse() or svc_export_parse() fails without being able to update
the request.

The threads that are waiting for the cache replies already have a
mechanism for dealing with timeouts (with cache_wait_req() and deferred
requests), so the question is what is so special about uninitialised
requests that we have to leak them in order to avoid a problem with
reboot?

> Dropping for the time being.
> 
> --b.
> 
> > Fixes: d6fc8821c2d2 ("SUNRPC/Cache: Always treat the invalid cache
> > as unexpired")
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  include/linux/sunrpc/cache.h |  3 ---
> >  net/sunrpc/cache.c           | 36 +++++++++++++++++++-------------
> > ----
> >  2 files changed, 19 insertions(+), 20 deletions(-)
> > 
> > diff --git a/include/linux/sunrpc/cache.h
> > b/include/linux/sunrpc/cache.h
> > index f8603724fbee..0428dd23fd79 100644
> > --- a/include/linux/sunrpc/cache.h
> > +++ b/include/linux/sunrpc/cache.h
> > @@ -202,9 +202,6 @@ static inline void cache_put(struct cache_head
> > *h, struct cache_detail *cd)
> >  
> >  static inline bool cache_is_expired(struct cache_detail *detail,
> > struct cache_head *h)
> >  {
> > -	if (!test_bit(CACHE_VALID, &h->flags))
> > -		return false;
> > -
> >  	return  (h->expiry_time < seconds_since_boot()) ||
> >  		(detail->flush_time >= h->last_refresh);
> >  }
> > diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> > index 52d927210d32..99d630150af6 100644
> > --- a/net/sunrpc/cache.c
> > +++ b/net/sunrpc/cache.c
> > @@ -65,13 +65,14 @@ static struct cache_head
> > *sunrpc_cache_find_rcu(struct cache_detail *detail,
> >  
> >  	rcu_read_lock();
> >  	hlist_for_each_entry_rcu(tmp, head, cache_list) {
> > -		if (detail->match(tmp, key)) {
> > -			if (cache_is_expired(detail, tmp))
> > -				continue;
> > -			tmp = cache_get_rcu(tmp);
> > -			rcu_read_unlock();
> > -			return tmp;
> > -		}
> > +		if (!detail->match(tmp, key))
> > +			continue;
> > +		if (test_bit(CACHE_VALID, &tmp->flags) &&
> > +		    cache_is_expired(detail, tmp))
> > +			continue;
> > +		tmp = cache_get_rcu(tmp);
> > +		rcu_read_unlock();
> > +		return tmp;
> >  	}
> >  	rcu_read_unlock();
> >  	return NULL;
> > @@ -114,17 +115,18 @@ static struct cache_head
> > *sunrpc_cache_add_entry(struct cache_detail *detail,
> >  
> >  	/* check if entry appeared while we slept */
> >  	hlist_for_each_entry_rcu(tmp, head, cache_list) {
> > -		if (detail->match(tmp, key)) {
> > -			if (cache_is_expired(detail, tmp)) {
> > -				sunrpc_begin_cache_remove_entry(tmp,
> > detail);
> > -				freeme = tmp;
> > -				break;
> > -			}
> > -			cache_get(tmp);
> > -			spin_unlock(&detail->hash_lock);
> > -			cache_put(new, detail);
> > -			return tmp;
> > +		if (!detail->match(tmp, key))
> > +			continue;
> > +		if (test_bit(CACHE_VALID, &tmp->flags) &&
> > +		    cache_is_expired(detail, tmp)) {
> > +			sunrpc_begin_cache_remove_entry(tmp, detail);
> > +			freeme = tmp;
> > +			break;
> >  		}
> > +		cache_get(tmp);
> > +		spin_unlock(&detail->hash_lock);
> > +		cache_put(new, detail);
> > +		return tmp;
> >  	}
> >  
> >  	hlist_add_head_rcu(&new->cache_list, head);
> > -- 
> > 2.24.1
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com