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

[Glauber Costa <glommer@parallels.com>]

[-- Attachment #1: Type: text/plain, Size: 536 bytes --]

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.



[-- Attachment #2: comment --]
[-- Type: text/plain, Size: 1302 bytes --]

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;
 	}