Re: [PATCH] Avoid useless inodes and dentries reclamation

[Dave Chinner <david@fromorbit.com>]

On Thu, Aug 29, 2013 at 11:07:56AM -0700, Tim Chen wrote:
> 
> > > Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> > > ---
> > >  fs/super.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/fs/super.c b/fs/super.c
> > > index 68307c0..70fa26c 100644
> > > --- a/fs/super.c
> > > +++ b/fs/super.c
> > > @@ -53,6 +53,7 @@ static char *sb_writers_name[SB_FREEZE_LEVELS] = {
> > >   * shrinker path and that leads to deadlock on the shrinker_rwsem. Hence we
> > >   * take a passive reference to the superblock to avoid this from occurring.
> > >   */
> > > +#define SB_CACHE_LOW 5
> > >  static int prune_super(struct shrinker *shrink, struct shrink_control *sc)
> > >  {
> > >  	struct super_block *sb;
> > > @@ -68,6 +69,13 @@ static int prune_super(struct shrinker *shrink, struct shrink_control *sc)
> > >  	if (sc->nr_to_scan && !(sc->gfp_mask & __GFP_FS))
> > >  		return -1;
> > >  
> > > +	/*
> > > +	 * Don't prune if we have few cached objects to reclaim to
> > > +	 * avoid useless sb_lock contention
> > > +	 */
> > > +	if ((sb->s_nr_dentry_unused + sb->s_nr_inodes_unused) <= SB_CACHE_LOW)
> > > +		return -1;
> > 
> > Those counters no longer exist in the current mmotm tree and the
> > shrinker infrastructure is somewhat different, so this patch isn't
> > the right way to solve this problem.
> 
> These changes in mmotm tree do complicate solutions for this
> scalability issue.
> 
> > 
> > Given that superblock LRUs and shrinkers in mmotm are node aware,
> > there may even be more pressure on the sblock in such a workload.  I
> > think the right way to deal with this is to give the shrinker itself
> > a "minimum call count" so that we can avoid even attempting to
> > shrink caches that does have enough entries in them to be worthwhile
> > shrinking.
> 
> By "minimum call count", do you mean tracking the number of free
> entries per node in the shrinker, and invoking shrinker 
> only when the number of free entries
> exceed "minimum call count"?

The new shrinker infrastructure has a ->count_objects() callout to
specifically return the number of objects in the cache.
shrink_slab_node() can check that return value against the "minimum
call count" and determine whether it needs to call ->scan_objects()
at all.

Actually, the shrinker already behaves like this with the batch_size
variable - the shrinker has to be asking for more items to be
scanned than the batch size. That means the problem is that counting
callouts are causing the problem, not the scanning callouts.

With the new code in the mmotm tree, for counting purposes we
probably don't need to grab a passive superblock reference at all -
the superblock and LRUs are guaranteed to be valid if the shrinker
is currently running, but we don't really care if the superblock is
being shutdown and the values that come back are invalid because the
->scan_objects() callout will fail to grab the superblock to do
anything with the calculated values.

Indeed, I've made no attempt to optimise the code in the mmotm tree
- I've been concerned with correctness. The fact that without any
optimisation it significantly lessens contention in my testing has
been sufficient to move forward with.

> There is some cost in
> list_lru_count_node to get the free entries, as we need
> to acquire the node's lru lock.

Right, but we really don't need the node's lru lock to get the count
- reading the count is racy from an external perspective, anyway, so
we can do lockless counting here.  See this patch I proposed a
while back, for example:

https://lkml.org/lkml/2013/7/31/7

> > That said, the memcg guys have been saying that even small numbers
> > of items per cache can be meaningful in terms of memory reclaim
> > (e.g. when there are lots of memcgs) then such a threshold might
> > only be appropriate for caches that are not memcg controlled. 
> 
> I've done some experiment with the CACHE thresholds.  Even setting
> the threshold at 0 (i.e. there're no free entries) remove almost all 
> the needless contentions.  That should make the memcg guys happy by
> not holding any extra free entries.

Ok, that's good to know, and it further indicates that it is the
->count_objects() callout that is the issue, not the
scanning/freeing.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com