Re: [PATCH] Busy inodes after unmount, be more verbose in generic_shutdown_super

[Neil Brown <neilb@suse.de>]

On Wednesday March 8, balbir@in.ibm.com wrote:
> > So: blending the better bits of various patches together, I've come up
> > with the following.  It still needs a changelog entry, but does anyone
> > want to ACK it ???
> 
> I think this patch is much more cleaner and refined.

Thanks.

> 
> From yesterdays comments I am beginning to wonder if it is enough to solve
> only the unmount race or should the fix be more generic to address the race
> between the shrinkers call to shrink_dcache_memory() and shrink_dcache_parent().
> 
> From what I understand of the race, the race occurs when dput of the
> parent fails to happen and because the referecne count is not 0,
> shrink_dcache_parent() skips over those dentries. The race occurs for
> the dentry and its ancestors above 

I think that in most cases, the race doesn't matter if
shrink_dcache_memory misses a dentry because someone else is holding a
temporary reference, it really doesn't matter.
Similarly most callers of shrink_dcache_parent are happy with a
best-effort.

Unmount is a special case because it wants to 'shrink' *all* of the
dentries for the filesystem.  In that case, someone holding a
transient reference s a bad thing.  In other cases it is, at best, a
minor inconvenience. 

> 
> I was wondering if the following would work (to solve the generic race)
> 
> Add a prune_mutex to the super-block. Hold on to it in prune_one_dentry()
> until we hit a parent dentry that is a mount point (d_mounted > 0) or
> the parent has a reference count > 1 or at the end of prune_one_dentry().
> This should ensure that for each super block dentry counts are consistent.
> Also get select_parent() to hold the super block's prune_mutex, so that it
> sees a consistent view of the super block.
> 
> Oh! now that I think about it, I think your solution looks like an
> elegant way to do the same thing. The only draw back is that it solves
> only the unmount race and there are some changes in generic_shutdown_super()
> which I do not understand.
> 

> > diff ./fs/super.c~current~ ./fs/super.c
> > --- ./fs/super.c~current~	2006-03-08 10:50:37.000000000 +1100
> > +++ ./fs/super.c	2006-03-08 11:02:12.000000000 +1100
> > @@ -81,6 +81,8 @@ static struct super_block *alloc_super(v
> >  		mutex_init(&s->s_dquot.dqio_mutex);
> >  		mutex_init(&s->s_dquot.dqonoff_mutex);
> >  		init_rwsem(&s->s_dquot.dqptr_sem);
> > +		s->s_prunes = 0;
> > +		init_waitqueue_head(&s->s_wait_prunes);
> >  		init_waitqueue_head(&s->s_wait_unfrozen);
> >  		s->s_maxbytes = MAX_NON_LFS;
> >  		s->dq_op = sb_dquot_ops;
> > @@ -231,6 +233,7 @@ void generic_shutdown_super(struct super
> >  
> >  	if (root) {
> >  		sb->s_root = NULL;
> > +		wait_on_prunes(sb);
> >  		shrink_dcache_sb(sb);
> 
> Hmm... in 2.6.16-rc5, I see
> 
> shrink_dcache_parent(root);
> shrink_dcache_anon(&sb->sb_anon);
> 
> without these calls, some dentries might not get moved to LRU list.
> 
> Am I missing something here?

I should have been more explicit that the patch was against
2.6.16-rc5-mm2.  This contains some dcache patches to allow nfs
filesystem to share superblocks, and one of the patches replaces the
calls to shrink_dcache_parent and shrink_dcache_anon with a single
call to a new function: shrink_dcache_sb.

Thanks for the feedback

NeilBrown

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/