Deadlock possibly caused by too_many_isolated.

Index: linux-2.6.32-SLE11-SP1/mm/vmscan.c
===================================================================

mm/vmscan.c +0 -0

@@ -1106,6 +1106,11 @@ static unsigned long shrink_inactive_lis
  		/* We are about to die and free our memory. Return now. */

Quoted 15 lines · Neil Brown

Index: linux-2.6.32-SLE11-SP1/mm/vmscan.c
===================================================================

mm/vmscan.c +5 -0

@@ -1106,6 +1106,11 @@ static unsigned long shrink_inactive_lis
  		/* We are about to die and free our memory. Return now. */
  		if (fatal_signal_pending(current))
  			return SWAP_CLUSTER_MAX;
+		if (!(sc->gfp_mask&  __GFP_IO))
+			/* Not allowed to do IO, so mustn't wait
+			 * on processes that might try to
+			 */
+			return SWAP_CLUSTER_MAX;
  	}
 

mm/vmscan.c static unsigned long shrink_inactive_lis · line 1116

Quoted 1 line

mm/vmscan.c +0 -0

  	/*

Close. We must also be sure that processes without __GFP_FS set in their gfp_mask do not wait on processes that do have __GFP_FS set.

Considering how many times we've run into a bug like this, I'm kicking myself for not having thought of it :(

Quoted 23 lines · On Tue, 14 Sep 2010 20:30:18 -0400 Rik van Riel <[email protected]> wrote:

Quoted 6 lines · Neil Brown

Index: linux-2.6.32-SLE11-SP1/mm/vmscan.c
===================================================================

mm/vmscan.c +0 -0

@@ -1106,6 +1106,11 @@ static unsigned long shrink_inactive_lis
  		/* We are about to die and free our memory. Return now. */

mm/vmscan.c static unsigned long shrink_inactive_lis · line 1116

Quoted 10 lines

mm/vmscan.c +5 -0

  		if (fatal_signal_pending(current))
  			return SWAP_CLUSTER_MAX;
+		if (!(sc->gfp_mask&  __GFP_IO))
+			/* Not allowed to do IO, so mustn't wait
+			 * on processes that might try to
+			 */
+			return SWAP_CLUSTER_MAX;
  	}
 
  	/*

Close. We must also be sure that processes without __GFP_FS set in their gfp_mask do not wait on processes that do have __GFP_FS set.

Quoted 3 lines

Considering how many times we've run into a bug like this, I'm kicking myself for not having thought of it :(

So maybe this? I've added the test for __GFP_FS, and moved the test before the congestion_wait on the basis that we really want to get back up the stack and try the mempool ASAP.

From: NeilBrown <[email protected]>

mm: Avoid possible deadlock caused by too_many_isolated()

If too_many_isolated() returns true while performing direct reclaim we can end up waiting for other threads to complete their direct reclaim. If those threads are allowed to enter the FS or IO to free memory, but this thread is not, then it is possible that those threads will be waiting on this thread and so we get a circular deadlock.

So: if too_many_isolated() returns true when the allocation did not permit FS or IO, fail shrink_inactive_list rather than blocking.

Signed-off-by NeilBrown <[email protected]>

mm/vmscan.c +0 -0

@@ -1101,6 +1101,12 @@ static unsigned long shrink_inactive_lis
 	int lumpy_reclaim = 0;
 

Quoted 26 lines · Neil Brown

Quoted 23 lines · On Tue, 14 Sep 2010 20:30:18 -0400 Rik van Riel <[email protected]> wrote:

Quoted 6 lines · Neil Brown

Index: linux-2.6.32-SLE11-SP1/mm/vmscan.c
===================================================================

mm/vmscan.c +0 -0

@@ -1106,6 +1106,11 @@ static unsigned long shrink_inactive_lis
  		/* We are about to die and free our memory. Return now. */

mm/vmscan.c static unsigned long shrink_inactive_lis · line 1116

Quoted 10 lines

mm/vmscan.c +5 -0

  		if (fatal_signal_pending(current))
  			return SWAP_CLUSTER_MAX;
+		if (!(sc->gfp_mask&  __GFP_IO))
+			/* Not allowed to do IO, so mustn't wait
+			 * on processes that might try to
+			 */
+			return SWAP_CLUSTER_MAX;
  	}
 
  	/*

Close. We must also be sure that processes without __GFP_FS set in their gfp_mask do not wait on processes that do have __GFP_FS set.

Quoted 7 lines

Quoted 3 lines

Considering how many times we've run into a bug like this, I'm kicking myself for not having thought of it :(

So maybe this? I've added the test for __GFP_FS, and moved the test before the congestion_wait on the basis that we really want to get back up the stack and try the mempool ASAP.

The patch may well fail the !__GFP_IO page allocation and then quickly exhaust the mempool.

Another approach may to let too_many_isolated() use much higher thresholds for !__GFP_IO/FS and lower ones for __GFP_IO/FS. ie. to allow at least nr2 NOIO/FS tasks to be blocked independent of the IO/FS ones. Since NOIO vmscans typically completes fast, it will then very hard to accumulate enough NOIO processes to be actually blocked.

                  IO/FS tasks                NOIO/FS tasks           full
                  block here                 block here              LRU size
|-----------------|--------------------------|-----------------------|
|      nr1        |           nr2            |

Quoted 45 lines · Wu Fengguang

Quoted 26 lines · Neil Brown

Quoted 23 lines · On Tue, 14 Sep 2010 20:30:18 -0400 Rik van Riel <[email protected]> wrote:

Quoted 6 lines · Neil Brown

Index: linux-2.6.32-SLE11-SP1/mm/vmscan.c
===================================================================

mm/vmscan.c +0 -0

@@ -1106,6 +1106,11 @@ static unsigned long shrink_inactive_lis
  		/* We are about to die and free our memory. Return now. */

mm/vmscan.c static unsigned long shrink_inactive_lis · line 1116

Quoted 10 lines

mm/vmscan.c +5 -0

  		if (fatal_signal_pending(current))
  			return SWAP_CLUSTER_MAX;
+		if (!(sc->gfp_mask&  __GFP_IO))
+			/* Not allowed to do IO, so mustn't wait
+			 * on processes that might try to
+			 */
+			return SWAP_CLUSTER_MAX;
  	}
 
  	/*

Close. We must also be sure that processes without __GFP_FS set in their gfp_mask do not wait on processes that do have __GFP_FS set.

Quoted 7 lines

Quoted 3 lines

Considering how many times we've run into a bug like this, I'm kicking myself for not having thought of it :(

So maybe this? I've added the test for __GFP_FS, and moved the test before the congestion_wait on the basis that we really want to get back up the stack and try the mempool ASAP.

The patch may well fail the !__GFP_IO page allocation and then quickly exhaust the mempool.

Another approach may to let too_many_isolated() use much higher thresholds for !__GFP_IO/FS and lower ones for __GFP_IO/FS. ie. to allow at least nr2 NOIO/FS tasks to be blocked independent of the IO/FS ones. Since NOIO vmscans typically completes fast, it will then very hard to accumulate enough NOIO processes to be actually blocked.

Quoted 4 lines

                  IO/FS tasks                NOIO/FS tasks           full
                  block here                 block here              LRU size
|-----------------|--------------------------|-----------------------|
|      nr1        |           nr2            |

How about this fix? We may need very high threshold for NOIO/NOFS to prevent possible regressions.

Quoted 51 lines · Wu Fengguang

Quoted 49 lines · Wu Fengguang

Quoted 26 lines · Neil Brown

Quoted 23 lines · On Tue, 14 Sep 2010 20:30:18 -0400 Rik van Riel <[email protected]> wrote:

Quoted 6 lines · Neil Brown

Index: linux-2.6.32-SLE11-SP1/mm/vmscan.c
===================================================================

mm/vmscan.c +0 -0

@@ -1106,6 +1106,11 @@ static unsigned long shrink_inactive_lis
  		/* We are about to die and free our memory. Return now. */

mm/vmscan.c static unsigned long shrink_inactive_lis · line 1116

Quoted 10 lines

mm/vmscan.c +5 -0

  		if (fatal_signal_pending(current))
  			return SWAP_CLUSTER_MAX;
+		if (!(sc->gfp_mask&  __GFP_IO))
+			/* Not allowed to do IO, so mustn't wait
+			 * on processes that might try to
+			 */
+			return SWAP_CLUSTER_MAX;
  	}
 
  	/*

Close. We must also be sure that processes without __GFP_FS set in their gfp_mask do not wait on processes that do have __GFP_FS set.

Quoted 7 lines

Quoted 3 lines

Considering how many times we've run into a bug like this, I'm kicking myself for not having thought of it :(

So maybe this? I've added the test for __GFP_FS, and moved the test before the congestion_wait on the basis that we really want to get back up the stack and try the mempool ASAP.

The patch may well fail the !__GFP_IO page allocation and then quickly exhaust the mempool.

Another approach may to let too_many_isolated() use much higher thresholds for !__GFP_IO/FS and lower ones for __GFP_IO/FS. ie. to allow at least nr2 NOIO/FS tasks to be blocked independent of the IO/FS ones. Since NOIO vmscans typically completes fast, it will then very hard to accumulate enough NOIO processes to be actually blocked.

                  IO/FS tasks                NOIO/FS tasks           full
                  block here                 block here              LRU size
|-----------------|--------------------------|-----------------------|
|      nr1        |           nr2            |

Quoted 2 lines

How about this fix? We may need very high threshold for NOIO/NOFS to prevent possible regressions.

Plus __GFP_WAIT..

Quoted 52 lines · Wu Fengguang

Quoted 51 lines · Wu Fengguang

Quoted 49 lines · Wu Fengguang

Quoted 26 lines · Neil Brown

Quoted 23 lines · On Tue, 14 Sep 2010 20:30:18 -0400 Rik van Riel <[email protected]> wrote:

Quoted 6 lines · Neil Brown

Index: linux-2.6.32-SLE11-SP1/mm/vmscan.c
===================================================================

mm/vmscan.c +0 -0

@@ -1106,6 +1106,11 @@ static unsigned long shrink_inactive_lis
  		/* We are about to die and free our memory. Return now. */

mm/vmscan.c static unsigned long shrink_inactive_lis · line 1116

Quoted 10 lines

mm/vmscan.c +5 -0

  		if (fatal_signal_pending(current))
  			return SWAP_CLUSTER_MAX;
+		if (!(sc->gfp_mask&  __GFP_IO))
+			/* Not allowed to do IO, so mustn't wait
+			 * on processes that might try to
+			 */
+			return SWAP_CLUSTER_MAX;
  	}
 
  	/*

Close. We must also be sure that processes without __GFP_FS set in their gfp_mask do not wait on processes that do have __GFP_FS set.

Quoted 7 lines

Quoted 3 lines

Considering how many times we've run into a bug like this, I'm kicking myself for not having thought of it :(

So maybe this? I've added the test for __GFP_FS, and moved the test before the congestion_wait on the basis that we really want to get back up the stack and try the mempool ASAP.

The patch may well fail the !__GFP_IO page allocation and then quickly exhaust the mempool.

Another approach may to let too_many_isolated() use much higher thresholds for !__GFP_IO/FS and lower ones for __GFP_IO/FS. ie. to allow at least nr2 NOIO/FS tasks to be blocked independent of the IO/FS ones. Since NOIO vmscans typically completes fast, it will then very hard to accumulate enough NOIO processes to be actually blocked.

                  IO/FS tasks                NOIO/FS tasks           full
                  block here                 block here              LRU size
|-----------------|--------------------------|-----------------------|
|      nr1        |           nr2            |

Quoted 4 lines

Quoted 2 lines

How about this fix? We may need very high threshold for NOIO/NOFS to prevent possible regressions.

Plus __GFP_WAIT..

Ah sorry! __GFP_WAIT cannot afford to wait by definition..

---

mm/vmscan.c +0 -0

index 225a759..becc63a 100644
@@ -1135,10 +1135,14 @@ static int too_many_isolated(struct zone *zone, int file,
 		struct scan_control *sc)

Review

Quoted 3 lines

(quoted diff) +3 -0

+	if (!(sc->gfp_mask & __GFP_WAIT))
+		return 0;
+

it appears __GFP_WAIT allocation doesn't go to direct reclaim.

Quoted 46 lines · Wu Fengguang

Quoted 45 lines · Wu Fengguang

Quoted 26 lines · Neil Brown

Quoted 23 lines · On Tue, 14 Sep 2010 20:30:18 -0400 Rik van Riel <[email protected]> wrote:

Quoted 6 lines · Neil Brown

Index: linux-2.6.32-SLE11-SP1/mm/vmscan.c
===================================================================

mm/vmscan.c +0 -0

@@ -1106,6 +1106,11 @@ static unsigned long shrink_inactive_lis
  		/* We are about to die and free our memory. Return now. */

mm/vmscan.c static unsigned long shrink_inactive_lis · line 1116

Quoted 10 lines

mm/vmscan.c +5 -0

  		if (fatal_signal_pending(current))
  			return SWAP_CLUSTER_MAX;
+		if (!(sc->gfp_mask&  __GFP_IO))
+			/* Not allowed to do IO, so mustn't wait
+			 * on processes that might try to
+			 */
+			return SWAP_CLUSTER_MAX;
  	}
 
  	/*

Close. We must also be sure that processes without __GFP_FS set in their gfp_mask do not wait on processes that do have __GFP_FS set.

Quoted 7 lines

Quoted 3 lines

Considering how many times we've run into a bug like this, I'm kicking myself for not having thought of it :(

So maybe this? I've added the test for __GFP_FS, and moved the test before the congestion_wait on the basis that we really want to get back up the stack and try the mempool ASAP.

The patch may well fail the !__GFP_IO page allocation and then quickly exhaust the mempool.

Another approach may to let too_many_isolated() use much higher thresholds for !__GFP_IO/FS and lower ones for __GFP_IO/FS. ie. to allow at least nr2 NOIO/FS tasks to be blocked independent of the IO/FS ones. Since NOIO vmscans typically completes fast, it will then very hard to accumulate enough NOIO processes to be actually blocked.

Quoted 7 lines

Quoted 4 lines

                  IO/FS tasks                NOIO/FS tasks           full
                  block here                 block here              LRU size
|-----------------|--------------------------|-----------------------|
|      nr1        |           nr2            |

How about this fix? We may need very high threshold for NOIO/NOFS to prevent possible regressions.

Plus __GFP_WAIT..

---

mm/vmscan.c +3 -1

index 225a759..6a896eb 100644
@@ -1135,6 +1135,7 @@ static int too_many_isolated(struct zone *zone, int file,
 		struct scan_control *sc)
 {
 	unsigned long inactive, isolated;
+	int ratio;
 
 	if (current_is_kswapd())
 		return 0;

@@ -1150,7 +1151,15 @@ static int too_many_isolated(struct zone *zone, int file,
 		isolated = zone_page_state(zone, NR_ISOLATED_ANON);
 	}
 
-	return isolated > inactive;
+	ratio = 1;
+	if (!(sc->gfp_mask & (__GFP_FS)))

Quoted 86 lines · On Wed, 15 Sep 2010 11:06:40 +0800 Wu Fengguang <[email protected]> wrote:

Quoted 46 lines · Wu Fengguang

Quoted 45 lines · Wu Fengguang

Quoted 26 lines · Neil Brown

Quoted 23 lines · On Tue, 14 Sep 2010 20:30:18 -0400 Rik van Riel <[email protected]> wrote:

Quoted 6 lines · Neil Brown

Index: linux-2.6.32-SLE11-SP1/mm/vmscan.c
===================================================================

mm/vmscan.c +0 -0

@@ -1106,6 +1106,11 @@ static unsigned long shrink_inactive_lis
  		/* We are about to die and free our memory. Return now. */

mm/vmscan.c static unsigned long shrink_inactive_lis · line 1116

Quoted 10 lines

mm/vmscan.c +5 -0

  		if (fatal_signal_pending(current))
  			return SWAP_CLUSTER_MAX;
+		if (!(sc->gfp_mask&  __GFP_IO))
+			/* Not allowed to do IO, so mustn't wait
+			 * on processes that might try to
+			 */
+			return SWAP_CLUSTER_MAX;
  	}
 
  	/*

Close. We must also be sure that processes without __GFP_FS set in their gfp_mask do not wait on processes that do have __GFP_FS set.

Quoted 7 lines

Quoted 3 lines

Considering how many times we've run into a bug like this, I'm kicking myself for not having thought of it :(

So maybe this? I've added the test for __GFP_FS, and moved the test before the congestion_wait on the basis that we really want to get back up the stack and try the mempool ASAP.

The patch may well fail the !__GFP_IO page allocation and then quickly exhaust the mempool.

Another approach may to let too_many_isolated() use much higher thresholds for !__GFP_IO/FS and lower ones for __GFP_IO/FS. ie. to allow at least nr2 NOIO/FS tasks to be blocked independent of the IO/FS ones. Since NOIO vmscans typically completes fast, it will then very hard to accumulate enough NOIO processes to be actually blocked.

Quoted 7 lines

Quoted 4 lines

                  IO/FS tasks                NOIO/FS tasks           full
                  block here                 block here              LRU size
|-----------------|--------------------------|-----------------------|
|      nr1        |           nr2            |

How about this fix? We may need very high threshold for NOIO/NOFS to prevent possible regressions.

Plus __GFP_WAIT..

---

mm/vmscan.c +10 -1

index 225a759..6a896eb 100644
@@ -1135,6 +1135,7 @@ static int too_many_isolated(struct zone *zone, int file,
 		struct scan_control *sc)
 {
 	unsigned long inactive, isolated;
+	int ratio;
 
 	if (current_is_kswapd())
 		return 0;

@@ -1150,7 +1151,15 @@ static int too_many_isolated(struct zone *zone, int file,
 		isolated = zone_page_state(zone, NR_ISOLATED_ANON);
 	}
 
-	return isolated > inactive;
+	ratio = 1;
+	if (!(sc->gfp_mask & (__GFP_FS)))
+		ratio <<= 1;
+	if (!(sc->gfp_mask & (__GFP_IO)))
+		ratio <<= 1;
+	if (!(sc->gfp_mask & (__GFP_WAIT)))
+		ratio <<= 1;
+
+	return isolated > inactive * ratio;
 }
 

mm/vmscan.c static int too_many_isolated(struct zone *zone, int file, · line 1165

Quoted 1 line

mm/vmscan.c +0 -0

 /*

Are you suggesting this instead of my patch, or as well as my patch?

Because while I think it sounds like a good idea I don't think it actually removes the chance of a deadlock, just makes it a lot less likely. So I think your patch combined with my patch would be a good total solution.

Neil,

Sorry for the rushed and imaginary ideas this morning..

Review static unsigned long shrink_inactive_lis · line 1109

Quoted 10 lines

(quoted diff) +6 -0

@@ -1101,6 +1101,12 @@ static unsigned long shrink_inactive_lis
 	int lumpy_reclaim = 0;
 
 	while (unlikely(too_many_isolated(zone, file, sc))) {
+		if ((sc->gfp_mask & GFP_IOFS) != GFP_IOFS)
+			/* Not allowed to do IO, so mustn't wait
+			 * on processes that might try to
+			 */
+			return SWAP_CLUSTER_MAX;
+

The above patch should behavior like this: it returns SWAP_CLUSTER_MAX to cheat all the way up to believe "enough pages have been reclaimed". So __alloc_pages_direct_reclaim() see non-zero *did_some_progress and go on to call get_page_from_freelist(). That normally fails because the task didn't really scanned the LRU lists. However it does have the possibility to succeed -- when so many processes are doing concurrent direct reclaims, it may luckily get one free page reclaimed by other tasks. What's more, if it does fail to get a free page, the upper layer __alloc_pages_slowpath() will be repeat recalling __alloc_pages_direct_reclaim(). So, sooner or later it will succeed in "stealing" a free page reclaimed by other tasks.

In summary, the patch behavior for !__GFP_IO/FS is
- won't do any page reclaim
- won't fail the page allocation (unexpected)
- will wait and steal one free page from others (unreasonable)

So it will address the problem you encountered, however it sounds pretty unexpected and illogical behavior, right?

