Re: [PATCH v2 -mm] memcg: prevent from OOM with too many dirty pages

[Hugh Dickins <hughd@google.com>]

On Thu, 12 Jul 2012, Michal Hocko wrote:
> On Wed 11-07-12 18:57:43, Hugh Dickins wrote:
> > 
> > I mentioned in Johannes's [03/11] thread a couple of days ago, that
> > I was having a problem with your wait_on_page_writeback() in mmotm.
> > 
> > It turns out that your original patch was fine, but you let dark angels
> > whisper into your ear, to persuade you to remove the "&& may_enter_fs".
> > 
> > Part of my load builds kernels on extN over loop over tmpfs: loop does
> > mapping_set_gfp_mask(mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS))
> > because it knows it will deadlock, if the loop thread enters reclaim,
> > and reclaim tries to write back a dirty page, one which needs the loop
> > thread to perform the write.
> 
> Good catch! I have totally missed the loop driver.
> 
> > With the may_enter_fs check restored, all is well.

Not as well as I thought when I wrote that: but those issues I'll deal
with in separate mail (and my alternative patch was no better).

> > I don't entirely
> > like your patch: I think it would be much better to wait in the same
> > place as the wait_iff_congested(), when the pages gathered have been
> > sent for writing and unlocked and putback and freed; 
> 
> I guess you mean
> 	if (nr_writeback && nr_writeback >=
>                         (nr_taken >> (DEF_PRIORITY - sc->priority)))
>                 wait_iff_congested(zone, BLK_RW_ASYNC, HZ/10);

Yes, I've appended the patch I was meaning below; but although it's
the way I had approached the issue, I don't in practice see any better
behaviour from mine than from yours.  So unless a good reason appears
later, to do it my way instead of yours, let's just forget about mine.

> 
> I have tried to hook here but it has some issues. First of all we do not
> know how long we should wait. Waiting for specific pages sounded more
> event based and more precise.
> 
> We can surely do better but I wanted to stop the OOM first without any
> other possible side effects on the global reclaim. I have tried to make
> the band aid as simple as possible. Memcg dirty pages accounting is
> forming already so we are one (tiny) step closer to the throttling.
>  
> > and I also wonder if it should go beyond the !global_reclaim case for
> > swap pages, because they don't participate in dirty limiting.
> 
> Worth a separate patch?

If I could ever generate a suitable testcase, yes.  But in practice,
the only way I've managed to generate such a preponderance of swapping
over file reclaim, is by using memcgs, which your patch already catches.
And if there actually is the swapping issue I suggest, then it's been
around for a very long time, apparently without complaint.

Here is the patch I had in mind: I'm posting it as illustration, so we
can look back to it in the archives if necessary; but it's definitely
not signed-off, I've seen no practical advantage over yours, probably
we just forget about this one below now.

But more mail to follow, returning to yours...

Hugh

p.s. KAMEZAWA-san, if you wonder why you're suddenly brought into this
conversation, it's because there was a typo in your email address before.

--- 3.5-rc6/vmscan.c	2012-06-03 06:42:11.000000000 -0700
+++ linux/vmscan.c	2012-07-13 11:53:20.372087273 -0700
@@ -675,7 +675,8 @@ static unsigned long shrink_page_list(st
 				      struct zone *zone,
 				      struct scan_control *sc,
 				      unsigned long *ret_nr_dirty,
-				      unsigned long *ret_nr_writeback)
+				      unsigned long *ret_nr_writeback,
+				      struct page **slow_page)
 {
 	LIST_HEAD(ret_pages);
 	LIST_HEAD(free_pages);
@@ -720,6 +721,27 @@ static unsigned long shrink_page_list(st
 			(PageSwapCache(page) && (sc->gfp_mask & __GFP_IO));
 
 		if (PageWriteback(page)) {
+			/*
+			 * memcg doesn't have any dirty pages throttling so we
+			 * could easily OOM just because too many pages are in
+			 * writeback from reclaim and there is nothing else to
+			 * reclaim.  Nor is swap subject to dirty throttling.
+			 *
+			 * Check may_enter_fs, certainly because a loop driver
+			 * thread might enter reclaim, and deadlock if it waits
+			 * on a page for which it is needed to do the write
+			 * (loop masks off __GFP_IO|__GFP_FS for this reason);
+			 * but more thought would probably show more reasons.
+			 *
+			 * Just use one page per shrink for this: wait on its
+			 * writeback once we have done the rest.  If device is
+			 * slow, in due course we shall choose one of its pages.
+			 */
+			if (!*slow_page && may_enter_fs && PageReclaim(page) &&
+			    (PageSwapCache(page) || !global_reclaim(sc))) {
+				*slow_page = page;
+				get_page(page);
+			}
 			nr_writeback++;
 			unlock_page(page);
 			goto keep;
@@ -1208,6 +1230,7 @@ shrink_inactive_list(unsigned long nr_to
 	int file = is_file_lru(lru);
 	struct zone *zone = lruvec_zone(lruvec);
 	struct zone_reclaim_stat *reclaim_stat = &lruvec->reclaim_stat;
+	struct page *slow_page = NULL;
 
 	while (unlikely(too_many_isolated(zone, file, sc))) {
 		congestion_wait(BLK_RW_ASYNC, HZ/10);
@@ -1245,7 +1268,7 @@ shrink_inactive_list(unsigned long nr_to
 		return 0;
 
 	nr_reclaimed = shrink_page_list(&page_list, zone, sc,
-						&nr_dirty, &nr_writeback);
+					&nr_dirty, &nr_writeback, &slow_page);
 
 	spin_lock_irq(&zone->lru_lock);
 
@@ -1292,8 +1315,13 @@ shrink_inactive_list(unsigned long nr_to
 	 *                     isolated page is PageWriteback
 	 */
 	if (nr_writeback && nr_writeback >=
-			(nr_taken >> (DEF_PRIORITY - sc->priority)))
+			(nr_taken >> (DEF_PRIORITY - sc->priority))) {
 		wait_iff_congested(zone, BLK_RW_ASYNC, HZ/10);
+		if (slow_page && PageReclaim(slow_page))
+			wait_on_page_writeback(slow_page);
+	}
+	if (slow_page)
+		put_page(slow_page);
 
 	trace_mm_vmscan_lru_shrink_inactive(zone->zone_pgdat->node_id,
 		zone_idx(zone),

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