Re: [PATCH] nfsd: when reusing an existing repcache entry, unhash it first

[Jeff Layton <jlayton@redhat.com>]

On Wed, 4 Dec 2013 00:33:36 -0800
Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, Dec 03, 2013 at 01:21:12PM -0500, Jeff Layton wrote:
> > Most of this code is protected by a single spinlock (cache_lock). The
> > main benefit to switching to list_lru would be that we could move to a
> > per-node lock. But, to make that worthwhile would mean we'd need to
> > redesign the locking and break the cache_lock into multiple locks.
> 
> No need to redo locks just yet, the biggest benefit is to use a well
> debug library for lru list handling, with the second biggest one being
> that you get out of the box shrinker support.
> 
> > Also, the existing code does take pains to reuse an expired entry off
> > the LRU list in preference to allocating a new one. The list_lru code
> > doesn't have a mechanism to scrape the first entry off the LRU list,
> > though I suppose we could add one or abuse the cb_arg in the walk
> > callback as a return pointer.
> 
> That's just because it is an old-school cache with a fixed upper bound
> of entries and no shrinker support.  With proper shrinker integration
> this nasty code isn't needed.

Well, the old code just had a fixed number of entries (1024 or so?),
which was ridiculously small on modern server. A busy server could blow
out the cache in less than the retransmit interval, which made it
effectively worthless.

The new code has a much larger upper bound, but it is still bounded by
the amount of low memory on the host. We have shrinker integration
(though its propriety might be in question).

Are you suggesting that we shouldn't try to bound the cache at all and
should instead just let it grow and rely on the shrinker to keep it in
check?

-- 
Jeff Layton <jlayton@redhat.com>