I believe this patch will address the problem equally well. What do you think?

Thanks,
Fengguang
---

mm: Avoid possible deadlock caused by too_many_isolated()

Neil finds that if too_many_isolated() returns true while performing direct reclaim we can end up waiting for other threads to complete their direct reclaim. If those threads are allowed to enter the FS or IO to free memory, but this thread is not, then it is possible that those threads will be waiting on this thread and so we get a circular deadlock.

some task enters direct reclaim with GFP_KERNEL
=> too_many_isolated() false
=> vmscan and run into dirty pages
=> pageout()
=> take some FS lock
=> fs/block code does GFP_NOIO allocation
=> enter direct reclaim again
=> too_many_isolated() true
=> waiting for others to progress, however the other
tasks may be circular waiting for the FS lock..

The fix is to let !__GFP_IO and !__GFP_FS direct reclaims enjoy higher priority than normal ones, by honouring them higher throttle threshold.

Now !__GFP_IO/FS reclaims won't be waiting for __GFP_IO/FS reclaims to progress. They will be blocked only when there are too many concurrent !__GFP_IO/FS reclaims, however that's very unlikely because the IO-less direct reclaims is able to progress much more faster, and they won't deadlock each other. The threshold is raised high enough for them, so that there can be sufficient parallel progress of !__GFP_IO/FS reclaims.

Reported-by NeilBrown <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 mm/vmscan.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

mm/vmscan.c +4 -1

@@ -1141,36 +1141,39 @@ int isolate_lru_page(struct page *page)
 	return ret;
 }
 
 /*
  * Are there way too many processes in the direct reclaim path already?
  */
 static int too_many_isolated(struct zone *zone, int file,
 		struct scan_control *sc)
 {
 	unsigned long inactive, isolated;
+	int ratio;
 
 	if (current_is_kswapd())
 		return 0;
 
 	if (!scanning_global_lru(sc))
 		return 0;
 
 	if (file) {
 		inactive = zone_page_state(zone, NR_INACTIVE_FILE);
 		isolated = zone_page_state(zone, NR_ISOLATED_FILE);
 	} else {
 		inactive = zone_page_state(zone, NR_INACTIVE_ANON);
 		isolated = zone_page_state(zone, NR_ISOLATED_ANON);
 	}
 
-	return isolated > inactive;
+	ratio = sc->gfp_mask & (__GFP_IO | __GFP_FS) ? 1 : 8;
+
+	return isolated > inactive * ratio;
 }
 
 /*
  * TODO: Try merging with migrations version of putback_lru_pages
  */
 static noinline_for_stack void
 putback_lru_pages(struct zone *zone, struct scan_control *sc,
 				unsigned long nr_anon, unsigned long nr_file,
 				struct list_head *page_list)
 {

Quoted 35 lines · On Wed, 15 Sep 2010 16:28:43 +0800 Wu Fengguang <[email protected]> wrote:

Neil,

Sorry for the rushed and imaginary ideas this morning..

Review static unsigned long shrink_inactive_lis · line 1109

Quoted 10 lines

(quoted diff) +6 -0

@@ -1101,6 +1101,12 @@ static unsigned long shrink_inactive_lis
 	int lumpy_reclaim = 0;
 
 	while (unlikely(too_many_isolated(zone, file, sc))) {
+		if ((sc->gfp_mask & GFP_IOFS) != GFP_IOFS)
+			/* Not allowed to do IO, so mustn't wait
+			 * on processes that might try to
+			 */
+			return SWAP_CLUSTER_MAX;
+

The above patch should behavior like this: it returns SWAP_CLUSTER_MAX to cheat all the way up to believe "enough pages have been reclaimed". So __alloc_pages_direct_reclaim() see non-zero *did_some_progress and go on to call get_page_from_freelist(). That normally fails because the task didn't really scanned the LRU lists. However it does have the possibility to succeed -- when so many processes are doing concurrent direct reclaims, it may luckily get one free page reclaimed by other tasks. What's more, if it does fail to get a free page, the upper layer __alloc_pages_slowpath() will be repeat recalling __alloc_pages_direct_reclaim(). So, sooner or later it will succeed in "stealing" a free page reclaimed by other tasks.

In summary, the patch behavior for !__GFP_IO/FS is
- won't do any page reclaim
- won't fail the page allocation (unexpected)
- will wait and steal one free page from others (unreasonable)

So it will address the problem you encountered, however it sounds pretty unexpected and illogical behavior, right?

Quoted 2 lines

I believe this patch will address the problem equally well. What do you think?

Thank you for the detailed explanation. Is agree with your reasoning and now understand why your patch is sufficient.

I will get it tested and let you know how that goes.

Thanks,
NeilBrown

Quoted 84 lines

Thanks,
Fengguang
---

mm: Avoid possible deadlock caused by too_many_isolated()

Neil finds that if too_many_isolated() returns true while performing direct reclaim we can end up waiting for other threads to complete their direct reclaim. If those threads are allowed to enter the FS or IO to free memory, but this thread is not, then it is possible that those threads will be waiting on this thread and so we get a circular deadlock.

some task enters direct reclaim with GFP_KERNEL
=> too_many_isolated() false
=> vmscan and run into dirty pages
=> pageout()
=> take some FS lock
=> fs/block code does GFP_NOIO allocation
=> enter direct reclaim again
=> too_many_isolated() true
=> waiting for others to progress, however the other
tasks may be circular waiting for the FS lock..

The fix is to let !__GFP_IO and !__GFP_FS direct reclaims enjoy higher priority than normal ones, by honouring them higher throttle threshold.

Now !__GFP_IO/FS reclaims won't be waiting for __GFP_IO/FS reclaims to progress. They will be blocked only when there are too many concurrent !__GFP_IO/FS reclaims, however that's very unlikely because the IO-less direct reclaims is able to progress much more faster, and they won't deadlock each other. The threshold is raised high enough for them, so that there can be sufficient parallel progress of !__GFP_IO/FS reclaims.

Reported-by NeilBrown <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 mm/vmscan.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

mm/vmscan.c +4 -1

@@ -1141,36 +1141,39 @@ int isolate_lru_page(struct page *page)
 	return ret;
 }
 
 /*
  * Are there way too many processes in the direct reclaim path already?
  */
 static int too_many_isolated(struct zone *zone, int file,
 		struct scan_control *sc)
 {
 	unsigned long inactive, isolated;
+	int ratio;
 
 	if (current_is_kswapd())
 		return 0;
 
 	if (!scanning_global_lru(sc))
 		return 0;
 
 	if (file) {
 		inactive = zone_page_state(zone, NR_INACTIVE_FILE);
 		isolated = zone_page_state(zone, NR_ISOLATED_FILE);
 	} else {
 		inactive = zone_page_state(zone, NR_INACTIVE_ANON);
 		isolated = zone_page_state(zone, NR_ISOLATED_ANON);
 	}
 
-	return isolated > inactive;
+	ratio = sc->gfp_mask & (__GFP_IO | __GFP_FS) ? 1 : 8;
+
+	return isolated > inactive * ratio;
 }
 
 /*
  * TODO: Try merging with migrations version of putback_lru_pages
  */
 static noinline_for_stack void
 putback_lru_pages(struct zone *zone, struct scan_control *sc,
 				unsigned long nr_anon, unsigned long nr_file,
 				struct list_head *page_list)
 {

Quoted 38 lines · On Wed, 15 Sep 2010 18:44:34 +1000 Neil Brown <[email protected]> wrote:

Quoted 35 lines · On Wed, 15 Sep 2010 16:28:43 +0800 Wu Fengguang <[email protected]> wrote:

Neil,

Sorry for the rushed and imaginary ideas this morning..

Review static unsigned long shrink_inactive_lis · line 1109

Quoted 10 lines

(quoted diff) +6 -0

@@ -1101,6 +1101,12 @@ static unsigned long shrink_inactive_lis
 	int lumpy_reclaim = 0;
 
 	while (unlikely(too_many_isolated(zone, file, sc))) {
+		if ((sc->gfp_mask & GFP_IOFS) != GFP_IOFS)
+			/* Not allowed to do IO, so mustn't wait
+			 * on processes that might try to
+			 */
+			return SWAP_CLUSTER_MAX;
+

The above patch should behavior like this: it returns SWAP_CLUSTER_MAX to cheat all the way up to believe "enough pages have been reclaimed". So __alloc_pages_direct_reclaim() see non-zero *did_some_progress and go on to call get_page_from_freelist(). That normally fails because the task didn't really scanned the LRU lists. However it does have the possibility to succeed -- when so many processes are doing concurrent direct reclaims, it may luckily get one free page reclaimed by other tasks. What's more, if it does fail to get a free page, the upper layer __alloc_pages_slowpath() will be repeat recalling __alloc_pages_direct_reclaim(). So, sooner or later it will succeed in "stealing" a free page reclaimed by other tasks.

In summary, the patch behavior for !__GFP_IO/FS is
- won't do any page reclaim
- won't fail the page allocation (unexpected)
- will wait and steal one free page from others (unreasonable)

So it will address the problem you encountered, however it sounds pretty unexpected and illogical behavior, right?

Quoted 7 lines

Quoted 2 lines

I believe this patch will address the problem equally well. What do you think?

Thank you for the detailed explanation. Is agree with your reasoning and now understand why your patch is sufficient.

I will get it tested and let you know how that goes.

(sorry this has taken a month to follow up).

Testing shows that this patch seems to work.
The test load (essentially kernbench) doesn't deadlock any more, though it does get bogged down thrashing in swap so it doesn't make a lot more progress :-) I guess that is to be expected.

One observation is that the kernbench generated 10%-20% more context switches with the patch than without. Is that to be expected?

Do you have plans for sending this patch upstream?

Testing shows that this patch seems to work.
The test load (essentially kernbench) doesn't deadlock any more, though it does get bogged down thrashing in swap so it doesn't make a lot more progress :-)  I guess that is to be expected.

Quoted 4 lines · Neil Brown

Testing shows that this patch seems to work.
The test load (essentially kernbench) doesn't deadlock any more, though it does get bogged down thrashing in swap so it doesn't make a lot more progress :-)  I guess that is to be expected.

I just noticed this thread, as your mail from today pushed it up.

In your original mail you wrote: " I recently had a customer (running 2.6.32) report a deadlock during very intensive IO with lots of processes. " and " Some threads that are blocked there, hold some IO lock (probably in the filesystem) and are trying to allocate memory inside the block device (md/raid1 to be precise) which is allocating with GFP_NOIO and has a mempool to fall back on."

I recently had the same problem (intense IO due to swapstorm created by 20 gcc processes hung my system) and after initially blaming the workqueue changes in 2.6.36 Tejun Heo determined that my problem was not the workqueues getting locked up, but that it was cause by an exhausted mempool:
http://marc.info/?l=linux-kernel&m=128655737012549&w=2

Instrumenting mm/mempool.c and retrying my workload showed that fs_bio_set from fs/bio.c looked like the mempool to blame and the code in drivers/md/raid1.c to be the misuser:
http://marc.info/?l=linux-kernel&m=128671179817823&w=2

I was even able to reproduce this hang with only using a normal RAID1 md device as swapspace and then using dd to fill a tmpfs until swapping was needed:
http://marc.info/?l=linux-raid&m=128699402805191&w=2

Looking back in the history of raid1.c and bio.c I found the following interesting parts:

* the change to allocate more then one bio via bio_clone() is from
2005, but it looks like it was OK back then, because at that point the fs_bio_set was allocation 256 entries
* in 2007 the size of the mempool was changed from 256 to only 2
entries (5972511b77809cb7c9ccdb79b825c54921c5c546 "A single unit is enough, lets scale it down to 2 just to be on the safe side.")
* only in 2009 the comment "To make this work, callers must never
allocate more than 1 bio at the time from this pool. Callers that need to allocate more than 1 bio must always submit the previously allocate bio for IO before attempting to allocate a new one. Failure to do so can cause livelocks under memory pressure." was added to bio_alloc() that is the base from my reasoning that raid1.c is broken. (And such a comment was not added to bio_clone() although both calls use the same mempool)

So could please look someone into raid1.c to confirm or deny that using multiple bio_clone() (one per drive) before submitting them together could also cause such deadlocks?

Thank for looking

Torsten

Quoted 50 lines · On Mon, 18 Oct 2010 12:58:17 +0200 Torsten Kaiser…

Quoted 4 lines · Neil Brown

Testing shows that this patch seems to work.
The test load (essentially kernbench) doesn't deadlock any more, though it does get bogged down thrashing in swap so it doesn't make a lot more progress :-)  I guess that is to be expected.

I just noticed this thread, as your mail from today pushed it up.

In your original mail you wrote: " I recently had a customer (running 2.6.32) report a deadlock during very intensive IO with lots of processes. " and " Some threads that are blocked there, hold some IO lock (probably in the filesystem) and are trying to allocate memory inside the block device (md/raid1 to be precise) which is allocating with GFP_NOIO and has a mempool to fall back on."

I recently had the same problem (intense IO due to swapstorm created by 20 gcc processes hung my system) and after initially blaming the workqueue changes in 2.6.36 Tejun Heo determined that my problem was not the workqueues getting locked up, but that it was cause by an exhausted mempool:
http://marc.info/?l=linux-kernel&m=128655737012549&w=2

Instrumenting mm/mempool.c and retrying my workload showed that fs_bio_set from fs/bio.c looked like the mempool to blame and the code in drivers/md/raid1.c to be the misuser:
http://marc.info/?l=linux-kernel&m=128671179817823&w=2

I was even able to reproduce this hang with only using a normal RAID1 md device as swapspace and then using dd to fill a tmpfs until swapping was needed:
http://marc.info/?l=linux-raid&m=128699402805191&w=2

Looking back in the history of raid1.c and bio.c I found the following interesting parts:

 * the change to allocate more then one bio via bio_clone() is from 2005, but it looks like it was OK back then, because at that point the fs_bio_set was allocation 256 entries
 * in 2007 the size of the mempool was changed from 256 to only 2 entries (5972511b77809cb7c9ccdb79b825c54921c5c546 "A single unit is enough, lets scale it down to 2 just to be on the safe side.")
 * only in 2009 the comment "To make this work, callers must never allocate more than 1 bio at the time from this pool. Callers that need to allocate more than 1 bio must always submit the previously allocate bio for IO before attempting to allocate a new one. Failure to do so can cause livelocks under memory pressure." was added to bio_alloc() that is the base from my reasoning that raid1.c is broken. (And such a comment was not added to bio_clone() although both calls use the same mempool)

Quoted 7 lines

So could please look someone into raid1.c to confirm or deny that using multiple bio_clone() (one per drive) before submitting them together could also cause such deadlocks?

Thank for looking

Torsten

Yes, thanks for the report.
This is a real bug exactly as you describe.

This is how I think I will fix it, though it needs a bit of review and testing before I can be certain.
Also I need to check raid10 etc to see if they can suffer too.

If you can test it I would really appreciate it.

Quoted 8 lines · Neil Brown

Yes, thanks for the report.
This is a real bug exactly as you describe.

This is how I think I will fix it, though it needs a bit of review and testing before I can be certain.
Also I need to check raid10 etc to see if they can suffer too.

If you can test it I would really appreciate it.

I did test it, but while it seemed to fix the deadlock, the system still got unusable.
The still running "vmstat 1" showed that the swapout was still progressing, but at a rate of ~20k sized bursts every 5 to 20 seconds.

I also tried to additionally add Wu's patch:

mm/vmscan.c      2010-10-19 00:13:04.000000000 +0800 renamed from mm/vmscan.c 2010-10-13 12:35:14.000000000 +0800 +7 -0

@@ -1163,6 +1163,13 @@ static int too_many_isolated(struct zone
               isolated = zone_page_state(zone, NR_ISOLATED_ANON);
       }
 
+       /*
+        * GFP_NOIO/GFP_NOFS callers are allowed to isolate more pages, so that
+        * they won't get blocked by normal ones and form circular deadlock.
+        */
+       if ((sc->gfp_mask & GFP_IOFS) == GFP_IOFS)
+               inactive >>= 3;
+
       return isolated > inactive;
 

Either it did help somewhat, or I was more lucky on my second try, but this time I needed ~5 tries instead of only 2 to get the system mostly stuck again. On the testrun with Wu's patch the writeout pattern was more stable, a burst of ~80kb each 20 seconds. But I would suspect that the size of the burst is rather random.

I do have a complete SysRq+T dump from the first run, I can send that to anyone how wants it.
(It's 190k so I don't want not spam it to the list)

Quoted 37 lines · Torsten Kaiser

Quoted 8 lines · Neil Brown

Yes, thanks for the report.
This is a real bug exactly as you describe.

This is how I think I will fix it, though it needs a bit of review and testing before I can be certain.
Also I need to check raid10 etc to see if they can suffer too.

If you can test it I would really appreciate it.

I did test it, but while it seemed to fix the deadlock, the system still got unusable.
The still running "vmstat 1" showed that the swapout was still progressing, but at a rate of ~20k sized bursts every 5 to 20 seconds.

I also tried to additionally add Wu's patch:

mm/vmscan.c      2010-10-19 00:13:04.000000000 +0800 renamed from mm/vmscan.c 2010-10-13 12:35:14.000000000 +0800 +7 -0

@@ -1163,6 +1163,13 @@ static int too_many_isolated(struct zone
               isolated = zone_page_state(zone, NR_ISOLATED_ANON);
       }
 
+       /*
+        * GFP_NOIO/GFP_NOFS callers are allowed to isolate more pages, so that
+        * they won't get blocked by normal ones and form circular deadlock.
+        */
+       if ((sc->gfp_mask & GFP_IOFS) == GFP_IOFS)
+               inactive >>= 3;
+
       return isolated > inactive;
 

Either it did help somewhat, or I was more lucky on my second try, but this time I needed ~5 tries instead of only 2 to get the system mostly stuck again. On the testrun with Wu's patch the writeout pattern was more stable, a burst of ~80kb each 20 seconds. But I would suspect that the size of the burst is rather random.

Quoted 3 lines

I do have a complete SysRq+T dump from the first run, I can send that to anyone how wants it.
(It's 190k so I don't want not spam it to the list)

Is this call trace from the SysRq+T violation the rule to only allocate one bio from bio_alloc() until its submitted?

[  549.700038] Call Trace:
[  549.700038]  [<ffffffff81566b54>] schedule_timeout+0x144/0x200
[  549.700038]  [<ffffffff81045cd0>] ? process_timeout+0x0/0x10
[  549.700038]  [<ffffffff81565e22>] io_schedule_timeout+0x42/0x60
[  549.700038]  [<ffffffff81083123>] mempool_alloc+0x163/0x1b0
[  549.700038]  [<ffffffff81053560>] ? autoremove_wake_function+0x0/0x40
[  549.700038]  [<ffffffff810ea2b9>] bio_alloc_bioset+0x39/0xf0
[  549.700038]  [<ffffffff810ea38d>] bio_clone+0x1d/0x50
[  549.700038]  [<ffffffff814318ed>] make_request+0x23d/0x850
[  549.700038]  [<ffffffff81082e20>] ? mempool_alloc_slab+0x10/0x20
[  549.700038]  [<ffffffff81045cd0>] ? process_timeout+0x0/0x10
[  549.700038]  [<ffffffff81436e63>] md_make_request+0xc3/0x220
[  549.700038]  [<ffffffff81083099>] ? mempool_alloc+0xd9/0x1b0
[  549.700038]  [<ffffffff811ec153>] generic_make_request+0x1b3/0x370
[  549.700038]  [<ffffffff810ea2d6>] ? bio_alloc_bioset+0x56/0xf0
[  549.700038]  [<ffffffff811ec36a>] submit_bio+0x5a/0xd0
[  549.700038]  [<ffffffff81080cf5>] ? unlock_page+0x25/0x30
[  549.700038]  [<ffffffff810a871e>] swap_writepage+0x7e/0xc0
[  549.700038]  [<ffffffff81090d99>] shmem_writepage+0x1c9/0x240
[  549.700038]  [<ffffffff8108c9cb>] pageout+0x11b/0x270
[  549.700038]  [<ffffffff8108cd78>] shrink_page_list+0x258/0x4d0
[  549.700038]  [<ffffffff8108d9e7>] shrink_inactive_list+0x187/0x310
[  549.700038]  [<ffffffff8102dcb1>] ? __wake_up_common+0x51/0x80
[  549.700038]  [<ffffffff811fc8b2>] ? cpumask_next_and+0x22/0x40
[  549.700038]  [<ffffffff8108e1c0>] shrink_zone+0x3e0/0x470
[  549.700038]  [<ffffffff8108e797>] try_to_free_pages+0x157/0x410
[  549.700038]  [<ffffffff81087c92>] __alloc_pages_nodemask+0x412/0x760
[  549.700038]  [<ffffffff810b27d6>] alloc_pages_current+0x76/0xe0
[  549.700038]  [<ffffffff810b6dad>] new_slab+0x1fd/0x2a0
[  549.700038]  [<ffffffff81045cd0>] ? process_timeout+0x0/0x10
[  549.700038]  [<ffffffff810b8721>] __slab_alloc+0x111/0x540
[  549.700038]  [<ffffffff81059961>] ? prepare_creds+0x21/0xb0
[  549.700038]  [<ffffffff810b92bb>] kmem_cache_alloc+0x9b/0xa0
[  549.700038]  [<ffffffff81059961>] prepare_creds+0x21/0xb0
[  549.700038]  [<ffffffff8104a919>] sys_setresgid+0x29/0x120
[  549.700038]  [<ffffffff8100242b>] system_call_fastpath+0x16/0x1b
[  549.700038]  ffff88011e125ea8 0000000000000046 ffff88011e125e08
ffffffff81073c59
[  549.700038]  0000000000012780 ffff88011ea905b0 ffff88011ea90808
ffff88011e125fd8
[  549.700038]  ffff88011ea90810 ffff88011e124010 0000000000012780

