On 13 Mar 2013 07:55:00 +0100 NeilBrown <neilb@xxxxxxx> wrote:
> On 11 Mar 2013 17:13:41 +0100 Bodo Stroesser <bstroesser@xxxxxxxxxxxxxx>
> wrote:
>
> > Hi,
> >
> > AFAICS, there is one more race in RPC cache.
> >
> > The problem showed up for the "auth.unix.ip" cache, that
> > has a reader.
> >
> > If a server thread tries to find a cache entry, it first
> > does a lookup (or calls ip_map_cached_get() in this specific
> > case). Then, it calls cache_check() for this entry.
> >
> > If the reader updates the cache entry just between the
> > thread's lookup and cache_check() execution, cache_check()
> > will do an upcall for this cache entry. This is, because
> > sunrpc_cache_update() calls cache_fresh_locked(old, 0),
> > which sets expiry_time to 0.
> >
> > Unfortunately, the reply to the upcall will not dequeue
> > the queued cache_request, as the reply will be assigned to
> > the cache entry newly created by the above cache update.
> >
> > The result is a growing queue of orphaned cache_request
> > structures --> memory leak.
> >
> > I found this on a SLES11 SP1 with a backport of the latest
> > patches that fix the other RPC races. On this old kernel,
> > the problem also leads to svc_drop() being called for the
> > affected RPC request (after svc_defer()).
> >
> > Best Regards
> > Bodo
>
> I don't think this is a real problem.
> The periodic call to "cache_clean" should find these orphaned requests and
> purge them. So you could get a short term memory leak, but it should
> correct itself.
> Do you agree?
>
> Thanks,
> NeilBrown
Meanwhile I found the missing part of the race!
It's just what I wrote above but additionally, immediately after
the reader updated the cache, cache_clean() unhashes the old cache
entry. In that case:
1) the thread will queue a request for a cache entry, that isn't
in the hash lists.
2) cache_clean() never will access this old cache entry again
3) every further cache update will call cache_dequeue() for a newer
cache entry, the request is never dequeued
--> memory leak.
I verified this inserting some debug instructions. In a testrun
that triggered 6 times, and the queue printed by crash after the
run had 6 orphaned entries.
As I wrote yesterday, I have a patch that solves the problem by
retesting the state of the cache entry after setting CACHE_PENDING
in cache_check(). I can send it if you like.
Bodo
P.S.:
Maybe I'm wrong, but AFAICS, there are two further minor problems
regarding (nearly) expired cache entries:
a) ip_map_cached_get() in some situations can return an already
outdated (it uses cache_valid that not fully checks the state)
cache entry. If cache_check() is caclled for that entry, it will
fail, I think
b) Generally, if a cache entry is returned by sunrpc_cache_lookup()
and the entry is in the last second of it's expiry_time, the
clock might move to the next second before cache_check is called.
If this happens, cache_check will fail, I think.
Do you agree?
ÿôèº{.nÇ+?·?®??+%?Ëÿ±éݶ¥?wÿº{.nÇ+?·¥?{±þwìþ)í?æèw* jg¬±¨ ¶????Ý¢jÿ¾«þG«?éÿ¢¸ ¢·¦j:+v?¨?wèjØm¶?ÿþø ¯ù ®w¥þ?àþf£¢·h??â?úÿ?Ù¥
