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

bfields
2020-02-07T18:18:19Z

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.

--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?