Messages in this thread | | | Date | Fri, 10 Mar 2006 11:28:39 +0300 | From | Kirill Korotaev <> | Subject | Re: [PATCH] Fix shrink_dcache_parent() against shrink_dcache_memory() race (3rd updated patch)] |
| |
Neil,
> On Thursday March 9, dev@openvz.org wrote: > >>Andrew, >> >>Acked-By: Kirill Korotaev <dev@openvz.org> > > > I'm afraid that I'm not convinced. > > >>>+static int wait_on_prunes(struct super_block *sb) >>>+{ >>>+ DEFINE_WAIT(wait); >>>+ int prunes_remaining = 0; >>>+ >>>+#ifdef DCACHE_DEBUG >>>+ printk(KERN_DEBUG "%s: waiting for %d prunes\n", __FUNCTION__, >>>+ sb->s_prunes); >>>+#endif >>>+ >>>+ spin_lock(&dcache_lock); >>>+ for (;;) { >>>+ prepare_to_wait(&sb->s_wait_prunes, &wait, >>>+ TASK_UNINTERRUPTIBLE); >>>+ if (!sb->s_prunes) >>>+ break; >>>+ spin_unlock(&dcache_lock); >>>+ schedule(); >>>+ prunes_remaining = 1; >>>+ spin_lock(&dcache_lock); >>>+ } >>>+ spin_unlock(&dcache_lock); >>>+ finish_wait(&sb->s_wait_prunes, &wait); >>>+ return prunes_remaining; >>>+} > > > I don't think that a return value from wait_on_prunes is meaningful. > All it tells us is whether a prune_one_dentry finished before or after > wait_on_prunes takes the spin_lock. This isn't very useful > information as it has no significance to upper levels. > > So: > > >>>+ do { >>>+ shrink_dcache_parent(root); >>>+ } while(wait_on_prunes(sb)); >>>+ > > > Suppose shrink_dcache_parent misses on dentry because the inode was being > iput. This iput completes immediately that > shrink_dcache_parent completes. It decrements ->s_prunes and when > wait_on_prunes takes dcache_lock, ->s_prunes is zero so the loop > terminates, and the remaining dentries - the parents of the dentry > what was undergoing iput - don't get put. you are right... :/ And this is actually why we originally inserted check for race in select_parent() under dcache_lock... I just lost my memory :(
> I really think that we need to stop prune_one_dentry from being called > on dentries for a filesystem that is being unmounted. With that code > currently in -git, that means passing a 'struct super_block *' into > prune_dcache so that it ignores any filesystem with ->s_root==NULL > unless that filesystem is the filesystem that was passed. Can try...
Thanks, Kirill
- 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/
| |