| Message ID | 1382971703-17393-30-git-send-email-luis.henriques@canonical.com |
|---|---|
| State | New |
| Headers | show |
This is part of a bigger series and was tagged for stable as a reminder only. Please don't apply for now. On Mon, Oct 28, 2013 at 02:47:48PM +0000, Luis Henriques wrote: > 3.5.7.24 -stable review patch. If anyone has any objections, please let me know. > > ------------------ > > From: Johannes Weiner <hannes@cmpxchg.org> > > commit 84235de394d9775bfaa7fa9762a59d91fef0c1fc upstream. > > Buffer allocation has a very crude indefinite loop around waking the > flusher threads and performing global NOFS direct reclaim because it can > not handle allocation failures. > > The most immediate problem with this is that the allocation may fail due > to a memory cgroup limit, where flushers + direct reclaim might not make > any progress towards resolving the situation at all. Because unlike the > global case, a memory cgroup may not have any cache at all, only > anonymous pages but no swap. This situation will lead to a reclaim > livelock with insane IO from waking the flushers and thrashing unrelated > filesystem cache in a tight loop. > > Use __GFP_NOFAIL allocations for buffers for now. This makes sure that > any looping happens in the page allocator, which knows how to > orchestrate kswapd, direct reclaim, and the flushers sensibly. It also > allows memory cgroups to detect allocations that can't handle failure > and will allow them to ultimately bypass the limit if reclaim can not > make progress. > > Reported-by: azurIt <azurit@pobox.sk> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> > Cc: Michal Hocko <mhocko@suse.cz> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > Signed-off-by: Luis Henriques <luis.henriques@canonical.com> > --- > fs/buffer.c | 14 ++++++++++++-- > mm/memcontrol.c | 2 ++ > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/fs/buffer.c b/fs/buffer.c > index 2c78739..2675e5a 100644 > --- a/fs/buffer.c > +++ b/fs/buffer.c > @@ -957,9 +957,19 @@ grow_dev_page(struct block_device *bdev, sector_t block, > struct buffer_head *bh; > sector_t end_block; > int ret = 0; /* Will call free_more_memory() */ > + gfp_t gfp_mask; > > - page = find_or_create_page(inode->i_mapping, index, > - (mapping_gfp_mask(inode->i_mapping) & ~__GFP_FS)|__GFP_MOVABLE); > + gfp_mask = mapping_gfp_mask(inode->i_mapping) & ~__GFP_FS; > + gfp_mask |= __GFP_MOVABLE; > + /* > + * XXX: __getblk_slow() can not really deal with failure and > + * will endlessly loop on improvised global reclaim. Prefer > + * looping in the allocator rather than here, at least that > + * code knows what it's doing. > + */ > + gfp_mask |= __GFP_NOFAIL; > + > + page = find_or_create_page(inode->i_mapping, index, gfp_mask); > if (!page) > return ret; > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 226b63e..953bf3c 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -2405,6 +2405,8 @@ done: > return 0; > nomem: > *ptr = NULL; > + if (gfp_mask & __GFP_NOFAIL) > + return 0; > return -ENOMEM; > bypass: > *ptr = root_mem_cgroup; > -- > 1.8.3.2 >
On Thu, Oct 31, 2013 at 10:00:08AM -0400, Johannes Weiner wrote: > This is part of a bigger series and was tagged for stable as a > reminder only. Please don't apply for now. Grrr... I need to start cleaning my email inbox before doing a release. I just saw the discussion in stable@. I'll do an emergency release reverting this patch. Thanks for catching this. Cheers, -- Luis > > On Mon, Oct 28, 2013 at 02:47:48PM +0000, Luis Henriques wrote: > > 3.5.7.24 -stable review patch. If anyone has any objections, please let me know. > > > > ------------------ > > > > From: Johannes Weiner <hannes@cmpxchg.org> > > > > commit 84235de394d9775bfaa7fa9762a59d91fef0c1fc upstream. > > > > Buffer allocation has a very crude indefinite loop around waking the > > flusher threads and performing global NOFS direct reclaim because it can > > not handle allocation failures. > > > > The most immediate problem with this is that the allocation may fail due > > to a memory cgroup limit, where flushers + direct reclaim might not make > > any progress towards resolving the situation at all. Because unlike the > > global case, a memory cgroup may not have any cache at all, only > > anonymous pages but no swap. This situation will lead to a reclaim > > livelock with insane IO from waking the flushers and thrashing unrelated > > filesystem cache in a tight loop. > > > > Use __GFP_NOFAIL allocations for buffers for now. This makes sure that > > any looping happens in the page allocator, which knows how to > > orchestrate kswapd, direct reclaim, and the flushers sensibly. It also > > allows memory cgroups to detect allocations that can't handle failure > > and will allow them to ultimately bypass the limit if reclaim can not > > make progress. > > > > Reported-by: azurIt <azurit@pobox.sk> > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> > > Cc: Michal Hocko <mhocko@suse.cz> > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > > Signed-off-by: Luis Henriques <luis.henriques@canonical.com> > > --- > > fs/buffer.c | 14 ++++++++++++-- > > mm/memcontrol.c | 2 ++ > > 2 files changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/fs/buffer.c b/fs/buffer.c > > index 2c78739..2675e5a 100644 > > --- a/fs/buffer.c > > +++ b/fs/buffer.c > > @@ -957,9 +957,19 @@ grow_dev_page(struct block_device *bdev, sector_t block, > > struct buffer_head *bh; > > sector_t end_block; > > int ret = 0; /* Will call free_more_memory() */ > > + gfp_t gfp_mask; > > > > - page = find_or_create_page(inode->i_mapping, index, > > - (mapping_gfp_mask(inode->i_mapping) & ~__GFP_FS)|__GFP_MOVABLE); > > + gfp_mask = mapping_gfp_mask(inode->i_mapping) & ~__GFP_FS; > > + gfp_mask |= __GFP_MOVABLE; > > + /* > > + * XXX: __getblk_slow() can not really deal with failure and > > + * will endlessly loop on improvised global reclaim. Prefer > > + * looping in the allocator rather than here, at least that > > + * code knows what it's doing. > > + */ > > + gfp_mask |= __GFP_NOFAIL; > > + > > + page = find_or_create_page(inode->i_mapping, index, gfp_mask); > > if (!page) > > return ret; > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index 226b63e..953bf3c 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -2405,6 +2405,8 @@ done: > > return 0; > > nomem: > > *ptr = NULL; > > + if (gfp_mask & __GFP_NOFAIL) > > + return 0; > > return -ENOMEM; > > bypass: > > *ptr = root_mem_cgroup; > > -- > > 1.8.3.2 > > > -- > To unsubscribe from this list: send the line "unsubscribe stable" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 31-10-13 10:00:08, Johannes Weiner wrote: > On Mon, Oct 28, 2013 at 02:47:48PM +0000, Luis Henriques wrote: > > 3.5.7.24 -stable review patch. If anyone has any objections, please let me know. > > > > ------------------ > > > > From: Johannes Weiner <hannes@cmpxchg.org> > > > > commit 84235de394d9775bfaa7fa9762a59d91fef0c1fc upstream. > > > > Buffer allocation has a very crude indefinite loop around waking the > > flusher threads and performing global NOFS direct reclaim because it can > > not handle allocation failures. > > > > The most immediate problem with this is that the allocation may fail due > > to a memory cgroup limit, where flushers + direct reclaim might not make > > any progress towards resolving the situation at all. Because unlike the > > global case, a memory cgroup may not have any cache at all, only > > anonymous pages but no swap. This situation will lead to a reclaim > > livelock with insane IO from waking the flushers and thrashing unrelated > > filesystem cache in a tight loop. > > > > Use __GFP_NOFAIL allocations for buffers for now. This makes sure that > > any looping happens in the page allocator, which knows how to > > orchestrate kswapd, direct reclaim, and the flushers sensibly. It also > > allows memory cgroups to detect allocations that can't handle failure > > and will allow them to ultimately bypass the limit if reclaim can not > > make progress. So I was under the impression that __GFP_NOFAIL is going away, doesn't it? At least about an year ago there was some effort to remove its users so we ended up creating loops like the above one (and similar ones for jbd/jbd2) in cases where handling the failure wasn't easily possible. And now it seems we are going in the opposite direction... At least we have a steady flow of patches guaranteed :) Honza > > > > Reported-by: azurIt <azurit@pobox.sk> > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> > > Cc: Michal Hocko <mhocko@suse.cz> > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > > Signed-off-by: Luis Henriques <luis.henriques@canonical.com> > > --- > > fs/buffer.c | 14 ++++++++++++-- > > mm/memcontrol.c | 2 ++ > > 2 files changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/fs/buffer.c b/fs/buffer.c > > index 2c78739..2675e5a 100644 > > --- a/fs/buffer.c > > +++ b/fs/buffer.c > > @@ -957,9 +957,19 @@ grow_dev_page(struct block_device *bdev, sector_t block, > > struct buffer_head *bh; > > sector_t end_block; > > int ret = 0; /* Will call free_more_memory() */ > > + gfp_t gfp_mask; > > > > - page = find_or_create_page(inode->i_mapping, index, > > - (mapping_gfp_mask(inode->i_mapping) & ~__GFP_FS)|__GFP_MOVABLE); > > + gfp_mask = mapping_gfp_mask(inode->i_mapping) & ~__GFP_FS; > > + gfp_mask |= __GFP_MOVABLE; > > + /* > > + * XXX: __getblk_slow() can not really deal with failure and > > + * will endlessly loop on improvised global reclaim. Prefer > > + * looping in the allocator rather than here, at least that > > + * code knows what it's doing. > > + */ > > + gfp_mask |= __GFP_NOFAIL; > > + > > + page = find_or_create_page(inode->i_mapping, index, gfp_mask); > > if (!page) > > return ret; > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index 226b63e..953bf3c 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -2405,6 +2405,8 @@ done: > > return 0; > > nomem: > > *ptr = NULL; > > + if (gfp_mask & __GFP_NOFAIL) > > + return 0; > > return -ENOMEM; > > bypass: > > *ptr = root_mem_cgroup; > > -- > > 1.8.3.2 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
On Thu, Oct 31, 2013 at 03:48:48PM +0100, Jan Kara wrote: > On Thu 31-10-13 10:00:08, Johannes Weiner wrote: > > On Mon, Oct 28, 2013 at 02:47:48PM +0000, Luis Henriques wrote: > > > 3.5.7.24 -stable review patch. If anyone has any objections, please let me know. > > > > > > ------------------ > > > > > > From: Johannes Weiner <hannes@cmpxchg.org> > > > > > > commit 84235de394d9775bfaa7fa9762a59d91fef0c1fc upstream. > > > > > > Buffer allocation has a very crude indefinite loop around waking the > > > flusher threads and performing global NOFS direct reclaim because it can > > > not handle allocation failures. > > > > > > The most immediate problem with this is that the allocation may fail due > > > to a memory cgroup limit, where flushers + direct reclaim might not make > > > any progress towards resolving the situation at all. Because unlike the > > > global case, a memory cgroup may not have any cache at all, only > > > anonymous pages but no swap. This situation will lead to a reclaim > > > livelock with insane IO from waking the flushers and thrashing unrelated > > > filesystem cache in a tight loop. > > > > > > Use __GFP_NOFAIL allocations for buffers for now. This makes sure that > > > any looping happens in the page allocator, which knows how to > > > orchestrate kswapd, direct reclaim, and the flushers sensibly. It also > > > allows memory cgroups to detect allocations that can't handle failure > > > and will allow them to ultimately bypass the limit if reclaim can not > > > make progress. > So I was under the impression that __GFP_NOFAIL is going away, doesn't > it? At least about an year ago there was some effort to remove its users so > we ended up creating loops like the above one (and similar ones for > jbd/jbd2) in cases where handling the failure wasn't easily possible. And now > it seems we are going in the opposite direction... At least we have a > steady flow of patches guaranteed :) Lol. I would assume that people had a problem with allocations that can not fail, rather than __GFP_NOFAIL. As long as we do have callsites that can't deal with failure, I'd much prefer __GFP_NOFAIL over open-coded looping. The page allocator is much better equipped to make forward progress and the problematic sites are immediately apparent/greppable. In order of preference, this is how allocation sites should deal with errors: 1. Gracefully abort current operation and move on 2. Stab eyes with fork 3. Use __GFP_NOFAIL ... but never loop around the allocation, please.
On Thu, 31 Oct 2013 15:48:48 +0100 Jan Kara <jack@suse.cz> wrote: > On Thu 31-10-13 10:00:08, Johannes Weiner wrote: > > On Mon, Oct 28, 2013 at 02:47:48PM +0000, Luis Henriques wrote: > > > 3.5.7.24 -stable review patch. If anyone has any objections, please let me know. > > > > > > ------------------ > > > > > > From: Johannes Weiner <hannes@cmpxchg.org> > > > > > > commit 84235de394d9775bfaa7fa9762a59d91fef0c1fc upstream. > > > > > > Buffer allocation has a very crude indefinite loop around waking the > > > flusher threads and performing global NOFS direct reclaim because it can > > > not handle allocation failures. > > > > > > The most immediate problem with this is that the allocation may fail due > > > to a memory cgroup limit, where flushers + direct reclaim might not make > > > any progress towards resolving the situation at all. Because unlike the > > > global case, a memory cgroup may not have any cache at all, only > > > anonymous pages but no swap. This situation will lead to a reclaim > > > livelock with insane IO from waking the flushers and thrashing unrelated > > > filesystem cache in a tight loop. > > > > > > Use __GFP_NOFAIL allocations for buffers for now. This makes sure that > > > any looping happens in the page allocator, which knows how to > > > orchestrate kswapd, direct reclaim, and the flushers sensibly. It also > > > allows memory cgroups to detect allocations that can't handle failure > > > and will allow them to ultimately bypass the limit if reclaim can not > > > make progress. > So I was under the impression that __GFP_NOFAIL is going away, doesn't > it? At least about an year ago there was some effort to remove its users so > we ended up creating loops like the above one (and similar ones for > jbd/jbd2) in cases where handling the failure wasn't easily possible. And now > it seems we are going in the opposite direction... At least we have a > steady flow of patches guaranteed :) Argh. The whole point behind __GFP_NOFAIL was to centralise the open-coded infinite-retry loops into the MM core. So they can be easily located and fixed up. Yes, __GFP_NOFAIL *should* go away, once all those infinite-retry loops are fixed to handle allocation failures. But it sounds like this "effort" was just undoing : commit f3615244f15c8bee5783fcf032717ffdfd56e219 : Author: akpm <akpm> : AuthorDate: Sun Apr 20 21:28:12 2003 +0000 : Commit: akpm <akpm> : CommitDate: Sun Apr 20 21:28:12 2003 +0000 : : [PATCH] implement __GFP_REPEAT, __GFP_NOFAIL, __GFP_NORETRY and thereby hiding the bad code from grep again :(
On Thu 31-10-13 09:03:53, Andrew Morton wrote: > On Thu, 31 Oct 2013 15:48:48 +0100 Jan Kara <jack@suse.cz> wrote: > > > On Thu 31-10-13 10:00:08, Johannes Weiner wrote: > > > On Mon, Oct 28, 2013 at 02:47:48PM +0000, Luis Henriques wrote: > > > > 3.5.7.24 -stable review patch. If anyone has any objections, please let me know. > > > > > > > > ------------------ > > > > > > > > From: Johannes Weiner <hannes@cmpxchg.org> > > > > > > > > commit 84235de394d9775bfaa7fa9762a59d91fef0c1fc upstream. > > > > > > > > Buffer allocation has a very crude indefinite loop around waking the > > > > flusher threads and performing global NOFS direct reclaim because it can > > > > not handle allocation failures. > > > > > > > > The most immediate problem with this is that the allocation may fail due > > > > to a memory cgroup limit, where flushers + direct reclaim might not make > > > > any progress towards resolving the situation at all. Because unlike the > > > > global case, a memory cgroup may not have any cache at all, only > > > > anonymous pages but no swap. This situation will lead to a reclaim > > > > livelock with insane IO from waking the flushers and thrashing unrelated > > > > filesystem cache in a tight loop. > > > > > > > > Use __GFP_NOFAIL allocations for buffers for now. This makes sure that > > > > any looping happens in the page allocator, which knows how to > > > > orchestrate kswapd, direct reclaim, and the flushers sensibly. It also > > > > allows memory cgroups to detect allocations that can't handle failure > > > > and will allow them to ultimately bypass the limit if reclaim can not > > > > make progress. > > So I was under the impression that __GFP_NOFAIL is going away, doesn't > > it? At least about an year ago there was some effort to remove its users so > > we ended up creating loops like the above one (and similar ones for > > jbd/jbd2) in cases where handling the failure wasn't easily possible. And now > > it seems we are going in the opposite direction... At least we have a > > steady flow of patches guaranteed :) > > Argh. The whole point behind __GFP_NOFAIL was to centralise the > open-coded infinite-retry loops into the MM core. So they can be > easily located and fixed up. > > Yes, __GFP_NOFAIL *should* go away, once all those infinite-retry loops > are fixed to handle allocation failures. But it sounds like this > "effort" was just undoing > > : commit f3615244f15c8bee5783fcf032717ffdfd56e219 > : Author: akpm <akpm> > : AuthorDate: Sun Apr 20 21:28:12 2003 +0000 > : Commit: akpm <akpm> > : CommitDate: Sun Apr 20 21:28:12 2003 +0000 > : > : [PATCH] implement __GFP_REPEAT, __GFP_NOFAIL, __GFP_NORETRY > > and thereby hiding the bad code from grep again :( So I also looked into history trying to find out why we opencoded the allocation loops. It seems originally the patch set described and referenced in http://lwn.net/Articles/401915/ from David Rientjes in 2010 triggered the discussion. You actually opposed to that series so I didn't merge the jbd patch. But jbd2 change got merged by Ted. Then an year later I've noticed jbd2 is avoiding __GFP_NOFAIL and forgot you were opposing that change and copied the change over to jbd. So I'll back out the jbd change. I'll also look into removing the retry loop from jbd2 (there the change actually made some sense because in some cases we can deal with allocation failure). Honza
diff --git a/fs/buffer.c b/fs/buffer.c index 2c78739..2675e5a 100644 --- a/fs/buffer.c +++ b/fs/buffer.c @@ -957,9 +957,19 @@ grow_dev_page(struct block_device *bdev, sector_t block, struct buffer_head *bh; sector_t end_block; int ret = 0; /* Will call free_more_memory() */ + gfp_t gfp_mask; - page = find_or_create_page(inode->i_mapping, index, - (mapping_gfp_mask(inode->i_mapping) & ~__GFP_FS)|__GFP_MOVABLE); + gfp_mask = mapping_gfp_mask(inode->i_mapping) & ~__GFP_FS; + gfp_mask |= __GFP_MOVABLE; + /* + * XXX: __getblk_slow() can not really deal with failure and + * will endlessly loop on improvised global reclaim. Prefer + * looping in the allocator rather than here, at least that + * code knows what it's doing. + */ + gfp_mask |= __GFP_NOFAIL; + + page = find_or_create_page(inode->i_mapping, index, gfp_mask); if (!page) return ret; diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 226b63e..953bf3c 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2405,6 +2405,8 @@ done: return 0; nomem: *ptr = NULL; + if (gfp_mask & __GFP_NOFAIL) + return 0; return -ENOMEM; bypass: *ptr = root_mem_cgroup;