Messages in this thread | | | Date | Mon, 3 Apr 2006 23:42:03 +0530 | From | "Balbir Singh" <> | Subject | Re: [PATCH] Fix dcache race during umount |
| |
Hi, Neil,
> > Cc: Jan Blunck <jblunck@suse.de> > Cc: Kirill Korotaev <dev@openvz.org> > Cc: olh@suse.de > Cc: bsingharora@gmail.com
Could you please make this balbir@in.ibm.com
> > Signed-off-by: Neil Brown <neilb@suse.de> > <snip> - /* If the dentry was recently referenced, don't free it. */ - if (dentry->d_flags & DCACHE_REFERENCED) { - dentry->d_flags &= ~DCACHE_REFERENCED; - list_add(&dentry->d_lru, &dentry_unused); - dentry_stat.nr_unused++; - spin_unlock(&dentry->d_lock); - continue; + if (!(dentry->d_flags & DCACHE_REFERENCED) && + (!sb || dentry->d_sb == sb)) {
Comments for the condition please. Something like
/* * If the dentry is not DCACHED_REFERENCED, it is time to move it to LRU list, * provided the super block is NULL (which means we are trying to reclaim memory * or this dentry belongs to the same super block that we want to shrink. */
One side-effect of this check I see is
Earlier, all prune_dcache() calls would prune the dentry cache. This condition will cause dentries belonging only those super blocks being shrink'ed to be freed up. shrink_dcache_memory() will have to do the additional work of freeing dentries (especially for file systems like sysfs, procfs, etc). But the good thing is it should make the per super block operations faster (like unmount). IMO, this is the correct behaviour, but I am not sure of the side-effects.
+ if (sb) { + prune_one_dentry(dentry); + continue; + } > + /* Need to avoid race with generic_shutdown_super */ > + if (down_read_trylock(&dentry->d_sb->s_umount) && > + dentry->d_sb->s_root != NULL) {
There is a probable bug here. What if down_read_trylock() succeeds and dentry->d_sb->s_root == NULL? We still need to do an up_read before we move on. The comment would be better put as
/* * If we are able to acquire the umount semaphore, then the super block cannot be unmounted * while we are pruning this dentry. This helps avoid a race condition that is caused due to * intermediate reference counts held by the children of the dentry in prune_one_dentry(). * This leads to select_dcache_parent() ignoring those dentries, leaving behind non-dput * dentries. The unmount happens before prune_one_dentry() can dput the dentries. */
> + prune_one_dentry(dentry); > + up_read(&dentry->d_sb->s_umount); > + continue; > + } > } <snip>
Looks good to go, except for the comments (especially the up_read() bug)
Balbir - 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/
| |