list_lru: fix broken LRU_RETRY behaviour

The LRU_RETRY code assumes that the list traversal status after we have dropped and regained the list lock. Unfortunately, this is not a valid assumption, and that can lead to racing traversals isolating objects that the other traversal expects to be the next item on the list. This is causing problems with the inode cache shrinker isolation, with races resulting in an inode on a dispose list being "isolated" because a racing traversal still thinks it is on the LRU. The inode is then never reclaimed and that causes hangs if a subsequent lookup on that inode occurs. Fix it by always restarting the list walk on a LRU_RETRY return from the isolate callback. Avoid the possibility of livelocks the current code was trying to avoid by always decrementing the nr_to_walk counter on retries so that even if we keep hitting the same item on the list we'll eventually stop trying to walk and exit out of the situation causing the problem. Reported-by: Michal Hocko <mhocko@suse.cz> Signed-off-by: Dave Chinner <dchinner@redhat.com> Cc: Glauber Costa <glommer@gmail.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

list_lru: fix broken LRU_RETRY behaviour
The LRU_RETRY code assumes that the list traversal status after we have dropped and regained the list lock. Unfortunately, this is not a valid assumption, and that can lead to racing traversals isolating objects that the other traversal expects to be the next item on the list. This is causing problems with the inode cache shrinker isolation, with races resulting in an inode on a dispose list being "isolated" because a racing traversal still thinks it is on the LRU. The inode is then never reclaimed and that causes hangs if a subsequent lookup on that inode occurs. Fix it by always restarting the list walk on a LRU_RETRY return from the isolate callback. Avoid the possibility of livelocks the current code was trying to avoid by always decrementing the nr_to_walk counter on retries so that even if we keep hitting the same item on the list we'll eventually stop trying to walk and exit out of the situation causing the problem. Reported-by: Michal Hocko <mhocko@suse.cz> Signed-off-by: Dave Chinner <dchinner@redhat.com> Cc: Glauber Costa <glommer@gmail.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Dave Chinner · Al Viro
1 parent 3b1d58a4c9
Showing 1 changed file with 12 additions and 17 deletions Side-by-side Diff
mm/list_lru.c
@@ -73,19 +73,19 @@
 	struct list_lru_node	*nlru = &lru->node[nid];
 	struct list_head *item, *n;
 	unsigned long isolated = 0;
-	/*
-	 * If we don't keep state of at which pass we are, we can loop at
-	 * LRU_RETRY, since we have no guarantees that the caller will be able
-	 * to do something other than retry on the next pass. We handle this by
-	 * allowing at most one retry per object. This should not be altered
-	 * by any condition other than LRU_RETRY.
-	 */
-	bool first_pass = true;
  
 	spin_lock(&nlru->lock);
 restart:
 	list_for_each_safe(item, n, &nlru->list) {
 		enum lru_status ret;
+
+		/*
+		 * decrement nr_to_walk first so that we don't livelock if we
+		 * get stuck on large numbesr of LRU_RETRY items
+		 */
+		if (--(*nr_to_walk) == 0)
+			break;
+
 		ret = isolate(item, &nlru->lock, cb_arg);
 		switch (ret) {
 		case LRU_REMOVED:
  
@@ -100,19 +100,14 @@
 		case LRU_SKIP:
 			break;
 		case LRU_RETRY:
-			if (!first_pass) {
-				first_pass = true;
-				break;
-			}
-			first_pass = false;
+			/*
+			 * The lru lock has been dropped, our list traversal is
+			 * now invalid and so we have to restart from scratch.
+			 */
 			goto restart;
 		default:
 			BUG();
 		}
-
-		if ((*nr_to_walk)-- == 0)
-			break;
-
 	}
  
 	spin_unlock(&nlru->lock);
...	...	@@ -73,19 +73,19 @@
73	73	struct list_lru_node *nlru = &lru->node[nid];
74	74	struct list_head item, n;
75	75	unsigned long isolated = 0;
76		- /*
77		- * If we don't keep state of at which pass we are, we can loop at
78		- * LRU_RETRY, since we have no guarantees that the caller will be able
79		- * to do something other than retry on the next pass. We handle this by
80		- * allowing at most one retry per object. This should not be altered
81		- * by any condition other than LRU_RETRY.
82		- */
83		- bool first_pass = true;
84	76
85	77	spin_lock(&nlru->lock);
86	78	restart:
87	79	list_for_each_safe(item, n, &nlru->list) {
88	80	enum lru_status ret;
	81	+
	82	+ /*
	83	+ * decrement nr_to_walk first so that we don't livelock if we
	84	+ * get stuck on large numbesr of LRU_RETRY items
	85	+ */
	86	+ if (--(*nr_to_walk) == 0)
	87	+ break;
	88	+
89	89	ret = isolate(item, &nlru->lock, cb_arg);
90	90	switch (ret) {
91	91	case LRU_REMOVED:
92	92
...	...	@@ -100,19 +100,14 @@
100	100	case LRU_SKIP:
101	101	break;
102	102	case LRU_RETRY:
103		- if (!first_pass) {
104		- first_pass = true;
105		- break;
106		- }
107		- first_pass = false;
	103	+ /*
	104	+ * The lru lock has been dropped, our list traversal is
	105	+ * now invalid and so we have to restart from scratch.
	106	+ */
108	107	goto restart;
109	108	default:
110	109	BUG();
111	110	}
112		-
113		- if ((*nr_to_walk)-- == 0)
114		- break;
115		-
116	111	}
117	112
118	113	spin_unlock(&nlru->lock);