Re: [PATCH] Fix shrink_dcache_parent() against shrink_dcache_memory() race (3rd updated patch)]

[Balbir Singh]

> Hmmm.... yes, I think that could work.  Patch below against
> 2.6.16-rc6-mm1.
>
> I cannot see down_read_trylock being slower than dcache_lock, so I
> suspect this shouldn't impact performance.
>
> Does this meet with everyone's approval?
>

I like the umount logic too, it is very similar to my solution of
down_read_trylock() on the umount semaphore, except that it does not
have the PF_XXXXXXX magic involved.

<snip>

> Signed-off-by: Neil Brown <neilb@xxxxxxx>
>
> ### Diffstat output
>  ./fs/dcache.c |   40 ++++++++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 12 deletions(-)
>
> diff ./fs/dcache.c~current~ ./fs/dcache.c
> --- ./fs/dcache.c~current~      2006-03-13 16:19:29.000000000 +1100
> +++ ./fs/dcache.c       2006-03-13 16:43:59.000000000 +1100
> @@ -383,6 +383,8 @@ static inline void prune_one_dentry(stru
>  /**
>   * prune_dcache - shrink the dcache
>   * @count: number of entries to try and free
> + * @sb: if given, ignore dentries for other superblocks
> + *         which are being unmounted.
>   *
>   * Shrink the dcache. This is done when we need
>   * more memory, or simply when we need to unmount
> @@ -393,7 +395,7 @@ static inline void prune_one_dentry(stru
>   * all the dentries are in use.
>   */
>
> -static void prune_dcache(int count)
> +static void prune_dcache(int count, struct super_block *sb)
>  {
>         spin_lock(&dcache_lock);
>         for (; count ; count--) {
> @@ -420,15 +422,27 @@ static void prune_dcache(int count)
>                         spin_unlock(&dentry->d_lock);
>                         continue;
>                 }
> -               /* 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 if condition please! It is a bit complex and I had to
read it a few times to understand what the condition achieves

> +                       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) {
> +                               prune_one_dentry(dentry);
> +                               up_read(&dentry->d_sb->s_umount);
> +                               continue;
> +                       }

There is a BUG here. What if down_ready_trylock() succeeds and
dentry->d_sb->s_root == NULL? We will be missing an up_read().

<snip>

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