Re: [PATCH v2 10/28] dcache: convert to use new lru list infrastructure

[Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>]

On Mon, Apr 08, 2013 at 05:14:44PM +0400, Glauber Costa wrote:
> On 03/29/2013 01:13 PM, Glauber Costa wrote:
> > +	if (dentry->d_flags & DCACHE_REFERENCED) {
> > +		dentry->d_flags &= ~DCACHE_REFERENCED;
> > +		spin_unlock(&dentry->d_lock);
> > +
> > +		/*
> > +		 * XXX: this list move should be be done under d_lock. Need to
> > +		 * determine if it is safe just to do it under the lru lock.
> > +		 */
> > +		return 1;
> > +	}
> 
> I've carefully audited the list manipulations in dcache and determined
> this is safe. I've replaced the fixme string for the following text. Let
> me know if you believe this is not right.

....

> diff --git a/fs/dcache.c b/fs/dcache.c
> index a2fc76e..8e166a4 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -855,8 +855,23 @@ dentry_lru_isolate(struct list_head *item, spinlock_t *lru_lock, void *arg)
>  		spin_unlock(&dentry->d_lock);
>  
>  		/*
> -		 * XXX: this list move should be be done under d_lock. Need to
> -		 * determine if it is safe just to do it under the lru lock.
> +		 * The list move itself will be made by the common LRU code. At
> +		 * this point, we've dropped the dentry->d_lock but keep the
> +		 * lru lock. This is safe to do, since every list movement is
> +		 * protected by the lru lock even if both locks are held.
> +		 *
> +		 * This is guaranteed by the fact that all LRU management
> +		 * functions are intermediated by the LRU API calls like
> +		 * list_lru_add and list_lru_del. List movement in this file
> +		 * only ever occur through this functions or through callbacks
> +		 * like this one, that are called from the LRU API.
> +		 *
> +		 * The only exceptions to this are functions like
> +		 * shrink_dentry_list, and code that first checks for the
> +		 * DCACHE_SHRINK_LIST flag.  Those are guaranteed to be
> +		 * operating only with stack provided lists after they are
> +		 * properly isolated from the main list.  It is thus, always a
> +		 * local access.
>  		 */
>  		return LRU_ROTATE;

It looks correct - I just never got around to doing the audit to
determine it was. Thanks!

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org