ffff88011e125fd8

swap_writepage() uses get_swap_bio() which uses bio_alloc() to get one bio. That bio is the submitted, but the submit path seems to get into make_request from raid1.c and that allocates a second bio from bio_alloc() via bio_clone().

I am seeing this pattern (swap_writepage calling md_make_request/make_request and then getting stuck in mempool_alloc) more than 5 times in the SysRq+T output...

Quoted 89 lines

Quoted 37 lines · Torsten Kaiser

Quoted 8 lines · Neil Brown

Yes, thanks for the report.
This is a real bug exactly as you describe.

This is how I think I will fix it, though it needs a bit of review and testing before I can be certain.
Also I need to check raid10 etc to see if they can suffer too.

If you can test it I would really appreciate it.

I did test it, but while it seemed to fix the deadlock, the system still got unusable.
The still running "vmstat 1" showed that the swapout was still progressing, but at a rate of ~20k sized bursts every 5 to 20 seconds.

I also tried to additionally add Wu's patch:

mm/vmscan.c      2010-10-19 00:13:04.000000000 +0800 renamed from mm/vmscan.c 2010-10-13 12:35:14.000000000 +0800 +7 -0

@@ -1163,6 +1163,13 @@ static int too_many_isolated(struct zone
               isolated = zone_page_state(zone, NR_ISOLATED_ANON);
       }
 
+       /*
+        * GFP_NOIO/GFP_NOFS callers are allowed to isolate more pages, so that
+        * they won't get blocked by normal ones and form circular deadlock.
+        */
+       if ((sc->gfp_mask & GFP_IOFS) == GFP_IOFS)
+               inactive >>= 3;
+
       return isolated > inactive;
 

Either it did help somewhat, or I was more lucky on my second try, but this time I needed ~5 tries instead of only 2 to get the system mostly stuck again. On the testrun with Wu's patch the writeout pattern was more stable, a burst of ~80kb each 20 seconds. But I would suspect that the size of the burst is rather random.

Quoted 3 lines

I do have a complete SysRq+T dump from the first run, I can send that to anyone how wants it.
(It's 190k so I don't want not spam it to the list)

Is this call trace from the SysRq+T violation the rule to only allocate one bio from bio_alloc() until its submitted?

[  549.700038] Call Trace:
[  549.700038]  [<ffffffff81566b54>] schedule_timeout+0x144/0x200
[  549.700038]  [<ffffffff81045cd0>] ? process_timeout+0x0/0x10
[  549.700038]  [<ffffffff81565e22>] io_schedule_timeout+0x42/0x60
[  549.700038]  [<ffffffff81083123>] mempool_alloc+0x163/0x1b0
[  549.700038]  [<ffffffff81053560>] ? autoremove_wake_function+0x0/0x40
[  549.700038]  [<ffffffff810ea2b9>] bio_alloc_bioset+0x39/0xf0
[  549.700038]  [<ffffffff810ea38d>] bio_clone+0x1d/0x50
[  549.700038]  [<ffffffff814318ed>] make_request+0x23d/0x850
[  549.700038]  [<ffffffff81082e20>] ? mempool_alloc_slab+0x10/0x20
[  549.700038]  [<ffffffff81045cd0>] ? process_timeout+0x0/0x10
[  549.700038]  [<ffffffff81436e63>] md_make_request+0xc3/0x220
[  549.700038]  [<ffffffff81083099>] ? mempool_alloc+0xd9/0x1b0
[  549.700038]  [<ffffffff811ec153>] generic_make_request+0x1b3/0x370
[  549.700038]  [<ffffffff810ea2d6>] ? bio_alloc_bioset+0x56/0xf0
[  549.700038]  [<ffffffff811ec36a>] submit_bio+0x5a/0xd0
[  549.700038]  [<ffffffff81080cf5>] ? unlock_page+0x25/0x30
[  549.700038]  [<ffffffff810a871e>] swap_writepage+0x7e/0xc0
[  549.700038]  [<ffffffff81090d99>] shmem_writepage+0x1c9/0x240
[  549.700038]  [<ffffffff8108c9cb>] pageout+0x11b/0x270
[  549.700038]  [<ffffffff8108cd78>] shrink_page_list+0x258/0x4d0
[  549.700038]  [<ffffffff8108d9e7>] shrink_inactive_list+0x187/0x310
[  549.700038]  [<ffffffff8102dcb1>] ? __wake_up_common+0x51/0x80
[  549.700038]  [<ffffffff811fc8b2>] ? cpumask_next_and+0x22/0x40
[  549.700038]  [<ffffffff8108e1c0>] shrink_zone+0x3e0/0x470
[  549.700038]  [<ffffffff8108e797>] try_to_free_pages+0x157/0x410
[  549.700038]  [<ffffffff81087c92>] __alloc_pages_nodemask+0x412/0x760
[  549.700038]  [<ffffffff810b27d6>] alloc_pages_current+0x76/0xe0
[  549.700038]  [<ffffffff810b6dad>] new_slab+0x1fd/0x2a0
[  549.700038]  [<ffffffff81045cd0>] ? process_timeout+0x0/0x10
[  549.700038]  [<ffffffff810b8721>] __slab_alloc+0x111/0x540
[  549.700038]  [<ffffffff81059961>] ? prepare_creds+0x21/0xb0
[  549.700038]  [<ffffffff810b92bb>] kmem_cache_alloc+0x9b/0xa0
[  549.700038]  [<ffffffff81059961>] prepare_creds+0x21/0xb0
[  549.700038]  [<ffffffff8104a919>] sys_setresgid+0x29/0x120
[  549.700038]  [<ffffffff8100242b>] system_call_fastpath+0x16/0x1b
[  549.700038]  ffff88011e125ea8 0000000000000046 ffff88011e125e08
ffffffff81073c59
[  549.700038]  0000000000012780 ffff88011ea905b0 ffff88011ea90808
ffff88011e125fd8
[  549.700038]  ffff88011ea90810 ffff88011e124010 0000000000012780

ffff88011e125fd8

Quoted 8 lines

swap_writepage() uses get_swap_bio() which uses bio_alloc() to get one bio. That bio is the submitted, but the submit path seems to get into make_request from raid1.c and that allocates a second bio from bio_alloc() via bio_clone().

I am seeing this pattern (swap_writepage calling md_make_request/make_request and then getting stuck in mempool_alloc) more than 5 times in the SysRq+T output...

I bet the root cause is the failure of pool->alloc(__GFP_NORETRY) inside mempool_alloc(), which can be fixed by this patch.

Thanks,
Fengguang
---

concurrent direct page reclaim problem

__GFP_NORETRY page allocations may fail when there are many concurrent page allocating tasks, but not necessary in real short of memory. The root cause is, tasks will first run direct page reclaim to free some pages from the LRU lists and put them to the per-cpu page lists and the buddy system, and then try to get a free page from there. However the free pages reclaimed by this task may be consumed by other tasks when the direct reclaim task is able to get the free page for itself.

Let's retry it a bit harder.

Quoted 128 lines · Torsten Kaiser

Quoted 89 lines

Quoted 37 lines · Torsten Kaiser

Quoted 8 lines · Neil Brown

Yes, thanks for the report.
This is a real bug exactly as you describe.

This is how I think I will fix it, though it needs a bit of review and testing before I can be certain.
Also I need to check raid10 etc to see if they can suffer too.

If you can test it I would really appreciate it.

I did test it, but while it seemed to fix the deadlock, the system still got unusable.
The still running "vmstat 1" showed that the swapout was still progressing, but at a rate of ~20k sized bursts every 5 to 20 seconds.

I also tried to additionally add Wu's patch:

mm/vmscan.c A A A 2010-10-19 00:13:04.000000000 +0800 renamed from mm/vmscan.c 2010-10-13 12:35:14.000000000 +0800 +7 -0

@@ -1163,6 +1163,13 @@ static int too_many_isolated(struct zone
 A  A  A  A  A  A  A  isolated = zone_page_state(zone, NR_ISOLATED_ANON);
 A  A  A  }
 
+ A  A  A  /*
+ A  A  A  A * GFP_NOIO/GFP_NOFS callers are allowed to isolate more pages, so that
+ A  A  A  A * they won't get blocked by normal ones and form circular deadlock.
+ A  A  A  A */
+ A  A  A  if ((sc->gfp_mask & GFP_IOFS) == GFP_IOFS)
+ A  A  A  A  A  A  A  inactive >>= 3;
+
 A  A  A  return isolated > inactive;
 

Either it did help somewhat, or I was more lucky on my second try, but this time I needed ~5 tries instead of only 2 to get the system mostly stuck again. On the testrun with Wu's patch the writeout pattern was more stable, a burst of ~80kb each 20 seconds. But I would suspect that the size of the burst is rather random.

Quoted 3 lines

I do have a complete SysRq+T dump from the first run, I can send that to anyone how wants it.
(It's 190k so I don't want not spam it to the list)

Is this call trace from the SysRq+T violation the rule to only allocate one bio from bio_alloc() until its submitted?

[  549.700038] Call Trace:
[  549.700038]  [<ffffffff81566b54>] schedule_timeout+0x144/0x200
[  549.700038]  [<ffffffff81045cd0>] ? process_timeout+0x0/0x10
[  549.700038]  [<ffffffff81565e22>] io_schedule_timeout+0x42/0x60
[  549.700038]  [<ffffffff81083123>] mempool_alloc+0x163/0x1b0
[  549.700038]  [<ffffffff81053560>] ? autoremove_wake_function+0x0/0x40
[  549.700038]  [<ffffffff810ea2b9>] bio_alloc_bioset+0x39/0xf0
[  549.700038]  [<ffffffff810ea38d>] bio_clone+0x1d/0x50
[  549.700038]  [<ffffffff814318ed>] make_request+0x23d/0x850
[  549.700038]  [<ffffffff81082e20>] ? mempool_alloc_slab+0x10/0x20
[  549.700038]  [<ffffffff81045cd0>] ? process_timeout+0x0/0x10
[  549.700038]  [<ffffffff81436e63>] md_make_request+0xc3/0x220
[  549.700038]  [<ffffffff81083099>] ? mempool_alloc+0xd9/0x1b0
[  549.700038]  [<ffffffff811ec153>] generic_make_request+0x1b3/0x370
[  549.700038]  [<ffffffff810ea2d6>] ? bio_alloc_bioset+0x56/0xf0
[  549.700038]  [<ffffffff811ec36a>] submit_bio+0x5a/0xd0
[  549.700038]  [<ffffffff81080cf5>] ? unlock_page+0x25/0x30
[  549.700038]  [<ffffffff810a871e>] swap_writepage+0x7e/0xc0
[  549.700038]  [<ffffffff81090d99>] shmem_writepage+0x1c9/0x240
[  549.700038]  [<ffffffff8108c9cb>] pageout+0x11b/0x270
[  549.700038]  [<ffffffff8108cd78>] shrink_page_list+0x258/0x4d0
[  549.700038]  [<ffffffff8108d9e7>] shrink_inactive_list+0x187/0x310
[  549.700038]  [<ffffffff8102dcb1>] ? __wake_up_common+0x51/0x80
[  549.700038]  [<ffffffff811fc8b2>] ? cpumask_next_and+0x22/0x40
[  549.700038]  [<ffffffff8108e1c0>] shrink_zone+0x3e0/0x470
[  549.700038]  [<ffffffff8108e797>] try_to_free_pages+0x157/0x410
[  549.700038]  [<ffffffff81087c92>] __alloc_pages_nodemask+0x412/0x760
[  549.700038]  [<ffffffff810b27d6>] alloc_pages_current+0x76/0xe0
[  549.700038]  [<ffffffff810b6dad>] new_slab+0x1fd/0x2a0
[  549.700038]  [<ffffffff81045cd0>] ? process_timeout+0x0/0x10
[  549.700038]  [<ffffffff810b8721>] __slab_alloc+0x111/0x540
[  549.700038]  [<ffffffff81059961>] ? prepare_creds+0x21/0xb0
[  549.700038]  [<ffffffff810b92bb>] kmem_cache_alloc+0x9b/0xa0
[  549.700038]  [<ffffffff81059961>] prepare_creds+0x21/0xb0
[  549.700038]  [<ffffffff8104a919>] sys_setresgid+0x29/0x120
[  549.700038]  [<ffffffff8100242b>] system_call_fastpath+0x16/0x1b
[  549.700038]  ffff88011e125ea8 0000000000000046 ffff88011e125e08
ffffffff81073c59
[  549.700038]  0000000000012780 ffff88011ea905b0 ffff88011ea90808
ffff88011e125fd8
[  549.700038]  ffff88011ea90810 ffff88011e124010 0000000000012780

ffff88011e125fd8

Quoted 8 lines

swap_writepage() uses get_swap_bio() which uses bio_alloc() to get one bio. That bio is the submitted, but the submit path seems to get into make_request from raid1.c and that allocates a second bio from bio_alloc() via bio_clone().

I am seeing this pattern (swap_writepage calling md_make_request/make_request and then getting stuck in mempool_alloc) more than 5 times in the SysRq+T output...

I bet the root cause is the failure of pool->alloc(__GFP_NORETRY) inside mempool_alloc(), which can be fixed by this patch.

Thanks,
Fengguang
---

concurrent direct page reclaim problem

__GFP_NORETRY page allocations may fail when there are many concurrent page allocating tasks, but not necessary in real short of memory. The root cause is, tasks will first run direct page reclaim to free some pages from the LRU lists and put them to the per-cpu page lists and the buddy system, and then try to get a free page from there. However the free pages reclaimed by this task may be consumed by other tasks when the direct reclaim task is able to get the free page for itself.

Let's retry it a bit harder.

mm/page_alloc.c +1 -1

