Re: [PATCH 11/46] fs: dcache scale hash

[Dave Chinner <david@fromorbit.com>]

On Sat, Nov 27, 2010 at 08:44:41PM +1100, Nick Piggin wrote:
> Add a new lock, dcache_hash_lock, to protect the dcache hash table from
> concurrent modification. d_hash is also protected by d_lock.
> 
> Signed-off-by: Nick Piggin <npiggin@kernel.dk>
> ---
>  fs/dcache.c            |   38 +++++++++++++++++++++++++++-----------
>  include/linux/dcache.h |    3 +++
>  2 files changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 4f9ccbe..50c65c7 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -35,12 +35,27 @@
>  #include <linux/hardirq.h>
>  #include "internal.h"
>  
> +/*
> + * Usage:
> + * dcache_hash_lock protects dcache hash table
> + *
> + * Ordering:
> + * dcache_lock
> + *   dentry->d_lock
> + *     dcache_hash_lock
> + *

What locking is used to keep DCACHE_UNHASHED/d_unhashed() in check
with the whether the dentry is on the hash list or not? It looks to
me that to make any hash modification, you have to hold both the
dentry->d_lock and the dcache_hash_lock to keep them in step. If
this is correct, can you add this to the comments above?

> + * if (dentry1 < dentry2)
> + *   dentry1->d_lock
> + *     dentry2->d_lock
> + */

Perhaps the places where we need to lock two dentries should use a
wrapper like we do for other objects. Such as:

void dentry_dlock_two(struct dentry *d1, struct dentry *d2)
{
	if (d1 < d2) {
		spin_lock(&d1->d_lock);
		spin_lock_nested(&d2->d_lock, DENTRY_D_LOCK_NESTED);
	} else {
		spin_lock(&d2->d_lock);
		spin_lock_nested(&d1->d_lock, DENTRY_D_LOCK_NESTED);
	}
}

> @@ -1581,7 +1598,9 @@ void d_rehash(struct dentry * entry)
>  {
>  	spin_lock(&dcache_lock);
>  	spin_lock(&entry->d_lock);
> +	spin_lock(&dcache_hash_lock);
>  	_d_rehash(entry);
> +	spin_unlock(&dcache_hash_lock);
>  	spin_unlock(&entry->d_lock);
>  	spin_unlock(&dcache_lock);
>  }

Shouldn't we really kill _d_rehash() by replacing all the callers
with direct calls to __d_rehash() first? There doesn't seem to be much
sense to keep both methods around....

> @@ -1661,8 +1680,6 @@ static void switch_names(struct dentry *dentry, struct dentry *target)
>   */
>  static void d_move_locked(struct dentry * dentry, struct dentry * target)
>  {
> -	struct hlist_head *list;
> -
>  	if (!dentry->d_inode)
>  		printk(KERN_WARNING "VFS: moving negative dcache entry\n");
>  
> @@ -1679,14 +1696,11 @@ static void d_move_locked(struct dentry * dentry, struct dentry * target)
>  	}
>  
>  	/* Move the dentry to the target hash queue, if on different bucket */
> -	if (d_unhashed(dentry))
> -		goto already_unhashed;
> -
> -	hlist_del_rcu(&dentry->d_hash);
> -
> -already_unhashed:
> -	list = d_hash(target->d_parent, target->d_name.hash);
> -	__d_rehash(dentry, list);
> +	spin_lock(&dcache_hash_lock);
> +	if (!d_unhashed(dentry))
> +		hlist_del_rcu(&dentry->d_hash);
> +	__d_rehash(dentry, d_hash(target->d_parent, target->d_name.hash));
> +	spin_unlock(&dcache_hash_lock);
>  
>  	/* Unhash the target: dput() will then get rid of it */
>  	__d_drop(target);
> @@ -1883,7 +1897,9 @@ struct dentry *d_materialise_unique(struct dentry *dentry, struct inode *inode)
>  found_lock:
>  	spin_lock(&actual->d_lock);
>  found:
> +	spin_lock(&dcache_hash_lock);
>  	_d_rehash(actual);
> +	spin_unlock(&dcache_hash_lock);
>  	spin_unlock(&actual->d_lock);
>  	spin_unlock(&dcache_lock);
>  out_nolock:
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index 6b5760b..7ce20f5 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -181,6 +181,7 @@ struct dentry_operations {
>  
>  #define DCACHE_CANT_MOUNT	0x0100
>  
> +extern spinlock_t dcache_hash_lock;
>  extern spinlock_t dcache_lock;
>  extern seqlock_t rename_lock;
>  
> @@ -204,7 +205,9 @@ static inline void __d_drop(struct dentry *dentry)
>  {
>  	if (!(dentry->d_flags & DCACHE_UNHASHED)) {
>  		dentry->d_flags |= DCACHE_UNHASHED;
> +		spin_lock(&dcache_hash_lock);
>  		hlist_del_rcu(&dentry->d_hash);
> +		spin_unlock(&dcache_hash_lock);
>  	}
>  }

Un-inline __d_drop so you don't need to make the dcache_hash_lock
visible outside of fs/dcache.c. That happens later in the series
anyway, so may as well do it now...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com