lkml.org 
[lkml]   [2006]   [Mar]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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/

    \
     
     \ /
      Last update: 2006-03-10 09:26    [W:0.035 / U:119.172 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site