@@ -1700,7 +1700,7 @@ should_alloc_retry(gfp_t gfp_mask, unsig
 				unsigned long pages_reclaimed)
 {
 	/* Do not loop if specifically requested */
-	if (gfp_mask & __GFP_NORETRY)
+	if (gfp_mask & __GFP_NORETRY && pages_reclaimed > (1 << (order + 12)))
 		return 0;
 

mm/page_alloc.c should_alloc_retry(gfp_t gfp_mask, unsig · line 1706

Quoted 1 line

mm/page_alloc.c +0 -0

 	/*

SLUB usually try high order allocation with __GFP_NORETRY at first. In other words, It strongly depend on __GFP_NORETRY don't any retry. I'm worry this...

And, in this case, stucked tasks have PF_MEMALLOC. allocation with PF_MEMALLOC failure mean this zone have zero memory purely. So, retrying don't solve anything.

And I think the root cause is in another.

bio_clone() use fs_bio_set internally.

	struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)
	{
	        struct bio *b = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, fs_bio_set);
	...

and fs_bio_set is initialized very small pool size.

	#define BIO_POOL_SIZE 2
	static int __init init_bio(void)
	{
		..
	        fs_bio_set = bioset_create(BIO_POOL_SIZE, 0);

So, I think raid1.c need to use their own bioset instead fs_bio_set. otherwise, bio pool exshost can happen very easily.

But I'm not sure. I'm not IO expert.

Quoted 128 lines · KOSAKI Motohiro

Quoted 128 lines · Torsten Kaiser

Quoted 89 lines

Quoted 37 lines · Torsten Kaiser

Quoted 8 lines · Neil Brown

Yes, thanks for the report.
This is a real bug exactly as you describe.

This is how I think I will fix it, though it needs a bit of review and testing before I can be certain.
Also I need to check raid10 etc to see if they can suffer too.

If you can test it I would really appreciate it.

I did test it, but while it seemed to fix the deadlock, the system still got unusable.
The still running "vmstat 1" showed that the swapout was still progressing, but at a rate of ~20k sized bursts every 5 to 20 seconds.

I also tried to additionally add Wu's patch:

mm/vmscan.c 2010-10-19 00:13:04.000000000 +0800 renamed from mm/vmscan.c 2010-10-13 12:35:14.000000000 +0800 +7 -0

@@ -1163,6 +1163,13 @@ static int too_many_isolated(struct zone
              isolated = zone_page_state(zone, NR_ISOLATED_ANON);
      }
 
+       /*
+        * GFP_NOIO/GFP_NOFS callers are allowed to isolate more pages, so that
+        * they won't get blocked by normal ones and form circular deadlock.
+        */
+       if ((sc->gfp_mask & GFP_IOFS) == GFP_IOFS)
+               inactive >>= 3;
+
      return isolated > inactive;
 

Either it did help somewhat, or I was more lucky on my second try, but this time I needed ~5 tries instead of only 2 to get the system mostly stuck again. On the testrun with Wu's patch the writeout pattern was more stable, a burst of ~80kb each 20 seconds. But I would suspect that the size of the burst is rather random.

Quoted 3 lines

I do have a complete SysRq+T dump from the first run, I can send that to anyone how wants it.
(It's 190k so I don't want not spam it to the list)

Is this call trace from the SysRq+T violation the rule to only allocate one bio from bio_alloc() until its submitted?

[  549.700038] Call Trace:
[  549.700038]  [<ffffffff81566b54>] schedule_timeout+0x144/0x200
[  549.700038]  [<ffffffff81045cd0>] ? process_timeout+0x0/0x10
[  549.700038]  [<ffffffff81565e22>] io_schedule_timeout+0x42/0x60
[  549.700038]  [<ffffffff81083123>] mempool_alloc+0x163/0x1b0
[  549.700038]  [<ffffffff81053560>] ? autoremove_wake_function+0x0/0x40
[  549.700038]  [<ffffffff810ea2b9>] bio_alloc_bioset+0x39/0xf0
[  549.700038]  [<ffffffff810ea38d>] bio_clone+0x1d/0x50
[  549.700038]  [<ffffffff814318ed>] make_request+0x23d/0x850
[  549.700038]  [<ffffffff81082e20>] ? mempool_alloc_slab+0x10/0x20
[  549.700038]  [<ffffffff81045cd0>] ? process_timeout+0x0/0x10
[  549.700038]  [<ffffffff81436e63>] md_make_request+0xc3/0x220
[  549.700038]  [<ffffffff81083099>] ? mempool_alloc+0xd9/0x1b0
[  549.700038]  [<ffffffff811ec153>] generic_make_request+0x1b3/0x370
[  549.700038]  [<ffffffff810ea2d6>] ? bio_alloc_bioset+0x56/0xf0
[  549.700038]  [<ffffffff811ec36a>] submit_bio+0x5a/0xd0
[  549.700038]  [<ffffffff81080cf5>] ? unlock_page+0x25/0x30
[  549.700038]  [<ffffffff810a871e>] swap_writepage+0x7e/0xc0
[  549.700038]  [<ffffffff81090d99>] shmem_writepage+0x1c9/0x240
[  549.700038]  [<ffffffff8108c9cb>] pageout+0x11b/0x270
[  549.700038]  [<ffffffff8108cd78>] shrink_page_list+0x258/0x4d0
[  549.700038]  [<ffffffff8108d9e7>] shrink_inactive_list+0x187/0x310
[  549.700038]  [<ffffffff8102dcb1>] ? __wake_up_common+0x51/0x80
[  549.700038]  [<ffffffff811fc8b2>] ? cpumask_next_and+0x22/0x40
[  549.700038]  [<ffffffff8108e1c0>] shrink_zone+0x3e0/0x470
[  549.700038]  [<ffffffff8108e797>] try_to_free_pages+0x157/0x410
[  549.700038]  [<ffffffff81087c92>] __alloc_pages_nodemask+0x412/0x760
[  549.700038]  [<ffffffff810b27d6>] alloc_pages_current+0x76/0xe0
[  549.700038]  [<ffffffff810b6dad>] new_slab+0x1fd/0x2a0
[  549.700038]  [<ffffffff81045cd0>] ? process_timeout+0x0/0x10
[  549.700038]  [<ffffffff810b8721>] __slab_alloc+0x111/0x540
[  549.700038]  [<ffffffff81059961>] ? prepare_creds+0x21/0xb0
[  549.700038]  [<ffffffff810b92bb>] kmem_cache_alloc+0x9b/0xa0
[  549.700038]  [<ffffffff81059961>] prepare_creds+0x21/0xb0
[  549.700038]  [<ffffffff8104a919>] sys_setresgid+0x29/0x120
[  549.700038]  [<ffffffff8100242b>] system_call_fastpath+0x16/0x1b
[  549.700038]  ffff88011e125ea8 0000000000000046 ffff88011e125e08
ffffffff81073c59
[  549.700038]  0000000000012780 ffff88011ea905b0 ffff88011ea90808
ffff88011e125fd8
[  549.700038]  ffff88011ea90810 ffff88011e124010 0000000000012780

ffff88011e125fd8

Quoted 8 lines

swap_writepage() uses get_swap_bio() which uses bio_alloc() to get one bio. That bio is the submitted, but the submit path seems to get into make_request from raid1.c and that allocates a second bio from bio_alloc() via bio_clone().

I am seeing this pattern (swap_writepage calling md_make_request/make_request and then getting stuck in mempool_alloc) more than 5 times in the SysRq+T output...

I bet the root cause is the failure of pool->alloc(__GFP_NORETRY) inside mempool_alloc(), which can be fixed by this patch.

Thanks,
Fengguang
---

concurrent direct page reclaim problem

__GFP_NORETRY page allocations may fail when there are many concurrent page allocating tasks, but not necessary in real short of memory. The root cause is, tasks will first run direct page reclaim to free some pages from the LRU lists and put them to the per-cpu page lists and the buddy system, and then try to get a free page from there. However the free pages reclaimed by this task may be consumed by other tasks when the direct reclaim task is able to get the free page for itself.

Let's retry it a bit harder.

mm/page_alloc.c +1 -1

@@ -1700,7 +1700,7 @@ should_alloc_retry(gfp_t gfp_mask, unsig
 				unsigned long pages_reclaimed)
 {
 	/* Do not loop if specifically requested */
-	if (gfp_mask & __GFP_NORETRY)
+	if (gfp_mask & __GFP_NORETRY && pages_reclaimed > (1 << (order + 12)))
 		return 0;
 

Quoted 5 lines

mm/page_alloc.c should_alloc_retry(gfp_t gfp_mask, unsig · line 1706

Quoted 1 line

mm/page_alloc.c +0 -0

 	/*

SLUB usually try high order allocation with __GFP_NORETRY at first. In other words, It strongly depend on __GFP_NORETRY don't any retry. I'm worry this...

Right. I noticed that too. Hopefully the "limited" retry won't impact it too much. That said, we do need a better solution than such hacks.

Quoted 2 lines

And, in this case, stucked tasks have PF_MEMALLOC. allocation with PF_MEMALLOC failure mean this zone have zero memory purely. So, retrying don't solve anything.

The zone has no free (buddy) memory, but has plenty of reclaimable pages. The concurrent page reclaimers may steal pages reclaimed by this task from time to time, but not always. So retry reclaiming will help.

Quoted 9 lines

And I think the root cause is in another.

bio_clone() use fs_bio_set internally.

	struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)
	{
	        struct bio *b = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, fs_bio_set);
	...

Quoted 7 lines

and fs_bio_set is initialized very small pool size.

	#define BIO_POOL_SIZE 2
	static int __init init_bio(void)
	{
		..
	        fs_bio_set = bioset_create(BIO_POOL_SIZE, 0);

Agreed. BIO_POOL_SIZE=2 is too small to be deadlock free.

Quoted 2 lines

So, I think raid1.c need to use their own bioset instead fs_bio_set. otherwise, bio pool exshost can happen very easily.

That would fix the deadlock, but not enough for good IO throughput when multiple CPUs are trying to submit IO. Increasing BIO_POOL_SIZE to a large value should help fix both the deadlock and IO throughput.

Quoted 1 line

But I'm not sure. I'm not IO expert.

[add CC to Jens]

Quoted 171 lines · Wu Fengguang

Quoted 128 lines · KOSAKI Motohiro

Quoted 128 lines · Torsten Kaiser

Quoted 89 lines

Quoted 37 lines · Torsten Kaiser

Quoted 8 lines · Neil Brown

Yes, thanks for the report.
This is a real bug exactly as you describe.

This is how I think I will fix it, though it needs a bit of review and testing before I can be certain.
Also I need to check raid10 etc to see if they can suffer too.

If you can test it I would really appreciate it.

I did test it, but while it seemed to fix the deadlock, the system still got unusable.
The still running "vmstat 1" showed that the swapout was still progressing, but at a rate of ~20k sized bursts every 5 to 20 seconds.

I also tried to additionally add Wu's patch:

mm/vmscan.c 2010-10-19 00:13:04.000000000 +0800 renamed from mm/vmscan.c 2010-10-13 12:35:14.000000000 +0800 +7 -0

@@ -1163,6 +1163,13 @@ static int too_many_isolated(struct zone
              isolated = zone_page_state(zone, NR_ISOLATED_ANON);
      }
 
+       /*
+        * GFP_NOIO/GFP_NOFS callers are allowed to isolate more pages, so that
+        * they won't get blocked by normal ones and form circular deadlock.
+        */
+       if ((sc->gfp_mask & GFP_IOFS) == GFP_IOFS)
+               inactive >>= 3;
+
      return isolated > inactive;
 

Either it did help somewhat, or I was more lucky on my second try, but this time I needed ~5 tries instead of only 2 to get the system mostly stuck again. On the testrun with Wu's patch the writeout pattern was more stable, a burst of ~80kb each 20 seconds. But I would suspect that the size of the burst is rather random.

Quoted 3 lines

I do have a complete SysRq+T dump from the first run, I can send that to anyone how wants it.
(It's 190k so I don't want not spam it to the list)

Is this call trace from the SysRq+T violation the rule to only allocate one bio from bio_alloc() until its submitted?

[  549.700038] Call Trace:
[  549.700038]  [<ffffffff81566b54>] schedule_timeout+0x144/0x200
[  549.700038]  [<ffffffff81045cd0>] ? process_timeout+0x0/0x10
[  549.700038]  [<ffffffff81565e22>] io_schedule_timeout+0x42/0x60
[  549.700038]  [<ffffffff81083123>] mempool_alloc+0x163/0x1b0
[  549.700038]  [<ffffffff81053560>] ? autoremove_wake_function+0x0/0x40
[  549.700038]  [<ffffffff810ea2b9>] bio_alloc_bioset+0x39/0xf0
[  549.700038]  [<ffffffff810ea38d>] bio_clone+0x1d/0x50
[  549.700038]  [<ffffffff814318ed>] make_request+0x23d/0x850
[  549.700038]  [<ffffffff81082e20>] ? mempool_alloc_slab+0x10/0x20
[  549.700038]  [<ffffffff81045cd0>] ? process_timeout+0x0/0x10
[  549.700038]  [<ffffffff81436e63>] md_make_request+0xc3/0x220
[  549.700038]  [<ffffffff81083099>] ? mempool_alloc+0xd9/0x1b0
[  549.700038]  [<ffffffff811ec153>] generic_make_request+0x1b3/0x370
[  549.700038]  [<ffffffff810ea2d6>] ? bio_alloc_bioset+0x56/0xf0
[  549.700038]  [<ffffffff811ec36a>] submit_bio+0x5a/0xd0
[  549.700038]  [<ffffffff81080cf5>] ? unlock_page+0x25/0x30
[  549.700038]  [<ffffffff810a871e>] swap_writepage+0x7e/0xc0
[  549.700038]  [<ffffffff81090d99>] shmem_writepage+0x1c9/0x240
[  549.700038]  [<ffffffff8108c9cb>] pageout+0x11b/0x270
[  549.700038]  [<ffffffff8108cd78>] shrink_page_list+0x258/0x4d0
[  549.700038]  [<ffffffff8108d9e7>] shrink_inactive_list+0x187/0x310
[  549.700038]  [<ffffffff8102dcb1>] ? __wake_up_common+0x51/0x80
[  549.700038]  [<ffffffff811fc8b2>] ? cpumask_next_and+0x22/0x40
[  549.700038]  [<ffffffff8108e1c0>] shrink_zone+0x3e0/0x470
[  549.700038]  [<ffffffff8108e797>] try_to_free_pages+0x157/0x410
[  549.700038]  [<ffffffff81087c92>] __alloc_pages_nodemask+0x412/0x760
[  549.700038]  [<ffffffff810b27d6>] alloc_pages_current+0x76/0xe0
[  549.700038]  [<ffffffff810b6dad>] new_slab+0x1fd/0x2a0
[  549.700038]  [<ffffffff81045cd0>] ? process_timeout+0x0/0x10
[  549.700038]  [<ffffffff810b8721>] __slab_alloc+0x111/0x540
[  549.700038]  [<ffffffff81059961>] ? prepare_creds+0x21/0xb0
[  549.700038]  [<ffffffff810b92bb>] kmem_cache_alloc+0x9b/0xa0
[  549.700038]  [<ffffffff81059961>] prepare_creds+0x21/0xb0
[  549.700038]  [<ffffffff8104a919>] sys_setresgid+0x29/0x120
[  549.700038]  [<ffffffff8100242b>] system_call_fastpath+0x16/0x1b
[  549.700038]  ffff88011e125ea8 0000000000000046 ffff88011e125e08
ffffffff81073c59
[  549.700038]  0000000000012780 ffff88011ea905b0 ffff88011ea90808
ffff88011e125fd8
[  549.700038]  ffff88011ea90810 ffff88011e124010 0000000000012780

ffff88011e125fd8

Quoted 8 lines

swap_writepage() uses get_swap_bio() which uses bio_alloc() to get one bio. That bio is the submitted, but the submit path seems to get into make_request from raid1.c and that allocates a second bio from bio_alloc() via bio_clone().

I am seeing this pattern (swap_writepage calling md_make_request/make_request and then getting stuck in mempool_alloc) more than 5 times in the SysRq+T output...

I bet the root cause is the failure of pool->alloc(__GFP_NORETRY) inside mempool_alloc(), which can be fixed by this patch.

Thanks,
Fengguang
---

concurrent direct page reclaim problem

__GFP_NORETRY page allocations may fail when there are many concurrent page allocating tasks, but not necessary in real short of memory. The root cause is, tasks will first run direct page reclaim to free some pages from the LRU lists and put them to the per-cpu page lists and the buddy system, and then try to get a free page from there. However the free pages reclaimed by this task may be consumed by other tasks when the direct reclaim task is able to get the free page for itself.

Let's retry it a bit harder.

mm/page_alloc.c +1 -1

@@ -1700,7 +1700,7 @@ should_alloc_retry(gfp_t gfp_mask, unsig
 				unsigned long pages_reclaimed)
 {
 	/* Do not loop if specifically requested */
-	if (gfp_mask & __GFP_NORETRY)
+	if (gfp_mask & __GFP_NORETRY && pages_reclaimed > (1 << (order + 12)))
 		return 0;
 

Quoted 5 lines

mm/page_alloc.c should_alloc_retry(gfp_t gfp_mask, unsig · line 1706

Quoted 1 line

mm/page_alloc.c +0 -0

 	/*

SLUB usually try high order allocation with __GFP_NORETRY at first. In other words, It strongly depend on __GFP_NORETRY don't any retry. I'm worry this...

Right. I noticed that too. Hopefully the "limited" retry won't impact it too much. That said, we do need a better solution than such hacks.

Quoted 2 lines

And, in this case, stucked tasks have PF_MEMALLOC. allocation with PF_MEMALLOC failure mean this zone have zero memory purely. So, retrying don't solve anything.

The zone has no free (buddy) memory, but has plenty of reclaimable pages. The concurrent page reclaimers may steal pages reclaimed by this task from time to time, but not always. So retry reclaiming will help.

Quoted 9 lines

And I think the root cause is in another.

bio_clone() use fs_bio_set internally.

	struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)
	{
	        struct bio *b = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, fs_bio_set);
	...

Quoted 7 lines

and fs_bio_set is initialized very small pool size.

	#define BIO_POOL_SIZE 2
	static int __init init_bio(void)
	{
		..
	        fs_bio_set = bioset_create(BIO_POOL_SIZE, 0);

Agreed. BIO_POOL_SIZE=2 is too small to be deadlock free.

Quoted 2 lines

So, I think raid1.c need to use their own bioset instead fs_bio_set. otherwise, bio pool exshost can happen very easily.

That would fix the deadlock, but not enough for good IO throughput when multiple CPUs are trying to submit IO. Increasing BIO_POOL_SIZE to a large value should help fix both the deadlock and IO throughput.

Quoted 3 lines

Quoted 1 line

But I'm not sure. I'm not IO expert.

[add CC to Jens]

We surely need 1 set aside for each level of that stack that will potentially consume one. 1 should be enough for the generic pool, and then clones will use a separate pool. So md and friends should really have a pool per device, so that stacking will always work properly.

There should be no throughput concerns, it should purely be a safe guard measure to prevent us deadlocking when doing IO for reclaim.

Quoted 4 lines

We surely need 1 set aside for each level of that stack that will potentially consume one. 1 should be enough for the generic pool, and then clones will use a separate pool. So md and friends should really have a pool per device, so that stacking will always work properly.

Agreed for the deadlock problem.

Quoted 2 lines

There should be no throughput concerns, it should purely be a safe guard measure to prevent us deadlocking when doing IO for reclaim.

It's easy to verify whether the minimal size will have negative impacts on IO throughput. In Torsten's case, increase BIO_POOL_SIZE by one and check how it performs.

Quoted 5 lines

Quoted 4 lines

We surely need 1 set aside for each level of that stack that will potentially consume one. 1 should be enough for the generic pool, and then clones will use a separate pool. So md and friends should really have a pool per device, so that stacking will always work properly.

Quoted 8 lines

Agreed for the deadlock problem.

Quoted 2 lines

There should be no throughput concerns, it should purely be a safe guard measure to prevent us deadlocking when doing IO for reclaim.

It's easy to verify whether the minimal size will have negative impacts on IO throughput. In Torsten's case, increase BIO_POOL_SIZE by one and check how it performs.

Sorry it seems simply increasing BIO_POOL_SIZE is not enough to fix possible deadlocks. We need adding new mempool(s). Because when there BIO_POOL_SIZE=2 and there are two concurrent reclaimers each take 1 reservation, they will deadlock each other when trying to take the next bio at the raid1 level.

Quoted 14 lines · Wu Fengguang

Quoted 5 lines

Quoted 4 lines

We surely need 1 set aside for each level of that stack that will potentially consume one. 1 should be enough for the generic pool, and then clones will use a separate pool. So md and friends should really have a pool per device, so that stacking will always work properly.

Quoted 8 lines

Agreed for the deadlock problem.

Quoted 2 lines

There should be no throughput concerns, it should purely be a safe guard measure to prevent us deadlocking when doing IO for reclaim.

It's easy to verify whether the minimal size will have negative impacts on IO throughput. In Torsten's case, increase BIO_POOL_SIZE by one and check how it performs.

Quoted 5 lines

Sorry it seems simply increasing BIO_POOL_SIZE is not enough to fix possible deadlocks. We need adding new mempool(s). Because when there BIO_POOL_SIZE=2 and there are two concurrent reclaimers each take 1 reservation, they will deadlock each other when trying to take the next bio at the raid1 level.

Yes, plus it's not a practical solution since you don't know how deep the stack is. As I wrote in the initial email, each consumer needs it's own private mempool (and just 1 entry should suffice).

Quoted 21 lines · Jens Axboe

Quoted 19 lines · Wu Fengguang

Quoted 5 lines

Quoted 4 lines

We surely need 1 set aside for each level of that stack that will potentially consume one. 1 should be enough for the generic pool, and then clones will use a separate pool. So md and friends should really have a pool per device, so that stacking will always work properly.

Quoted 8 lines

Agreed for the deadlock problem.

Quoted 2 lines

There should be no throughput concerns, it should purely be a safe guard measure to prevent us deadlocking when doing IO for reclaim.

It's easy to verify whether the minimal size will have negative impacts on IO throughput. In Torsten's case, increase BIO_POOL_SIZE by one and check how it performs.

Sorry it seems simply increasing BIO_POOL_SIZE is not enough to fix possible deadlocks. We need adding new mempool(s). Because when there BIO_POOL_SIZE=2 and there are two concurrent reclaimers each take 1 reservation, they will deadlock each other when trying to take the next bio at the raid1 level.

Quoted 3 lines

Yes, plus it's not a practical solution since you don't know how deep the stack is. As I wrote in the initial email, each consumer needs it's own private mempool (and just 1 entry should suffice).

You are right. The below scratch patch adds minimal mempool code for raid1. It passed simple stress test of resync + 3 dd writers. Although write throughput is rather slow in my qemu, I don't observe any temporary/permanent stuck ups.

 drivers/md/raid1.c  |   32 ++++++++++++++++++++++++++++----
 drivers/md/raid1.h  |    2 ++
 fs/bio.c            |   31 +++++++++++++++++++++----------
 include/linux/bio.h |    2 ++
 4 files changed, 53 insertions(+), 14 deletions(-)

drivers/md/raid1.c +28 -4

@@ -76,6 +76,14 @@ static void r1bio_pool_free(void *r1_bio
 	kfree(r1_bio);
 }
 
+static void r1_bio_destructor(struct bio *bio)
+{
+	r1bio_t *r1_bio = bio->bi_private;
+	conf_t *conf = r1_bio->mddev->private;
+
+	bio_free(bio, conf->r1_bio_set);
+}
+
 #define RESYNC_BLOCK_SIZE (64*1024)
 //#define RESYNC_BLOCK_SIZE PAGE_SIZE
 #define RESYNC_SECTORS (RESYNC_BLOCK_SIZE >> 9)

@@ -85,6 +93,7 @@ static void r1bio_pool_free(void *r1_bio
 static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data)
 {
 	struct pool_info *pi = data;
+	conf_t *conf = pi->mddev->private;
 	struct page *page;
 	r1bio_t *r1_bio;
 	struct bio *bio;

@@ -100,7 +109,8 @@ static void * r1buf_pool_alloc(gfp_t gfp
 	 * Allocate bios : 1 for reading, n-1 for writing
 	 */
 	for (j = pi->raid_disks ; j-- ; ) {
-		bio = bio_alloc(gfp_flags, RESYNC_PAGES);
+		bio = bio_alloc_bioset(gfp_flags, RESYNC_PAGES,
+				       conf->r1_bio_set);
 		if (!bio)
 			goto out_free_bio;
 		r1_bio->bios[j] = bio;

@@ -386,6 +396,10 @@ static void raid1_end_write_request(stru
 				!test_bit(R1BIO_Degraded, &r1_bio->state),
 				behind);
 		md_write_end(r1_bio->mddev);
+		if (to_put) {
+			bio_put(to_put);
+			to_put = NULL;
+		}
 		raid_end_bio_io(r1_bio);
 	}
 

@@ -851,7 +865,7 @@ static int make_request(mddev_t *mddev, 
 		}
 		r1_bio->read_disk = rdisk;
 
-		read_bio = bio_clone(bio, GFP_NOIO);
+		read_bio = bio_clone_bioset(bio, GFP_NOIO, conf->r1_bio_set);
 
 		r1_bio->bios[rdisk] = read_bio;
 

@@ -946,7 +960,7 @@ static int make_request(mddev_t *mddev, 
 		if (!r1_bio->bios[i])
 			continue;
 
-		mbio = bio_clone(bio, GFP_NOIO);
+		mbio = bio_clone_bioset(bio, GFP_NOIO, conf->r1_bio_set);
 		r1_bio->bios[i] = mbio;
 
 		mbio->bi_sector	= r1_bio->sector + conf->mirrors[i].rdev->data_offset;

@@ -1646,7 +1660,9 @@ static void raid1d(mddev_t *mddev)
 					mddev->ro ? IO_BLOCKED : NULL;
 				r1_bio->read_disk = disk;
 				bio_put(bio);
-				bio = bio_clone(r1_bio->master_bio, GFP_NOIO);
+				bio = bio_clone_bioset(r1_bio->master_bio,
+						       GFP_NOIO,
+						       conf->r1_bio_set);
 				r1_bio->bios[r1_bio->read_disk] = bio;
 				rdev = conf->mirrors[disk].rdev;
 				if (printk_ratelimit())

@@ -1948,6 +1964,10 @@ static conf_t *setup_conf(mddev_t *mddev
 					  conf->poolinfo);
 	if (!conf->r1bio_pool)
 		goto abort;
+	conf->r1_bio_set = bioset_create(mddev->raid_disks * 2, 0);
+	if (!conf->r1_bio_set)
+		goto abort;
+	conf->r1_bio_set->bio_destructor = r1_bio_destructor;
 
 	conf->poolinfo->mddev = mddev;
 

@@ -2012,6 +2032,8 @@ static conf_t *setup_conf(mddev_t *mddev
 	if (conf) {
 		if (conf->r1bio_pool)
 			mempool_destroy(conf->r1bio_pool);
+		if (conf->r1_bio_set)
+			bioset_free(conf->r1_bio_set);
 		kfree(conf->mirrors);
 		safe_put_page(conf->tmppage);
 		kfree(conf->poolinfo);

@@ -2121,6 +2143,8 @@ static int stop(mddev_t *mddev)
 	blk_sync_queue(mddev->queue); /* the unplug fn references 'conf'*/
 	if (conf->r1bio_pool)
 		mempool_destroy(conf->r1bio_pool);
+	if (conf->r1_bio_set)
+		bioset_free(conf->r1_bio_set);
 	kfree(conf->mirrors);
 	kfree(conf->poolinfo);
 	kfree(conf);

fs/bio.c +21 -10

@@ -306,6 +306,7 @@ out_set:
 	bio->bi_flags |= idx << BIO_POOL_OFFSET;
 	bio->bi_max_vecs = nr_iovecs;
 	bio->bi_io_vec = bvl;
+	bio->bi_destructor = bs->bio_destructor;
 	return bio;
 
 err_free:

@@ -340,12 +341,7 @@ static void bio_fs_destructor(struct bio
  */
 struct bio *bio_alloc(gfp_t gfp_mask, int nr_iovecs)
 {
-	struct bio *bio = bio_alloc_bioset(gfp_mask, nr_iovecs, fs_bio_set);
-
-	if (bio)
-		bio->bi_destructor = bio_fs_destructor;
-
-	return bio;
+	return bio_alloc_bioset(gfp_mask, nr_iovecs, fs_bio_set);
 }
 EXPORT_SYMBOL(bio_alloc);
 

@@ -460,20 +456,21 @@ void __bio_clone(struct bio *bio, struct
 EXPORT_SYMBOL(__bio_clone);
 
 /**
- *	bio_clone	-	clone a bio
+ *	bio_clone_bioset	-	clone a bio
  *	@bio: bio to clone
  *	@gfp_mask: allocation priority
+ *	@bs: bio_set to allocate from
  *
  * 	Like __bio_clone, only also allocates the returned bio
  */
-struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)
+struct bio *
+bio_clone_bioset(struct bio *bio, gfp_t gfp_mask, struct bio_set *bs)
 {
-	struct bio *b = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, fs_bio_set);
+	struct bio *b = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, bs);
 
 	if (!b)
 		return NULL;
 
-	b->bi_destructor = bio_fs_destructor;
 	__bio_clone(b, bio);
 
 	if (bio_integrity(bio)) {

@@ -489,6 +486,19 @@ struct bio *bio_clone(struct bio *bio, g
 
 	return b;
 }
+EXPORT_SYMBOL(bio_clone_bioset);
+
+/**
+ *	bio_clone	-	clone a bio
+ *	@bio: bio to clone
+ *	@gfp_mask: allocation priority
+ *
+ *	Like __bio_clone, only also allocates the returned bio
+ */
+struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)
+{
+	return bio_clone_bioset(bio, gfp_mask, fs_bio_set);
+}
 EXPORT_SYMBOL(bio_clone);
 
 /**

@@ -1664,6 +1674,7 @@ static int __init init_bio(void)
 	fs_bio_set = bioset_create(BIO_POOL_SIZE, 0);
 	if (!fs_bio_set)
 		panic("bio: can't allocate bios\n");
+	fs_bio_set->bio_destructor = bio_fs_destructor;
 
 	bio_split_pool = mempool_create_kmalloc_pool(BIO_SPLIT_ENTRIES,
 						     sizeof(struct bio_pair));

include/linux/bio.h +2 -0

@@ -227,6 +227,7 @@ extern int bio_phys_segments(struct requ
 
 extern void __bio_clone(struct bio *, struct bio *);
 extern struct bio *bio_clone(struct bio *, gfp_t);
+extern struct bio *bio_clone_bioset(struct bio *, gfp_t, struct bio_set *);
 
 extern void bio_init(struct bio *);
 

@@ -299,6 +300,7 @@ struct bio_set {
 	mempool_t *bio_integrity_pool;
 #endif
 	mempool_t *bvec_pool;
+	bio_destructor_t	*bio_destructor;
 };
 
 struct biovec_slab {

drivers/md/raid1.h +2 -0

@@ -60,6 +60,8 @@ struct r1_private_data_s {
 	mempool_t *r1bio_pool;
 	mempool_t *r1buf_pool;
 
+	struct bio_set *r1_bio_set;
+
 	/* When taking over an array from a different personality, we store
 	 * the new thread here until we fully activate the array.
 	 */

Quoted 22 lines · On Mon, 25 Oct 2010 00:52:34 +0800 Wu Fengguang <[email protected]> wrote:

Quoted 21 lines · Jens Axboe

Quoted 19 lines · Wu Fengguang

Quoted 5 lines

Quoted 4 lines

We surely need 1 set aside for each level of that stack that will potentially consume one. 1 should be enough for the generic pool, and then clones will use a separate pool. So md and friends should really have a pool per device, so that stacking will always work properly.

Quoted 8 lines

Agreed for the deadlock problem.

Quoted 2 lines

There should be no throughput concerns, it should purely be a safe guard measure to prevent us deadlocking when doing IO for reclaim.

It's easy to verify whether the minimal size will have negative impacts on IO throughput. In Torsten's case, increase BIO_POOL_SIZE by one and check how it performs.

Sorry it seems simply increasing BIO_POOL_SIZE is not enough to fix possible deadlocks. We need adding new mempool(s). Because when there BIO_POOL_SIZE=2 and there are two concurrent reclaimers each take 1 reservation, they will deadlock each other when trying to take the next bio at the raid1 level.

Quoted 8 lines

Quoted 3 lines

Yes, plus it's not a practical solution since you don't know how deep the stack is. As I wrote in the initial email, each consumer needs it's own private mempool (and just 1 entry should suffice).

You are right. The below scratch patch adds minimal mempool code for raid1. It passed simple stress test of resync + 3 dd writers. Although write throughput is rather slow in my qemu, I don't observe any temporary/permanent stuck ups.

Hi,
thanks for the patch. I'll make a few changes to what I finally apply - for example we don't really need mempools in r1buf_poll_alloc as that isn't on the writeout path - so I'll tidy that up first.

Also I'll avoid making changes to fs/bio.c at first. It may still be a good idea to have a bio_clone_bioset, but that should be a separate patch - there are at least 3 places that would use it.

Thanks - I'll try to get this into the current merge window.

Quoted 213 lines

 drivers/md/raid1.c  |   32 ++++++++++++++++++++++++++++----
 drivers/md/raid1.h  |    2 ++
 fs/bio.c            |   31 +++++++++++++++++++++----------
 include/linux/bio.h |    2 ++
 4 files changed, 53 insertions(+), 14 deletions(-)

drivers/md/raid1.c +28 -4

@@ -76,6 +76,14 @@ static void r1bio_pool_free(void *r1_bio
 	kfree(r1_bio);
 }
 
+static void r1_bio_destructor(struct bio *bio)
+{
+	r1bio_t *r1_bio = bio->bi_private;
+	conf_t *conf = r1_bio->mddev->private;
+
+	bio_free(bio, conf->r1_bio_set);
+}
+
 #define RESYNC_BLOCK_SIZE (64*1024)
 //#define RESYNC_BLOCK_SIZE PAGE_SIZE
 #define RESYNC_SECTORS (RESYNC_BLOCK_SIZE >> 9)

@@ -85,6 +93,7 @@ static void r1bio_pool_free(void *r1_bio
 static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data)
 {
 	struct pool_info *pi = data;
+	conf_t *conf = pi->mddev->private;
 	struct page *page;
 	r1bio_t *r1_bio;
 	struct bio *bio;

@@ -100,7 +109,8 @@ static void * r1buf_pool_alloc(gfp_t gfp
 	 * Allocate bios : 1 for reading, n-1 for writing
 	 */
 	for (j = pi->raid_disks ; j-- ; ) {
-		bio = bio_alloc(gfp_flags, RESYNC_PAGES);
+		bio = bio_alloc_bioset(gfp_flags, RESYNC_PAGES,
+				       conf->r1_bio_set);
 		if (!bio)
 			goto out_free_bio;
 		r1_bio->bios[j] = bio;

@@ -386,6 +396,10 @@ static void raid1_end_write_request(stru
 				!test_bit(R1BIO_Degraded, &r1_bio->state),
 				behind);
 		md_write_end(r1_bio->mddev);
+		if (to_put) {
+			bio_put(to_put);
+			to_put = NULL;
+		}
 		raid_end_bio_io(r1_bio);
 	}
 

@@ -851,7 +865,7 @@ static int make_request(mddev_t *mddev, 
 		}
 		r1_bio->read_disk = rdisk;
 
-		read_bio = bio_clone(bio, GFP_NOIO);
+		read_bio = bio_clone_bioset(bio, GFP_NOIO, conf->r1_bio_set);
 
 		r1_bio->bios[rdisk] = read_bio;
 

@@ -946,7 +960,7 @@ static int make_request(mddev_t *mddev, 
 		if (!r1_bio->bios[i])
 			continue;
 
-		mbio = bio_clone(bio, GFP_NOIO);
+		mbio = bio_clone_bioset(bio, GFP_NOIO, conf->r1_bio_set);
 		r1_bio->bios[i] = mbio;
 
 		mbio->bi_sector	= r1_bio->sector + conf->mirrors[i].rdev->data_offset;

@@ -1646,7 +1660,9 @@ static void raid1d(mddev_t *mddev)
 					mddev->ro ? IO_BLOCKED : NULL;
 				r1_bio->read_disk = disk;
 				bio_put(bio);
-				bio = bio_clone(r1_bio->master_bio, GFP_NOIO);
+				bio = bio_clone_bioset(r1_bio->master_bio,
+						       GFP_NOIO,
+						       conf->r1_bio_set);
 				r1_bio->bios[r1_bio->read_disk] = bio;
 				rdev = conf->mirrors[disk].rdev;
 				if (printk_ratelimit())

@@ -1948,6 +1964,10 @@ static conf_t *setup_conf(mddev_t *mddev
 					  conf->poolinfo);
 	if (!conf->r1bio_pool)
 		goto abort;
+	conf->r1_bio_set = bioset_create(mddev->raid_disks * 2, 0);
+	if (!conf->r1_bio_set)
+		goto abort;
+	conf->r1_bio_set->bio_destructor = r1_bio_destructor;
 
 	conf->poolinfo->mddev = mddev;
 

@@ -2012,6 +2032,8 @@ static conf_t *setup_conf(mddev_t *mddev
 	if (conf) {
 		if (conf->r1bio_pool)
 			mempool_destroy(conf->r1bio_pool);
+		if (conf->r1_bio_set)
+			bioset_free(conf->r1_bio_set);
 		kfree(conf->mirrors);
 		safe_put_page(conf->tmppage);
 		kfree(conf->poolinfo);

@@ -2121,6 +2143,8 @@ static int stop(mddev_t *mddev)
 	blk_sync_queue(mddev->queue); /* the unplug fn references 'conf'*/
 	if (conf->r1bio_pool)
 		mempool_destroy(conf->r1bio_pool);
+	if (conf->r1_bio_set)
+		bioset_free(conf->r1_bio_set);
 	kfree(conf->mirrors);
 	kfree(conf->poolinfo);
 	kfree(conf);

fs/bio.c +21 -10

@@ -306,6 +306,7 @@ out_set:
 	bio->bi_flags |= idx << BIO_POOL_OFFSET;
 	bio->bi_max_vecs = nr_iovecs;
 	bio->bi_io_vec = bvl;
+	bio->bi_destructor = bs->bio_destructor;
 	return bio;
 
 err_free:

@@ -340,12 +341,7 @@ static void bio_fs_destructor(struct bio
  */
 struct bio *bio_alloc(gfp_t gfp_mask, int nr_iovecs)
 {
-	struct bio *bio = bio_alloc_bioset(gfp_mask, nr_iovecs, fs_bio_set);
-
-	if (bio)
-		bio->bi_destructor = bio_fs_destructor;
-
-	return bio;
+	return bio_alloc_bioset(gfp_mask, nr_iovecs, fs_bio_set);
 }
 EXPORT_SYMBOL(bio_alloc);
 

@@ -460,20 +456,21 @@ void __bio_clone(struct bio *bio, struct
 EXPORT_SYMBOL(__bio_clone);
 
 /**
- *	bio_clone	-	clone a bio
+ *	bio_clone_bioset	-	clone a bio
  *	@bio: bio to clone
  *	@gfp_mask: allocation priority
+ *	@bs: bio_set to allocate from
  *
  * 	Like __bio_clone, only also allocates the returned bio
  */
-struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)
+struct bio *
+bio_clone_bioset(struct bio *bio, gfp_t gfp_mask, struct bio_set *bs)
 {
-	struct bio *b = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, fs_bio_set);
+	struct bio *b = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, bs);
 
 	if (!b)
 		return NULL;
 
-	b->bi_destructor = bio_fs_destructor;
 	__bio_clone(b, bio);
 
 	if (bio_integrity(bio)) {

@@ -489,6 +486,19 @@ struct bio *bio_clone(struct bio *bio, g
 
 	return b;
 }
+EXPORT_SYMBOL(bio_clone_bioset);
+
+/**
+ *	bio_clone	-	clone a bio
+ *	@bio: bio to clone
+ *	@gfp_mask: allocation priority
+ *
+ *	Like __bio_clone, only also allocates the returned bio
+ */
+struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)
+{
+	return bio_clone_bioset(bio, gfp_mask, fs_bio_set);
+}
 EXPORT_SYMBOL(bio_clone);
 
 /**

@@ -1664,6 +1674,7 @@ static int __init init_bio(void)
 	fs_bio_set = bioset_create(BIO_POOL_SIZE, 0);
 	if (!fs_bio_set)
 		panic("bio: can't allocate bios\n");
+	fs_bio_set->bio_destructor = bio_fs_destructor;
 
 	bio_split_pool = mempool_create_kmalloc_pool(BIO_SPLIT_ENTRIES,
 						     sizeof(struct bio_pair));

include/linux/bio.h +2 -0

@@ -227,6 +227,7 @@ extern int bio_phys_segments(struct requ
 
 extern void __bio_clone(struct bio *, struct bio *);
 extern struct bio *bio_clone(struct bio *, gfp_t);
+extern struct bio *bio_clone_bioset(struct bio *, gfp_t, struct bio_set *);
 
 extern void bio_init(struct bio *);
 

@@ -299,6 +300,7 @@ struct bio_set {
 	mempool_t *bio_integrity_pool;
 #endif
 	mempool_t *bvec_pool;
+	bio_destructor_t	*bio_destructor;
 };
 
 struct biovec_slab {

drivers/md/raid1.h +0 -0

@@ -60,6 +60,8 @@ struct r1_private_data_s {
 	mempool_t *r1bio_pool;
 	mempool_t *r1buf_pool;
 

drivers/md/raid1.h struct r1_private_data_s { · line 67

Quoted 5 lines

drivers/md/raid1.h +2 -0

+	struct bio_set *r1_bio_set;
+
 	/* When taking over an array from a different personality, we store
 	 * the new thread here until we fully activate the array.
 	 */

Quoted 5 lines · Torsten Kaiser

swap_writepage() uses get_swap_bio() which uses bio_alloc() to get one bio. That bio is the submitted, but the submit path seems to get into make_request from raid1.c and that allocates a second bio from bio_alloc() via bio_clone().

Quoted 3 lines

I am seeing this pattern (swap_writepage calling md_make_request/make_request and then getting stuck in mempool_alloc) more than 5 times in the SysRq+T output...

I bet the root cause is the failure of pool->alloc(__GFP_NORETRY) inside mempool_alloc(), which can be fixed by this patch.

Thanks,
Fengguang
---

concurrent direct page reclaim problem

 __GFP_NORETRY page allocations may fail when there are many concurrent page allocating tasks, but not necessary in real short of memory. The root cause is, tasks will first run direct page reclaim to free some pages from the LRU lists and put them to the per-cpu page lists and the buddy system, and then try to get a free page from there.  However the free pages reclaimed by this task may be consumed by other tasks when the direct reclaim task is able to get the free page for itself.

 Let's retry it a bit harder.

mm/page_alloc.c  2010-10-20 13:50:54.000000000 +0800 renamed from mm/page_alloc.c     2010-10-20 13:44:50.000000000 +0800 +1 -2

@@ -1700,7 +1700,7 @@ should_alloc_retry(gfp_t gfp_mask, unsig
                                unsigned long pages_reclaimed)
  {
        /* Do not loop if specifically requested */
-       if (gfp_mask & __GFP_NORETRY)
+       if (gfp_mask & __GFP_NORETRY && pages_reclaimed > (1 << (order + 12)))
                return 0;
 
        /*
--

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

Quoted 6 lines · Wu Fengguang

Quoted 5 lines · Torsten Kaiser

swap_writepage() uses get_swap_bio() which uses bio_alloc() to get one bio. That bio is the submitted, but the submit path seems to get into make_request from raid1.c and that allocates a second bio from bio_alloc() via bio_clone().

Quoted 6 lines

Quoted 3 lines

I am seeing this pattern (swap_writepage calling md_make_request/make_request and then getting stuck in mempool_alloc) more than 5 times in the SysRq+T output...

I bet the root cause is the failure of pool->alloc(__GFP_NORETRY) inside mempool_alloc(), which can be fixed by this patch.

No. I tested the patch (ontop of Neils fix and your patch regarding too_many_isolated()), but the system got stuck the same way on the first try to fill the tmpfs.
I think the basic problem is, that the mempool that should guarantee progress is exhausted because the raid1 device is stacked between the pageout code and the disks and so the "use only 1 bio"-rule gets violated.

Quoted 11 lines

---

concurrent direct page reclaim problem

A __GFP_NORETRY page allocations may fail when there are many concurrent page A allocating tasks, but not necessary in real short of memory. The root cause A is, tasks will first run direct page reclaim to free some pages from the LRU A lists and put them to the per-cpu page lists and the buddy system, and then A try to get a free page from there. A However the free pages reclaimed by this A task may be consumed by other tasks when the direct reclaim task is able to A get the free page for itself.

I believe the facts disagree with that assumtion. My bad for not posting this before, but I also used SysRq+M to see whats going on, but each time there still was some free memory.
Here is the SysRq+M output from the run with only Neils patch applied, but on each other run the same ~14Mb stayed free

[  437.481365] SysRq : Show Memory
[  437.490003] Mem-Info:
[  437.491357] Node 0 DMA per-cpu:
[  437.500032] CPU    0: hi:    0, btch:   1 usd:   0
[  437.500032] CPU    1: hi:    0, btch:   1 usd:   0
[  437.500032] CPU    2: hi:    0, btch:   1 usd:   0
[  437.500032] CPU    3: hi:    0, btch:   1 usd:   0
[  437.500032] Node 0 DMA32 per-cpu:
[  437.500032] CPU    0: hi:  186, btch:  31 usd: 138
[  437.500032] CPU    1: hi:  186, btch:  31 usd:  30
[  437.500032] CPU    2: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    3: hi:  186, btch:  31 usd:   0
[  437.500032] Node 1 DMA32 per-cpu:
[  437.500032] CPU    0: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    1: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    2: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    3: hi:  186, btch:  31 usd:   0
[  437.500032] Node 1 Normal per-cpu:
[  437.500032] CPU    0: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    1: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    2: hi:  186, btch:  31 usd:  25
[  437.500032] CPU    3: hi:  186, btch:  31 usd:  30
[  437.500032] active_anon:2039 inactive_anon:985233 isolated_anon:682
[  437.500032]  active_file:1667 inactive_file:1723 isolated_file:0
[  437.500032]  unevictable:0 dirty:0 writeback:25387 unstable:0
[  437.500032]  free:3471 slab_reclaimable:2840 slab_unreclaimable:6337
[  437.500032]  mapped:1284 shmem:960501 pagetables:523 bounce:0
[  437.500032] Node 0 DMA free:8008kB min:28kB low:32kB high:40kB

active_anon:0kB inact
ive_anon:7596kB active_file:12kB inactive_file:0kB unevictable:0kB isolated(anon):0kB i
solated(file):0kB present:15768kB mlocked:0kB dirty:0kB writeback:404kB mapped:0kB shme
m:7192kB slab_reclaimable:32kB slab_unreclaimable:304kB kernel_stack:0kB pagetables:0kB
unstable:0kB bounce:0kB writeback_tmp:0kB pages_scanned:118
all_unreclaimable? no

[  437.500032] lowmem_reserve[]: 0 2004 2004 2004
[  437.500032] Node 0 DMA32 free:2980kB min:4036kB low:5044kB

high:6052kB active_anon:2
844kB inactive_anon:1918424kB active_file:3428kB inactive_file:3780kB unevictable:0kB isolated(anon):1232kB isolated(file):0kB present:2052320kB mlocked:0kB dirty:0kB writeback:72016kB mapped:2232kB shmem:1847640kB slab_reclaimable:5444kB slab_unreclaimable:13508kB kernel_stack:744kB pagetables:864kB unstable:0kB bounce:0kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no

[  437.500032] lowmem_reserve[]: 0 0 0 0
[  437.500032] Node 1 DMA32 free:2188kB min:3036kB low:3792kB

high:4552kB active_anon:0kB inactive_anon:1555368kB active_file:0kB inactive_file:28kB unevictable:0kB isolated(anon):768kB isolated(file):0kB present:1544000kB mlocked:0kB dirty:0kB writeback:21160kB mapped:0kB shmem:1534960kB slab_reclaimable:3728kB slab_unreclaimable:7076kB kernel_stack:8kB pagetables:0kB unstable:0kB bounce:0kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no

[  437.500032] lowmem_reserve[]: 0 0 505 505
[  437.500032] Node 1 Normal free:708kB min:1016kB low:1268kB

high:1524kB active_anon:5312kB inactive_anon:459544kB active_file:3228kB inactive_file:3084kB unevictable:0kB isolated(anon):728kB isolated(file):0kB present:517120kB mlocked:0kB dirty:0kB writeback:7968kB mapped:2904kB shmem:452212kB slab_reclaimable:2156kB slab_unreclaimable:4460kB kernel_stack:200kB pagetables:1228kB unstable:0kB bounce:0kB writeback_tmp:0kB pages_scanned:9678 all_unreclaimable? no

[  437.500032] lowmem_reserve[]: 0 0 0 0
[  437.500032] Node 0 DMA: 2*4kB 2*8kB 1*16kB 3*32kB 3*64kB 4*128kB
4*256kB 2*512kB 1*1024kB 2*2048kB 0*4096kB = 8008kB
[  437.500032] Node 0 DMA32: 27*4kB 15*8kB 8*16kB 8*32kB 7*64kB
1*128kB 1*256kB 1*512kB 1*1024kB 0*2048kB 0*4096kB = 2980kB
[  437.500032] Node 1 DMA32: 1*4kB 6*8kB 3*16kB 1*32kB 0*64kB 1*128kB
0*256kB 0*512kB 0*1024kB 1*2048kB 0*4096kB = 2308kB
[  437.500032] Node 1 Normal: 39*4kB 13*8kB 10*16kB 3*32kB 1*64kB
1*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB = 708kB
[  437.500032] 989289 total pagecache pages
[  437.500032] 25398 pages in swap cache
[  437.500032] Swap cache stats: add 859204, delete 833806, find 28/39
[  437.500032] Free swap  = 9865628kB
[  437.500032] Total swap = 10000316kB
[  437.500032] 1048575 pages RAM
[  437.500032] 33809 pages reserved
[  437.500032] 7996 pages shared
[  437.500032] 1008521 pages non-shared

Quoted 12 lines

A Let's retry it a bit harder.

mm/page_alloc.c A 2010-10-20 13:50:54.000000000 +0800 renamed from mm/page_alloc.c A A 2010-10-20 13:44:50.000000000 +0800 +0 -0

@@ -1700,7 +1700,7 @@ should_alloc_retry(gfp_t gfp_mask, unsig
 A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A unsigned long pages_reclaimed)
 A {

A A A A /* Do not loop if specifically requested */

- A  A  A  if (gfp_mask & __GFP_NORETRY)
+ A  A  A  if (gfp_mask & __GFP_NORETRY && pages_reclaimed > (1 << (order + 12)))

A A A A A A A A return 0;

Quoted 7 lines

A A A A /*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at A http://vger.kernel.org/majordomo-info.html
Please read the FAQ at A http://www.tux.org/lkml/

Quoted 14 lines · Torsten Kaiser

Quoted 6 lines · Wu Fengguang

Quoted 5 lines · Torsten Kaiser

swap_writepage() uses get_swap_bio() which uses bio_alloc() to get one bio. That bio is the submitted, but the submit path seems to get into make_request from raid1.c and that allocates a second bio from bio_alloc() via bio_clone().

Quoted 6 lines

Quoted 3 lines

I am seeing this pattern (swap_writepage calling md_make_request/make_request and then getting stuck in mempool_alloc) more than 5 times in the SysRq+T output...

I bet the root cause is the failure of pool->alloc(__GFP_NORETRY) inside mempool_alloc(), which can be fixed by this patch.

Quoted 7 lines

No. I tested the patch (ontop of Neils fix and your patch regarding too_many_isolated()), but the system got stuck the same way on the first try to fill the tmpfs.
I think the basic problem is, that the mempool that should guarantee progress is exhausted because the raid1 device is stacked between the pageout code and the disks and so the "use only 1 bio"-rule gets violated.

The mempool get exhausted because pool->alloc() failed at least 2 times. But there are no such high memory pressure except for some parallel reclaimers. It seems the below patch does not completely stop the page allocation failure, hence does not stop the deadlock.

As you and KOSAKI said, the root cause is BIO_POOL_SIZE being smaller than the total possible allocations in the IO stack. Then why not bumping up BIO_POOL_SIZE to something like 64? It will be large enough to allow multiple stacked IO layers.

And the larger value will allow more concurrent flying IOs for better IO throughput in such situation. Commit 5972511b7 lowers it from 256 to 2 because it believes that pool->alloc() will only fail on somehow OOM situation. However truth is __GFP_NORETRY allocations fail much more easily in _normal_ operations (whenever there are multiple concurrent page reclaimers). We have to be able to perform better in such situation.  The __GFP_NORETRY patch to reduce failures is one option, increasing BIO_POOL_SIZE is another.

So would you try this fix?

include/linux/bio.h      2010-10-20 16:56:54.000000000 +0800 renamed from include/linux/bio.h 2010-10-20 16:55:57.000000000 +0800 +1 -1

@@ -286,7 +286,7 @@ static inline void bio_set_completion_cp
  * These memory pools in turn all allocate from the bio_slab
  * and the bvec_slabs[].
  */
-#define BIO_POOL_SIZE 2
+#define BIO_POOL_SIZE  64
  #define BIOVEC_NR_POOLS 6
  #define BIOVEC_MAX_IDX (BIOVEC_NR_POOLS - 1)

Quoted 6 lines · Wu Fengguang

Quoted 5 lines · Torsten Kaiser

swap_writepage() uses get_swap_bio() which uses bio_alloc() to get one bio. That bio is the submitted, but the submit path seems to get into make_request from raid1.c and that allocates a second bio from bio_alloc() via bio_clone().

Quoted 6 lines

Quoted 3 lines

I am seeing this pattern (swap_writepage calling md_make_request/make_request and then getting stuck in mempool_alloc) more than 5 times in the SysRq+T output...

I bet the root cause is the failure of pool->alloc(__GFP_NORETRY) inside mempool_alloc(), which can be fixed by this patch.

No. I tested the patch (ontop of Neils fix and your patch regarding too_many_isolated()), but the system got stuck the same way on the first try to fill the tmpfs.
I think the basic problem is, that the mempool that should guarantee progress is exhausted because the raid1 device is stacked between the pageout code and the disks and so the "use only 1 bio"-rule gets violated.

Quoted 6 lines

Thanks,
Fengguang
---

concurrent direct page reclaim problem

Quoted 7 lines

?__GFP_NORETRY page allocations may fail when there are many concurrent page ?allocating tasks, but not necessary in real short of memory. The root cause ?is, tasks will first run direct page reclaim to free some pages from the LRU ?lists and put them to the per-cpu page lists and the buddy system, and then ?try to get a free page from there. ?However the free pages reclaimed by this ?task may be consumed by other tasks when the direct reclaim task is able to ?get the free page for itself.

I believe the facts disagree with that assumtion. My bad for not posting this before, but I also used SysRq+M to see whats going on, but each time there still was some free memory.
Here is the SysRq+M output from the run with only Neils patch applied, but on each other run the same ~14Mb stayed free

[  437.481365] SysRq : Show Memory
[  437.490003] Mem-Info:
[  437.491357] Node 0 DMA per-cpu:
[  437.500032] CPU    0: hi:    0, btch:   1 usd:   0
[  437.500032] CPU    1: hi:    0, btch:   1 usd:   0
[  437.500032] CPU    2: hi:    0, btch:   1 usd:   0
[  437.500032] CPU    3: hi:    0, btch:   1 usd:   0
[  437.500032] Node 0 DMA32 per-cpu:
[  437.500032] CPU    0: hi:  186, btch:  31 usd: 138
[  437.500032] CPU    1: hi:  186, btch:  31 usd:  30
[  437.500032] CPU    2: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    3: hi:  186, btch:  31 usd:   0
[  437.500032] Node 1 DMA32 per-cpu:
[  437.500032] CPU    0: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    1: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    2: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    3: hi:  186, btch:  31 usd:   0
[  437.500032] Node 1 Normal per-cpu:
[  437.500032] CPU    0: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    1: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    2: hi:  186, btch:  31 usd:  25
[  437.500032] CPU    3: hi:  186, btch:  31 usd:  30
[  437.500032] active_anon:2039 inactive_anon:985233 isolated_anon:682
[  437.500032]  active_file:1667 inactive_file:1723 isolated_file:0
[  437.500032]  unevictable:0 dirty:0 writeback:25387 unstable:0
[  437.500032]  free:3471 slab_reclaimable:2840 slab_unreclaimable:6337
[  437.500032]  mapped:1284 shmem:960501 pagetables:523 bounce:0
[  437.500032] Node 0 DMA free:8008kB min:28kB low:32kB high:40kB

active_anon:0kB inact
ive_anon:7596kB active_file:12kB inactive_file:0kB unevictable:0kB isolated(anon):0kB i
solated(file):0kB present:15768kB mlocked:0kB dirty:0kB writeback:404kB mapped:0kB shme
m:7192kB slab_reclaimable:32kB slab_unreclaimable:304kB kernel_stack:0kB pagetables:0kB
unstable:0kB bounce:0kB writeback_tmp:0kB pages_scanned:118
all_unreclaimable? no

[  437.500032] lowmem_reserve[]: 0 2004 2004 2004

[  437.500032] Node 0 DMA32 free:2980kB min:4036kB low:5044kB

high:6052kB active_anon:2
844kB inactive_anon:1918424kB active_file:3428kB inactive_file:3780kB unevictable:0kB isolated(anon):1232kB isolated(file):0kB present:2052320kB mlocked:0kB dirty:0kB writeback:72016kB mapped:2232kB shmem:1847640kB slab_reclaimable:5444kB slab_unreclaimable:13508kB kernel_stack:744kB pagetables:864kB unstable:0kB bounce:0kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no

[  437.500032] lowmem_reserve[]: 0 0 0 0

[  437.500032] Node 1 DMA32 free:2188kB min:3036kB low:3792kB

high:4552kB active_anon:0kB inactive_anon:1555368kB active_file:0kB inactive_file:28kB unevictable:0kB isolated(anon):768kB isolated(file):0kB present:1544000kB mlocked:0kB dirty:0kB writeback:21160kB mapped:0kB shmem:1534960kB slab_reclaimable:3728kB slab_unreclaimable:7076kB kernel_stack:8kB pagetables:0kB unstable:0kB bounce:0kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no

[  437.500032] lowmem_reserve[]: 0 0 505 505

[  437.500032] Node 1 Normal free:708kB min:1016kB low:1268kB

high:1524kB active_anon:5312kB inactive_anon:459544kB active_file:3228kB inactive_file:3084kB unevictable:0kB isolated(anon):728kB isolated(file):0kB present:517120kB mlocked:0kB dirty:0kB writeback:7968kB mapped:2904kB shmem:452212kB slab_reclaimable:2156kB slab_unreclaimable:4460kB kernel_stack:200kB pagetables:1228kB unstable:0kB bounce:0kB writeback_tmp:0kB pages_scanned:9678 all_unreclaimable? no

[  437.500032] lowmem_reserve[]: 0 0 0 0

[  437.500032] Node 0 DMA: 2*4kB 2*8kB 1*16kB 3*32kB 3*64kB 4*128kB
4*256kB 2*512kB 1*1024kB 2*2048kB 0*4096kB = 8008kB
[  437.500032] Node 0 DMA32: 27*4kB 15*8kB 8*16kB 8*32kB 7*64kB
1*128kB 1*256kB 1*512kB 1*1024kB 0*2048kB 0*4096kB = 2980kB
[  437.500032] Node 1 DMA32: 1*4kB 6*8kB 3*16kB 1*32kB 0*64kB 1*128kB
0*256kB 0*512kB 0*1024kB 1*2048kB 0*4096kB = 2308kB
[  437.500032] Node 1 Normal: 39*4kB 13*8kB 10*16kB 3*32kB 1*64kB
1*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB = 708kB
[  437.500032] 989289 total pagecache pages

[  437.500032] 25398 pages in swap cache
[  437.500032] Swap cache stats: add 859204, delete 833806, find 28/39
[  437.500032] Free swap  = 9865628kB
[  437.500032] Total swap = 10000316kB
[  437.500032] 1048575 pages RAM
[  437.500032] 33809 pages reserved
[  437.500032] 7996 pages shared
[  437.500032] 1008521 pages non-shared

Hello

Quoted 36 lines · Torsten Kaiser

Quoted 6 lines · Wu Fengguang

Quoted 5 lines · Torsten Kaiser

swap_writepage() uses get_swap_bio() which uses bio_alloc() to get one bio. That bio is the submitted, but the submit path seems to get into make_request from raid1.c and that allocates a second bio from bio_alloc() via bio_clone().

Quoted 6 lines

Quoted 3 lines

I am seeing this pattern (swap_writepage calling md_make_request/make_request and then getting stuck in mempool_alloc) more than 5 times in the SysRq+T output...

I bet the root cause is the failure of pool->alloc(__GFP_NORETRY) inside mempool_alloc(), which can be fixed by this patch.

No. I tested the patch (ontop of Neils fix and your patch regarding too_many_isolated()), but the system got stuck the same way on the first try to fill the tmpfs.
I think the basic problem is, that the mempool that should guarantee progress is exhausted because the raid1 device is stacked between the pageout code and the disks and so the "use only 1 bio"-rule gets violated.

Quoted 13 lines

Thanks,
Fengguang
---

concurrent direct page reclaim problem

?__GFP_NORETRY page allocations may fail when there are many concurrent page ?allocating tasks, but not necessary in real short of memory. The root cause ?is, tasks will first run direct page reclaim to free some pages from the LRU ?lists and put them to the per-cpu page lists and the buddy system, and then ?try to get a free page from there. ?However the free pages reclaimed by this ?task may be consumed by other tasks when the direct reclaim task is able to ?get the free page for itself.

Quoted 5 lines

I believe the facts disagree with that assumtion. My bad for not posting this before, but I also used SysRq+M to see whats going on, but each time there still was some free memory.
Here is the SysRq+M output from the run with only Neils patch applied, but on each other run the same ~14Mb stayed free

What is your problem?(Sorry if you explained it several time).

I read the thread.
It seems Wu's patch solved deadlock problem by FS lock holding and too_many_isolated. What is the problem remained in your case? unusable system by swapstorm? If it is, I think it's expected behavior. Please see the below comment. (If I don't catch your point, Please explain your problem.)

Quoted 30 lines

[  437.481365] SysRq : Show Memory
[  437.490003] Mem-Info:
[  437.491357] Node 0 DMA per-cpu:
[  437.500032] CPU    0: hi:    0, btch:   1 usd:   0
[  437.500032] CPU    1: hi:    0, btch:   1 usd:   0
[  437.500032] CPU    2: hi:    0, btch:   1 usd:   0
[  437.500032] CPU    3: hi:    0, btch:   1 usd:   0
[  437.500032] Node 0 DMA32 per-cpu:
[  437.500032] CPU    0: hi:  186, btch:  31 usd: 138
[  437.500032] CPU    1: hi:  186, btch:  31 usd:  30
[  437.500032] CPU    2: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    3: hi:  186, btch:  31 usd:   0
[  437.500032] Node 1 DMA32 per-cpu:
[  437.500032] CPU    0: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    1: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    2: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    3: hi:  186, btch:  31 usd:   0
[  437.500032] Node 1 Normal per-cpu:
[  437.500032] CPU    0: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    1: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    2: hi:  186, btch:  31 usd:  25
[  437.500032] CPU    3: hi:  186, btch:  31 usd:  30
[  437.500032] active_anon:2039 inactive_anon:985233 isolated_anon:682 [  437.500032]  active_file:1667 inactive_file:1723 isolated_file:0 [  437.500032]  unevictable:0 dirty:0 writeback:25387 unstable:0 [  437.500032]  free:3471 slab_reclaimable:2840 slab_unreclaimable:6337 [  437.500032]  mapped:1284 shmem:960501 pagetables:523 bounce:0 [  437.500032] Node 0 DMA free:8008kB min:28kB low:32kB high:40kB active_anon:0kB inact
ive_anon:7596kB active_file:12kB inactive_file:0kB unevictable:0kB

Quoted 8 lines

isolated(anon):0kB i
solated(file):0kB present:15768kB mlocked:0kB dirty:0kB writeback:404kB mapped:0kB shme
m:7192kB slab_reclaimable:32kB slab_unreclaimable:304kB kernel_stack:0kB pagetables:0kB
 unstable:0kB bounce:0kB writeback_tmp:0kB pages_scanned:118 all_unreclaimable? no
[  437.500032] lowmem_reserve[]: 0 2004 2004 2004

Node 0 DMA : free 8008K but lowmem_reserve 8012K(2004 pages) So page allocator can't allocate the page unless preferred zone is DMA

Quoted 10 lines

[  437.500032] Node 0 DMA32 free:2980kB min:4036kB low:5044kB high:6052kB active_anon:2
844kB inactive_anon:1918424kB active_file:3428kB inactive_file:3780kB unevictable:0kB isolated(anon):1232kB isolated(file):0kB present:2052320kB mlocked:0kB dirty:0kB writeback:72016kB mapped:2232kB shmem:1847640kB slab_reclaimable:5444kB slab_unreclaimable:13508kB kernel_stack:744kB pagetables:864kB unstable:0kB bounce:0kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
[  437.500032] lowmem_reserve[]: 0 0 0 0

Node 0 DMA32 : free 2980K but min 4036K.
Few file LRU compare to anon LRU

Normally, it could fail to allocate the page.
'Normal' means caller doesn't request alloc_pages with __GFP_HIGH or !__GFP_WAIT Generally many call sites don't pass gfp_flag with __GFP_HIGH|!__GFP_WAIT

On Wed, Oct 20, 2010 at 4:23 PM, Minchan Kim <[email protected]> wrote:

Quoted 1 line

What is your problem?(Sorry if you explained it several time).

The original problem was that using too many gcc's caused a swapstorm that completely hung my system.
I first blame it one the workqueue changes in 2.6.36-rc1 and/or its interaction with XFS (because in -rc5 a workqueue related problem in XFS got fixed), but Tejun Heo found out that a) it were exhausted mempools and not the workqueues and that the problems itself existed at least in 2.6.35 already. In
http://marc.info/?l=linux-raid&m=128699402805191&w=2 I have describe a simpler testcase that I found after looking more closely into the mempools.

Short story: swaping over RAID1 (drivers/md/raid1.c) can cause a system hang, because it is using too much of the fs_bio_set mempool from fs/bio.c.

Quoted 5 lines

I read the thread.
It seems Wu's patch solved deadlock problem by FS lock holding and too_many_isolated. What is the problem remained in your case? unusable system by swapstorm? If it is, I think it's expected behavior. Please see the below comment. (If I don't catch your point, Please explain your problem.)

I do not have a problem, if the system becomes unusable *during* a swapstorm, but it should recover. That is not the case in my system. With Wu's too_many_isolated-patch and Neil's patch agains raid1.c the system does no longer seem to be completely stuck (a swapoutrate of ~80kb every 20 seconds still happens), but I would still expect a better recovery time. (At that rate the recovery would probably take a few days...)

Quoted 54 lines

Quoted 31 lines

[  437.481365] SysRq : Show Memory
[  437.490003] Mem-Info:
[  437.491357] Node 0 DMA per-cpu:
[  437.500032] CPU    0: hi:    0, btch:   1 usd:   0
[  437.500032] CPU    1: hi:    0, btch:   1 usd:   0
[  437.500032] CPU    2: hi:    0, btch:   1 usd:   0
[  437.500032] CPU    3: hi:    0, btch:   1 usd:   0
[  437.500032] Node 0 DMA32 per-cpu:
[  437.500032] CPU    0: hi:  186, btch:  31 usd: 138
[  437.500032] CPU    1: hi:  186, btch:  31 usd:  30
[  437.500032] CPU    2: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    3: hi:  186, btch:  31 usd:   0
[  437.500032] Node 1 DMA32 per-cpu:
[  437.500032] CPU    0: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    1: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    2: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    3: hi:  186, btch:  31 usd:   0
[  437.500032] Node 1 Normal per-cpu:
[  437.500032] CPU    0: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    1: hi:  186, btch:  31 usd:   0
[  437.500032] CPU    2: hi:  186, btch:  31 usd:  25
[  437.500032] CPU    3: hi:  186, btch:  31 usd:  30
[  437.500032] active_anon:2039 inactive_anon:985233 isolated_anon:682 [  437.500032]  active_file:1667 inactive_file:1723 isolated_file:0 [  437.500032]  unevictable:0 dirty:0 writeback:25387 unstable:0 [  437.500032]  free:3471 slab_reclaimable:2840 slab_unreclaimable:6337 [  437.500032]  mapped:1284 shmem:960501 pagetables:523 bounce:0 [  437.500032] Node 0 DMA free:8008kB min:28kB low:32kB high:40kB active_anon:0kB inact
ive_anon:7596kB active_file:12kB inactive_file:0kB unevictable:0kB

Quoted 8 lines

isolated(anon):0kB i
solated(file):0kB present:15768kB mlocked:0kB dirty:0kB writeback:404kB mapped:0kB shme
m:7192kB slab_reclaimable:32kB slab_unreclaimable:304kB kernel_stack:0kB pagetables:0kB
 unstable:0kB bounce:0kB writeback_tmp:0kB pages_scanned:118 all_unreclaimable? no
[  437.500032] lowmem_reserve[]: 0 2004 2004 2004

Node 0 DMA : free 8008K but lowmem_reserve 8012K(2004 pages) So page allocator can't allocate the page unless preferred zone is DMA

Quoted 10 lines

[  437.500032] Node 0 DMA32 free:2980kB min:4036kB low:5044kB high:6052kB active_anon:2
844kB inactive_anon:1918424kB active_file:3428kB inactive_file:3780kB unevictable:0kB isolated(anon):1232kB isolated(file):0kB present:2052320kB mlocked:0kB dirty:0kB writeback:72016kB mapped:2232kB shmem:1847640kB slab_reclaimable:5444kB slab_unreclaimable:13508kB kernel_stack:744kB pagetables:864kB unstable:0kB bounce:0kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
[  437.500032] lowmem_reserve[]: 0 0 0 0

Quoted 2 lines

Node 0 DMA32 : free 2980K but min 4036K.
Few file LRU compare to anon LRU

In the testcase I fill a tmpfs as fast as I can with data from /dev/zero. So nearly everything gets swapped out and only the last written data from the tmpfs fills all RAM. (I have 4GB RAM, the tmpfs is limited to 6GB, 16 dd's are writing into it)

Quoted 51 lines

Normally, it could fail to allocate the page.
'Normal' means caller doesn't request alloc_pages with __GFP_HIGH or !__GFP_WAIT Generally many call sites don't pass gfp_flag with __GFP_HIGH|!__GFP_WAIT.

Quoted 8 lines

[  437.500032] Node 1 DMA32 free:2188kB min:3036kB low:3792kB high:4552kB active_anon:0kB inactive_anon:1555368kB active_file:0kB inactive_file:28kB unevictable:0kB isolated(anon):768kB isolated(file):0kB present:1544000kB mlocked:0kB dirty:0kB writeback:21160kB mapped:0kB shmem:1534960kB slab_reclaimable:3728kB slab_unreclaimable:7076kB kernel_stack:8kB pagetables:0kB unstable:0kB bounce:0kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no [  437.500032] lowmem_reserve[]: 0 0 505 505

Node 1 DMA32 free : 2188K min 3036K
It's a same situation with Node 0 DMA32.
Normally, it could fail to allocate the page.
Few file LRU compare to anon LRU

Quoted 9 lines

[  437.500032] Node 1 Normal free:708kB min:1016kB low:1268kB high:1524kB active_anon:5312kB inactive_anon:459544kB active_file:3228kB inactive_file:3084kB unevictable:0kB isolated(anon):728kB isolated(file):0kB present:517120kB mlocked:0kB dirty:0kB writeback:7968kB mapped:2904kB shmem:452212kB slab_reclaimable:2156kB slab_unreclaimable:4460kB kernel_stack:200kB pagetables:1228kB unstable:0kB bounce:0kB writeback_tmp:0kB pages_scanned:9678 all_unreclaimable? no
[  437.500032] lowmem_reserve[]: 0 0 0 0

Node 1 Normal : free 708K min 1016K
Normally, it could fail to allocate the page.
Few file LRU compare to anon LRU

Quoted 9 lines

[  437.500032] Node 0 DMA: 2*4kB 2*8kB 1*16kB 3*32kB 3*64kB 4*128kB 4*256kB 2*512kB 1*1024kB 2*2048kB 0*4096kB = 8008kB
[  437.500032] Node 0 DMA32: 27*4kB 15*8kB 8*16kB 8*32kB 7*64kB 1*128kB 1*256kB 1*512kB 1*1024kB 0*2048kB 0*4096kB = 2980kB [  437.500032] Node 1 DMA32: 1*4kB 6*8kB 3*16kB 1*32kB 0*64kB 1*128kB 0*256kB 0*512kB 0*1024kB 1*2048kB 0*4096kB = 2308kB
[  437.500032] Node 1 Normal: 39*4kB 13*8kB 10*16kB 3*32kB 1*64kB 1*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB = 708kB [  437.500032] 989289 total pagecache pages

Quoted 9 lines

[  437.500032] 25398 pages in swap cache
[  437.500032] Swap cache stats: add 859204, delete 833806, find 28/39 [  437.500032] Free swap  = 9865628kB
[  437.500032] Total swap = 10000316kB
[  437.500032] 1048575 pages RAM
[  437.500032] 33809 pages reserved
[  437.500032] 7996 pages shared
[  437.500032] 1008521 pages non-shared

Quoted 3 lines

All zones don't have enough pages and don't have enough file lru pages. So swapout is expected behavior, I think.
It means your workload exceeds your system available DRAM size.

Yes, as intended. I wanted to create many writes to a RAID1 device under memory pressure to show/verify that the current use of mempools in raid1.c is buggered.

That is not really any sane workload, that literally is just there to create a swapstorm and then see if the system survives it.

The problem is, that the system is not surviving it: bio allocations fail in raid1.c and it falls back to the fs_bio_set mempool. But that mempool is only 2 entries big, because you should ever only use one of its entries at a time. But the current mainline code from raid1.c allocates one bio per drive before submitting it -> That bug is fixed my Neil's patch and I would have expected that to fix my hang. But it seems that there is an additional problem so that mempool still get emptied. And that means that no writeback happens any longer and so the kernel can't swapout and gets stuck.

I think the last mail from Jens Axboe is the correct answer, not increasing the fs_bio_set mempool size via BIO_POOL_SIZE.

But should that go even further: Forbid any use of bio_alloc() and bio_clone() in any device drivers? Or at the very least in all device drivers that could be used for swapspace?

Torsten

Quoted 44 lines · On Wed, 15 Sep 2010 18:44:34 +1000 Neil Brown <[email protected]> wrote:

Quoted 32 lines · On Wed, 15 Sep 2010 16:28:43 +0800 Wu Fengguang <[email protected]> wrote:

Neil,

Sorry for the rushed and imaginary ideas this morning..

Review static unsigned long shrink_inactive_lis · line 1109

Quoted 10 lines

(quoted diff) +6 -0

@@ -1101,6 +1101,12 @@ static unsigned long shrink_inactive_lis
 	int lumpy_reclaim = 0;
 
 	while (unlikely(too_many_isolated(zone, file, sc))) {
+		if ((sc->gfp_mask & GFP_IOFS) != GFP_IOFS)
+			/* Not allowed to do IO, so mustn't wait
+			 * on processes that might try to
+			 */
+			return SWAP_CLUSTER_MAX;
+

The above patch should behavior like this: it returns SWAP_CLUSTER_MAX to cheat all the way up to believe "enough pages have been reclaimed". So __alloc_pages_direct_reclaim() see non-zero *did_some_progress and go on to call get_page_from_freelist(). That normally fails because the task didn't really scanned the LRU lists. However it does have the possibility to succeed -- when so many processes are doing concurrent direct reclaims, it may luckily get one free page reclaimed by other tasks. What's more, if it does fail to get a free page, the upper layer __alloc_pages_slowpath() will be repeat recalling __alloc_pages_direct_reclaim(). So, sooner or later it will succeed in "stealing" a free page reclaimed by other tasks.

In summary, the patch behavior for !__GFP_IO/FS is
- won't do any page reclaim
- won't fail the page allocation (unexpected)
- will wait and steal one free page from others (unreasonable)

Quoted 5 lines

So it will address the problem you encountered, however it sounds pretty unexpected and illogical behavior, right?

I believe this patch will address the problem equally well. What do you think?

Thank you for the detailed explanation. Is agree with your reasoning and now understand why your patch is sufficient.

Quoted 1 line

I will get it tested and let you know how that goes.

(sorry this has taken a month to follow up).

Testing shows that this patch seems to work.
The test load (essentially kernbench) doesn't deadlock any more, though it

does get bogged down thrashing in swap so it doesn't make a lot more progress :-) I guess that is to be expected.

One observation is that the kernbench generated 10%-20% more context switches with the patch than without. Is that to be expected?

Do you have plans for sending this patch upstream?

Neil find that if too_many_isolated() returns true while performing direct reclaim we can end up waiting for other threads to complete their direct reclaim. If those threads are allowed to enter the FS or IO to free memory, but this thread is not, then it is possible that those threads will be waiting on this thread and so we get a circular deadlock.

some task enters direct reclaim with GFP_KERNEL
=> too_many_isolated() false
=> vmscan and run into dirty pages
=> pageout()
=> take some FS lock
=> fs/block code does GFP_NOIO allocation
=> enter direct reclaim again
=> too_many_isolated() true
=> waiting for others to progress, however the other
tasks may be circular waiting for the FS lock..

The fix is to let !__GFP_IO and !__GFP_FS direct reclaims enjoy higher priority than normal ones, by honouring them higher throttle threshold.

Now !GFP_IOFS reclaims won't be waiting for GFP_IOFS reclaims to progress. They will be blocked only when there are too many concurrent !GFP_IOFS reclaims, however that's very unlikely because the IO-less direct reclaims is able to progress much more faster, and they won't deadlock each other. The threshold is raised high enough for them, so that there can be sufficient parallel progress of !GFP_IOFS reclaims.

Quoted 21 lines · On Tue, 19 Oct 2010 00:15:04 +0800 Wu Fengguang <[email protected]> wrote:

Neil find that if too_many_isolated() returns true while performing direct reclaim we can end up waiting for other threads to complete their direct reclaim. If those threads are allowed to enter the FS or IO to free memory, but this thread is not, then it is possible that those threads will be waiting on this thread and so we get a circular deadlock.

some task enters direct reclaim with GFP_KERNEL
=> too_many_isolated() false
=> vmscan and run into dirty pages
=> pageout()
=> take some FS lock
=> fs/block code does GFP_NOIO allocation
=> enter direct reclaim again
=> too_many_isolated() true
=> waiting for others to progress, however the other
tasks may be circular waiting for the FS lock..

The fix is to let !__GFP_IO and !__GFP_FS direct reclaims enjoy higher priority than normal ones, by honouring them higher throttle threshold.

Quoted 6 lines

Now !GFP_IOFS reclaims won't be waiting for GFP_IOFS reclaims to progress. They will be blocked only when there are too many concurrent !GFP_IOFS reclaims, however that's very unlikely because the IO-less direct reclaims is able to progress much more faster, and they won't deadlock each other. The threshold is raised high enough for them, so that there can be sufficient parallel progress of !GFP_IOFS reclaims.

I'm not sure that this is really a full fix. Torsten's analysis does appear to point at the real bug: raid1 has code paths which allocate more than a single element from a mempool without starting IO against previous elements.

Giving these allocations the ability to dip further into reserves will make occurrence of the bug less likely, but if enough threads all do this at the same time, that reserve will be exhausted and we're back to square one?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Quoted 10 lines · On Mon, 18 Oct 2010 14:58:59 -0700 Andrew Morton <[email protected]>…

Quoted 7 lines · On Tue, 19 Oct 2010 00:15:04 +0800 Wu Fengguang <[email protected]> wrote:

Neil find that if too_many_isolated() returns true while performing direct reclaim we can end up waiting for other threads to complete their direct reclaim. If those threads are allowed to enter the FS or IO to free memory, but this thread is not, then it is possible that those threads will be waiting on this thread and so we get a circular deadlock.

Quoted 10 lines

Quoted 10 lines

some task enters direct reclaim with GFP_KERNEL
=> too_many_isolated() false
=> vmscan and run into dirty pages
=> pageout()
=> take some FS lock
=> fs/block code does GFP_NOIO allocation
=> enter direct reclaim again
=> too_many_isolated() true
=> waiting for others to progress, however the other
tasks may be circular waiting for the FS lock..

Quoted 10 lines

Quoted 9 lines

The fix is to let !__GFP_IO and !__GFP_FS direct reclaims enjoy higher priority than normal ones, by honouring them higher throttle threshold.

Now !GFP_IOFS reclaims won't be waiting for GFP_IOFS reclaims to progress. They will be blocked only when there are too many concurrent !GFP_IOFS reclaims, however that's very unlikely because the IO-less direct reclaims is able to progress much more faster, and they won't deadlock each other. The threshold is raised high enough for them, so that there can be sufficient parallel progress of !GFP_IOFS reclaims.

Quoted 4 lines

I'm not sure that this is really a full fix. Torsten's analysis does appear to point at the real bug: raid1 has code paths which allocate more than a single element from a mempool without starting IO against previous elements.

... point at "a" real bug.

I think there are two bugs here.
The raid1 bug that Torsten mentions is certainly real (and has been around for an embarrassingly long time).
The bug that I identified in too_many_isolated is also a real bug and can be triggered without md/raid1 in the mix.
So this is not a 'full fix' for every bug in the kernel :-), but it could well be a full fix for this particular bug.

Quoted 8 lines

I think there are two bugs here.
The raid1 bug that Torsten mentions is certainly real (and has been around for an embarrassingly long time).
The bug that I identified in too_many_isolated is also a real bug and can be triggered without md/raid1 in the mix.
So this is not a 'full fix' for every bug in the kernel :-), but it could well be a full fix for this particular bug.

Can we just delete the too_many_isolated() logic? (Crappy comment describes what the code does but not why it does it).

Quoted 11 lines

Quoted 8 lines

I think there are two bugs here.
The raid1 bug that Torsten mentions is certainly real (and has been around for an embarrassingly long time).
The bug that I identified in too_many_isolated is also a real bug and can be triggered without md/raid1 in the mix.
So this is not a 'full fix' for every bug in the kernel :-), but it could well be a full fix for this particular bug.

Can we just delete the too_many_isolated() logic?  (Crappy comment describes what the code does but not why it does it).

if my remember is correct, we got bug report that LTP may makes misterious OOM killer invocation about 1-2 years ago. because, if too many parocess are in reclaim path, all of reclaimable pages can be isolated and last reclaimer found the system don't have any reclaimable pages and lead to invoke OOM killer

Quoted 12 lines · KOSAKI Motohiro

Quoted 11 lines

Quoted 8 lines

I think there are two bugs here.
The raid1 bug that Torsten mentions is certainly real (and has been around for an embarrassingly long time).
The bug that I identified in too_many_isolated is also a real bug and can be triggered without md/raid1 in the mix.
So this is not a 'full fix' for every bug in the kernel :-), but it could well be a full fix for this particular bug.

Can we just delete the too_many_isolated() logic?  (Crappy comment describes what the code does but not why it does it).

Quoted 8 lines

if my remember is correct, we got bug report that LTP may makes misterious OOM killer invocation about 1-2 years ago. because, if too many parocess are in reclaim path, all of reclaimable pages can be isolated and last reclaimer found the system don't have any reclaimable pages and lead to invoke OOM killer. We have strong motivation to avoid false positive oom. then, some discusstion made this patch.

if my remember is incorrect, I hope Wu or Rik fix me.

AFAIR, it's right.

How about this?

It's rather aggressive throttling than old(ie, it considers not lru type granularity but zone )
But I think it can prevent unnecessary OOM problem and solve deadlock problem.

Quoted 25 lines

Quoted 12 lines · KOSAKI Motohiro

Quoted 11 lines

Quoted 8 lines

I think there are two bugs here.
The raid1 bug that Torsten mentions is certainly real (and has been around for an embarrassingly long time).
The bug that I identified in too_many_isolated is also a real bug and can be triggered without md/raid1 in the mix.
So this is not a 'full fix' for every bug in the kernel :-), but it could well be a full fix for this particular bug.

Can we just delete the too_many_isolated() logic?  (Crappy comment describes what the code does but not why it does it).

Quoted 8 lines

if my remember is correct, we got bug report that LTP may makes misterious OOM killer invocation about 1-2 years ago. because, if too many parocess are in reclaim path, all of reclaimable pages can be isolated and last reclaimer found the system don't have any reclaimable pages and lead to invoke OOM killer. We have strong motivation to avoid false positive oom. then, some discusstion made this patch.

if my remember is incorrect, I hope Wu or Rik fix me.

AFAIR, it's right.

Quoted 5 lines

How about this?

It's rather aggressive throttling than old(ie, it considers not lru type granularity but zone )
But I think it can prevent unnecessary OOM problem and solve deadlock problem.

Can you please elaborate your intention? Do you think Wu's approach is wrong?

Quoted 31 lines · KOSAKI Motohiro

Quoted 30 lines

Quoted 12 lines · KOSAKI Motohiro

Quoted 11 lines

Quoted 8 lines

I think there are two bugs here.
The raid1 bug that Torsten mentions is certainly real (and has been around for an embarrassingly long time).
The bug that I identified in too_many_isolated is also a real bug and can be triggered without md/raid1 in the mix.
So this is not a 'full fix' for every bug in the kernel :-), but it could well be a full fix for this particular bug.

Can we just delete the too_many_isolated() logic?  (Crappy comment describes what the code does but not why it does it).

Quoted 8 lines

if my remember is correct, we got bug report that LTP may makes misterious OOM killer invocation about 1-2 years ago. because, if too many parocess are in reclaim path, all of reclaimable pages can be isolated and last reclaimer found the system don't have any reclaimable pages and lead to invoke OOM killer. We have strong motivation to avoid false positive oom. then, some discusstion made this patch.

if my remember is incorrect, I hope Wu or Rik fix me.

AFAIR, it's right.

How about this?

It's rather aggressive throttling than old(ie, it considers not lru type granularity but zone )
But I think it can prevent unnecessary OOM problem and solve deadlock problem.

Quoted 1 line

Can you please elaborate your intention? Do you think Wu's approach is wrong?

No. I think Wu's patch may work well. But I agree Andrew. Couldn't we remove the too_many_isolated logic? If it is, we can solve the problem simply.
But If we remove the logic, we will meet long time ago problem, again. So my patch's intention is to prevent OOM and deadlock problem with simple patch without adding new heuristic in too_many_isolated.

Quoted 33 lines

Quoted 31 lines · KOSAKI Motohiro

Quoted 30 lines

Quoted 12 lines · KOSAKI Motohiro

Quoted 11 lines

Quoted 8 lines

I think there are two bugs here.
The raid1 bug that Torsten mentions is certainly real (and has been around for an embarrassingly long time).
The bug that I identified in too_many_isolated is also a real bug and can be triggered without md/raid1 in the mix.
So this is not a 'full fix' for every bug in the kernel :-), but it could well be a full fix for this particular bug.

Can we just delete the too_many_isolated() logic?  (Crappy comment describes what the code does but not why it does it).

Quoted 8 lines

if my remember is correct, we got bug report that LTP may makes misterious OOM killer invocation about 1-2 years ago. because, if too many parocess are in reclaim path, all of reclaimable pages can be isolated and last reclaimer found the system don't have any reclaimable pages and lead to invoke OOM killer. We have strong motivation to avoid false positive oom. then, some discusstion made this patch.

if my remember is incorrect, I hope Wu or Rik fix me.

AFAIR, it's right.

How about this?

It's rather aggressive throttling than old(ie, it considers not lru type granularity but zone )
But I think it can prevent unnecessary OOM problem and solve deadlock problem.

Quoted 8 lines

Quoted 1 line

Can you please elaborate your intention? Do you think Wu's approach is wrong?

No. I think Wu's patch may work well. But I agree Andrew. Couldn't we remove the too_many_isolated logic? If it is, we can solve the problem simply.
But If we remove the logic, we will meet long time ago problem, again. So my patch's intention is to prevent OOM and deadlock problem with simple patch without adding new heuristic in too_many_isolated.

But your patch is much false positive/negative chance because isolated pages timing and too_many_isolated_zone() call site are in far distance place.

So, if anyone don't say Wu's one is wrong, I like his one.

Quoted 11 lines

Quoted 8 lines

Quoted 1 line

Can you please elaborate your intention? Do you think Wu's approach is wrong?

No. I think Wu's patch may work well. But I agree Andrew. Couldn't we remove the too_many_isolated logic? If it is, we can solve the problem simply.
But If we remove the logic, we will meet long time ago problem, again. So my patch's intention is to prevent OOM and deadlock problem with simple patch without adding new heuristic in too_many_isolated.

But your patch is much false positive/negative chance because isolated pages timing and too_many_isolated_zone() call site are in far distance place.

Yes.
How about the returning *did_some_progress can imply too_many_isolated fail by using MSB or new variable?
Then, page_allocator can check it whether it causes read reclaim fail or parallel reclaim.
The point is let's throttle without holding FS/IO lock.

(quoted diff) +0 -1

@@ -2054,10 +2069,11 @@ rebalance:
                goto got_pg;
 
        /*
-        * If we failed to make any progress reclaiming, then we are

Quoted 5 lines

(quoted diff) +0 -1

@@ -2054,10 +2069,11 @@ rebalance:
                 goto got_pg;
 
         /*
-        * If we failed to make any progress reclaiming, then we are

Review rebalance: · line 2079

Quoted 10 lines

(quoted diff) +4 -2

-        * running out of options and have to consider going OOM
+        * If we failed to make any progress reclaiming and there aren't
+        * many parallel reclaiming, then we are unning out of options and
+        * have to consider going OOM
          */
-       if (!did_some_progress) {
+       if (!did_some_progress && !too_many_isolated_zone(preferred_zone)) {
                 if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
                         if (oom_killer_disabled)
                                 goto nopage;

This is simply wrong.

It disabled this block for 99% system because there won't be enough tasks to make (!too_many_isolated_zone == true). As a result the LRU will be scanned like mad and no task get OOMed when it should be.

Thanks,
Fengguang

Hi Wu,

Quoted 18 lines · Wu Fengguang

Quoted 4 lines

(quoted diff) +0 -0

@@ -2054,10 +2069,11 @@ rebalance:
 A  A  A  A  A  A  A  A  goto got_pg;
 

A A A A /*

Quoted 11 lines

- A  A  A  A * If we failed to make any progress reclaiming, then we are
- A  A  A  A * running out of options and have to consider going OOM
+ A  A  A  A * If we failed to make any progress reclaiming and there aren't
+ A  A  A  A * many parallel reclaiming, then we are unning out of options and
+ A  A  A  A * have to consider going OOM
A  A  A  A  A */
- A  A  A  if (!did_some_progress) {
+ A  A  A  if (!did_some_progress && !too_many_isolated_zone(preferred_zone)) {
A  A  A  A  A  A  A  A  if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
A  A  A  A  A  A  A  A  A  A  A  A  if (oom_killer_disabled)
A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  goto nopage;

This is simply wrong.

Quoted 3 lines

It disabled this block for 99% system because there won't be enough tasks to make (!too_many_isolated_zone == true). As a result the LRU will be scanned like mad and no task get OOMed when it should be.

If !too_many_isolated_zone is false, it means there are already many direct reclaiming tasks.
So they could exit reclaim path and !too_many_isolated_zone will be true. What am I missing now?

Quoted 28 lines · Minchan Kim

Hi Wu,

Quoted 18 lines · Wu Fengguang

Quoted 5 lines

(quoted diff) +0 -1

@@ -2054,10 +2069,11 @@ rebalance:
                 goto got_pg;
 
         /*
-        * If we failed to make any progress reclaiming, then we are

Review rebalance: · line 2079

Quoted 10 lines

(quoted diff) +4 -2

-        * running out of options and have to consider going OOM
+        * If we failed to make any progress reclaiming and there aren't
+        * many parallel reclaiming, then we are unning out of options and
+        * have to consider going OOM
          */
-       if (!did_some_progress) {
+       if (!did_some_progress && !too_many_isolated_zone(preferred_zone)) {
                 if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
                         if (oom_killer_disabled)
                                 goto nopage;

This is simply wrong.

Quoted 3 lines

It disabled this block for 99% system because there won't be enough tasks to make (!too_many_isolated_zone == true). As a result the LRU will be scanned like mad and no task get OOMed when it should be.

If !too_many_isolated_zone is false, it means there are already many direct reclaiming tasks.
So they could exit reclaim path and !too_many_isolated_zone will be true

Quoted 31 lines · Minchan Kim

Quoted 21 lines

Hi Wu,

Quoted 18 lines · Wu Fengguang

Quoted 5 lines

(quoted diff) +0 -1

@@ -2054,10 +2069,11 @@ rebalance:
                 goto got_pg;
 
         /*
-        * If we failed to make any progress reclaiming, then we are

Review rebalance: · line 2079

Quoted 10 lines

(quoted diff) +4 -2

-        * running out of options and have to consider going OOM
+        * If we failed to make any progress reclaiming and there aren't
+        * many parallel reclaiming, then we are unning out of options and
+        * have to consider going OOM
          */
-       if (!did_some_progress) {
+       if (!did_some_progress && !too_many_isolated_zone(preferred_zone)) {
                 if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
                         if (oom_killer_disabled)
                                 goto nopage;

This is simply wrong.

Quoted 8 lines

Quoted 3 lines

It disabled this block for 99% system because there won't be enough tasks to make (!too_many_isolated_zone == true). As a result the LRU will be scanned like mad and no task get OOMed when it should be.

If !too_many_isolated_zone is false, it means there are already many direct reclaiming tasks.
So they could exit reclaim path and !too_many_isolated_zone will be true. What am I missing now?

Quoted 5 lines

Ah sorry, my brain get short circuited.. but I still feel uneasy with this change. It's not fixing the root cause and won't prevent too many LRU pages be isolated. It's too late to test too_many_isolated_zone() after direct reclaim returns (after sleeping for a long time).

Intend to agree.
I think root cause is a infinite looping in too_many_isolated holding FS lock. Would it be simple that too_many_isolated would be bail out after some try?

Quoted 38 lines · Wu Fengguang

Quoted 36 lines · Minchan Kim

Quoted 21 lines

Hi Wu,

Quoted 18 lines · Wu Fengguang

Quoted 5 lines

(quoted diff) +0 -1

@@ -2054,10 +2069,11 @@ rebalance:
                 goto got_pg;
 
         /*
-        * If we failed to make any progress reclaiming, then we are

Review rebalance: · line 2079

Quoted 10 lines

(quoted diff) +4 -2

-        * running out of options and have to consider going OOM
+        * If we failed to make any progress reclaiming and there aren't
+        * many parallel reclaiming, then we are unning out of options and
+        * have to consider going OOM
          */
-       if (!did_some_progress) {
+       if (!did_some_progress && !too_many_isolated_zone(preferred_zone)) {
                 if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
                         if (oom_killer_disabled)
                                 goto nopage;

This is simply wrong.

Quoted 8 lines

Quoted 3 lines

It disabled this block for 99% system because there won't be enough tasks to make (!too_many_isolated_zone == true). As a result the LRU will be scanned like mad and no task get OOMed when it should be.

If !too_many_isolated_zone is false, it means there are already many direct reclaiming tasks.
So they could exit reclaim path and !too_many_isolated_zone will be true. What am I missing now?

Ah sorry, my brain get short circuited.. but I still feel uneasy with this change. It's not fixing the root cause and won't prevent too many LRU pages be isolated. It's too late to test too_many_isolated_zone() after direct reclaim returns (after sleeping for a long time).

Quoted 3 lines

Intend to agree.
I think root cause is a infinite looping in too_many_isolated holding FS lock. Would it be simple that too_many_isolated would be bail out after some try?

How?
A lot of caller don't have good recover logic when memory allocation fail occur.

Quoted 31 lines · Wu Fengguang

Quoted 21 lines · Minchan Kim

Hi Wu,

Quoted 18 lines · Wu Fengguang

Quoted 5 lines

(quoted diff) +0 -1

@@ -2054,10 +2069,11 @@ rebalance:
                goto got_pg;
 
        /*
-        * If we failed to make any progress reclaiming, then we are

Review rebalance: · line 2079

Quoted 10 lines

(quoted diff) +4 -2

-        * running out of options and have to consider going OOM
+        * If we failed to make any progress reclaiming and there aren't
+        * many parallel reclaiming, then we are unning out of options and
+        * have to consider going OOM
         */
-       if (!did_some_progress) {
+       if (!did_some_progress && !too_many_isolated_zone(preferred_zone)) {
                if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
                        if (oom_killer_disabled)
                                goto nopage;

This is simply wrong.

Quoted 8 lines

Quoted 3 lines

It disabled this block for 99% system because there won't be enough tasks to make (!too_many_isolated_zone == true). As a result the LRU will be scanned like mad and no task get OOMed when it should be.

If !too_many_isolated_zone is false, it means there are already many direct reclaiming tasks.
So they could exit reclaim path and !too_many_isolated_zone will be true. What am I missing now?

Quoted 5 lines

Ah sorry, my brain get short circuited.. but I still feel uneasy with this change. It's not fixing the root cause and won't prevent too many LRU pages be isolated. It's too late to test too_many_isolated_zone() after direct reclaim returns (after sleeping for a long time).

Intend to agree.
I think root cause is a infinite looping in too_many_isolated holding FS lock. Would it be simple that too_many_isolated would be bail out after some try?

Quoted 38 lines · Minchan Kim

Quoted 36 lines · Wu Fengguang

Quoted 21 lines · Minchan Kim

Hi Wu,

Quoted 18 lines · Wu Fengguang

Quoted 5 lines

(quoted diff) +0 -1

@@ -2054,10 +2069,11 @@ rebalance:
                 goto got_pg;
 
         /*
-        * If we failed to make any progress reclaiming, then we are

Review rebalance: · line 2079

Quoted 10 lines

(quoted diff) +4 -2

-        * running out of options and have to consider going OOM
+        * If we failed to make any progress reclaiming and there aren't
+        * many parallel reclaiming, then we are unning out of options and
+        * have to consider going OOM
          */
-       if (!did_some_progress) {
+       if (!did_some_progress && !too_many_isolated_zone(preferred_zone)) {
                 if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
                         if (oom_killer_disabled)
                                 goto nopage;

This is simply wrong.

Quoted 8 lines

Quoted 3 lines

It disabled this block for 99% system because there won't be enough tasks to make (!too_many_isolated_zone == true). As a result the LRU will be scanned like mad and no task get OOMed when it should be.

If !too_many_isolated_zone is false, it means there are already many direct reclaiming tasks.
So they could exit reclaim path and !too_many_isolated_zone will be true. What am I missing now?

Ah sorry, my brain get short circuited.. but I still feel uneasy with this change. It's not fixing the root cause and won't prevent too many LRU pages be isolated. It's too late to test too_many_isolated_zone() after direct reclaim returns (after sleeping for a long time).

Quoted 3 lines

Intend to agree.
I think root cause is a infinite looping in too_many_isolated holding FS lock. Would it be simple that too_many_isolated would be bail out after some try?

I'm wondering if we need too_many_isolated_zone logic. The do_try_to_free_pages will return progress till all zones are unreclaimable. Assume before this we don't oomkiller. If the direct reclaim fails but has progress, it will sleep.

Quoted 42 lines · Shaohua Li

Quoted 41 lines · Minchan Kim

Quoted 31 lines · Wu Fengguang

Quoted 21 lines · Minchan Kim

Hi Wu,

Quoted 18 lines · Wu Fengguang

Quoted 5 lines

(quoted diff) +0 -1

@@ -2054,10 +2069,11 @@ rebalance:
                 goto got_pg;
 
         /*
-        * If we failed to make any progress reclaiming, then we are

Review rebalance: · line 2079

Quoted 10 lines

(quoted diff) +4 -2

-        * running out of options and have to consider going OOM
+        * If we failed to make any progress reclaiming and there aren't
+        * many parallel reclaiming, then we are unning out of options and
+        * have to consider going OOM
          */
-       if (!did_some_progress) {
+       if (!did_some_progress && !too_many_isolated_zone(preferred_zone)) {
                 if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
                         if (oom_killer_disabled)
                                 goto nopage;

This is simply wrong.

Quoted 8 lines

Quoted 3 lines

It disabled this block for 99% system because there won't be enough tasks to make (!too_many_isolated_zone == true). As a result the LRU will be scanned like mad and no task get OOMed when it should be.

If !too_many_isolated_zone is false, it means there are already many direct reclaiming tasks.
So they could exit reclaim path and !too_many_isolated_zone will be true. What am I missing now?

Quoted 5 lines

Ah sorry, my brain get short circuited.. but I still feel uneasy with this change. It's not fixing the root cause and won't prevent too many LRU pages be isolated. It's too late to test too_many_isolated_zone() after direct reclaim returns (after sleeping for a long time).

Intend to agree.
I think root cause is a infinite looping in too_many_isolated holding FS lock. Would it be simple that too_many_isolated would be bail out after some try?

Quoted 3 lines

I'm wondering if we need too_many_isolated_zone logic. The do_try_to_free_pages will return progress till all zones are unreclaimable. Assume before this we don't oomkiller. If the direct reclaim fails but has progress, it will sleep.

Not sure if this is clear. What I mean is we can delete too_many_isolated_zone, do_try_to_free_pages can still return 1 till all zones are unreclaimable. Before this direct reclaim will not oom, because it sees progress and will call congestion_wait to sleep. Am I missing anything?

Quoted 13 lines · On Tue, 19 Oct 2010 09:31:42 +1100 Neil Brown <[email protected]> wrote:

Quoted 10 lines · On Mon, 18 Oct 2010 14:58:59 -0700 Andrew Morton <[email protected]>…

Quoted 7 lines · On Tue, 19 Oct 2010 00:15:04 +0800 Wu Fengguang <[email protected]> wrote:

Neil find that if too_many_isolated() returns true while performing direct reclaim we can end up waiting for other threads to complete their direct reclaim. If those threads are allowed to enter the FS or IO to free memory, but this thread is not, then it is possible that those threads will be waiting on this thread and so we get a circular deadlock.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

some task enters direct reclaim with GFP_KERNEL
=> too_many_isolated() false
=> vmscan and run into dirty pages
=> pageout()
=> take some FS lock
=> fs/block code does GFP_NOIO allocation
=> enter direct reclaim again
=> too_many_isolated() true
=> waiting for others to progress, however the other
tasks may be circular waiting for the FS lock..

I'm assuming that the last four "=>"'s here should have been indented another stop.

Quoted 17 lines

Quoted 10 lines

Quoted 9 lines

The fix is to let !__GFP_IO and !__GFP_FS direct reclaims enjoy higher priority than normal ones, by honouring them higher throttle threshold.

Now !GFP_IOFS reclaims won't be waiting for GFP_IOFS reclaims to progress. They will be blocked only when there are too many concurrent !GFP_IOFS reclaims, however that's very unlikely because the IO-less direct reclaims is able to progress much more faster, and they won't deadlock each other. The threshold is raised high enough for them, so that there can be sufficient parallel progress of !GFP_IOFS reclaims.

Quoted 4 lines

I'm not sure that this is really a full fix. Torsten's analysis does appear to point at the real bug: raid1 has code paths which allocate more than a single element from a mempool without starting IO against previous elements.

... point at "a" real bug.

Quoted 8 lines

I think there are two bugs here.
The raid1 bug that Torsten mentions is certainly real (and has been around for an embarrassingly long time).
The bug that I identified in too_many_isolated is also a real bug and can be triggered without md/raid1 in the mix.
So this is not a 'full fix' for every bug in the kernel :-),

but it could well be a full fix for this particular bug.

Can we just delete the too_many_isolated() logic? (Crappy comment

describes what the code does but not why it does it).