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: Mon, 10 Feb 2020 18:47:54 +0000	[thread overview]
Message-ID: <9cb604f804e8be5a689cf6e5ee7c517dd0c6e8f8.camel@hammerspace.com> (raw)
In-Reply-To: <20200207181817.GC17036@fieldses.org>

On Fri, 2020-02-07 at 13:18 -0500, bfields@fieldses.org wrote:
> On Fri, Feb 07, 2020 at 02:25:27PM +0000, Trond Myklebust wrote:
> > 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?
> 
> I'm basically doing
> 
> 	./nfs4.1/testserver.py myserver:/path reboot
> 			--serverhelper=examples/server_helper.sh
> 			--serverhelperarg=myserver
> 
> For all I know at this point, the change could be exposing a pynfs-
> side
> bug.
> 
> > 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.
> 
> Understood.  I was curious whether this was found by code inspection
> or
> because you'd run across a case where the leak was causing a
> practical
> problem.

I was seeing hangs in some cases where the path lookup was temporarily
failing in the export path downcall. In that case, the mount daemon
just ignores the returned error, and leaves it up to the kernel to fix
what is happening.

The problem is that since the garbage collector is currently told to
ignore stuff if CACHE_VALID is not set, and since cache_check() won't
do a second upcall, then any RPC call trying to access data on the path
that was affected just hangs and eventually times out. Nothing prompts
for this condition to be fixed.

> 
> --b.
> 
> > 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?
Trond Myklebust
CTO, Hammerspace Inc
4300 El Camino Real, Suite 105
Los Altos, CA 94022
www.hammer.space

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com