Re: [PATCH] Avoid useless inodes and dentries reclamation

[Dave Chinner <david@fromorbit.com>]

On Fri, Aug 30, 2013 at 09:21:34AM -0700, Tim Chen wrote:
> On Fri, 2013-08-30 at 11:40 +1000, Dave Chinner wrote:
> 
> > 
> > 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.
> 
> If that's the case, then we should remove grab_super_passive
> from the super_cache_count code.  That should remove the bottleneck
> in reclamation.
> 
> Thanks for your detailed explanation.
> 
> Tim
> 
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> ---
> diff --git a/fs/super.c b/fs/super.c
> index 73d0952..4df1fab 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -112,9 +112,6 @@ static unsigned long super_cache_count(struct shrinker *shrink,
>  
>  	sb = container_of(shrink, struct super_block, s_shrink);
>  
> -	if (!grab_super_passive(sb))
> -		return 0;
> -

I think the function needs a comment explaining why we aren't
grabbing the sb here, otherwise people are going to read the code
and ask why it's different to the scanning callout.

>  	if (sb->s_op && sb->s_op->nr_cached_objects)
>  		total_objects = sb->s_op->nr_cached_objects(sb,
>  						 sc->nid);

But seeing this triggered further thought on my part. Being called
during unmount means that ->nr_cached_objects implementations need
to be robust against unmount tearing down private filesystem
structures.  Right now, grab_super_passive() protects us from that
because it won't be able to get the sb->s_umount lock while
generic_shutdown_super() is doing it's work.

IOWs, the superblock based shrinker operations are safe because the
structures don't get torn down until after the shrinker is
unregistered. That's not true for the structures that
->nr_cached_objects() use: ->put_super() tears them down before the
shrinker is unregistered and only grab_super_passive() protects us
from thay.

Let me have a bit more of a think about this - the solution may
simply be unregistering the shrinker before we call ->kill_sb() so
the shrinker can't get called while we are tearing down the fs.
First, though, I need to go back and remind myself of why I put that
after ->kill_sb() in the first place.  If we unregister the shrinker
before ->kill_sb is called, then we can probably get rid of
grab_super_passive() in both shrinker callouts because they will no
longer need to handle running concurrently with ->kill_sb()....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com