Re: [PATCH] shrink_dcache_parent() races against shrink_dcache_memory()

[Jan Blunck <jblunck@suse.de>]

On Mon, Jan 30, Balbir Singh wrote:

> >  static inline void prune_one_dentry(struct dentry * dentry)
> >  {
> > +	struct super_block *sb = dentry->d_sb;
> >  	struct dentry * parent;
> >  
> >  	__d_drop(dentry);
> >  	list_del(&dentry->d_u.d_child);
> >  	dentry_stat.nr_dentry--;	/* For d_free, below */
> > +	sb->s_prunes++;
> >  	dentry_iput(dentry);
> >  	parent = dentry->d_parent;
> >  	d_free(dentry);
> >  	if (parent != dentry)
> >  		dput(parent);
> >  	spin_lock(&dcache_lock);
> > +	sb->s_prunes--;
> > +	wake_up(&sb->s_wait_prunes);
> >  }
> >
>   
> We can think about optimizing this to
>    if (!sb->sprunes)
> 	wake_up(&sb->s_wait_prunes);
> 

Hardly. This is only the case when two or more shrinkers are active in
parallel. If that was the case often, we would have seen this much more
frequent IMHO.

> > @@ -634,8 +666,12 @@ void shrink_dcache_parent(struct dentry 
> >  {
> >  	int found;
> >  
> > + again:
> >  	while ((found = select_parent(parent)) != 0)
> >  		prune_dcache(found);
> > +
> > +	if (wait_on_prunes(parent->d_sb))
> > +		goto again;
> >  }
> 
> Is the goto again required? At this point select_parent() should have pruned
> all entries, except those missed due to the race. These should be captured
> by sb->s_prunes. Once the code comes out of wait_on_prunes() everything
> should be ok since a dput has happened on the missed parent dentries.

Yes, because the last select_parent might returned zero because the parent of
the dentry which is just pruned isn't dereferenced yet. Although we can change
it to something like

   do { 
      while(select_parent())
   } while(wait_on_prunes()) 


> > +++ linux-2.6/include/linux/fs.h
> > @@ -833,6 +833,9 @@ struct super_block {
> >  	struct list_head	s_instances;
> >  	struct quota_info	s_dquot;	/* Diskquota specific options */
> >  
> > +	int			s_prunes;
> 
> Can this be an unsigned int? Perhaps you might to mention that is protected
> by the dcache_lock.
> 

Yes, will fix that.

Regards,
	Jan

-- 
Jan Blunck                                               jblunck@suse.de
SuSE LINUX AG - A Novell company
Maxfeldstr. 5                                          +49-911-74053-608
D-90409 Nürnberg                                      http://www.suse.de