[RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[Fabian Frederick]

Loop around congestion_wait on allocation failure/alloc_journal_list
like already fixed in other FS.

(Does it need returning -ENOMEM after some retries ?)

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 fs/reiserfs/journal.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
index fd77703..e8c56a9 100644
--- a/fs/reiserfs/journal.c
+++ b/fs/reiserfs/journal.c
@@ -2487,8 +2487,13 @@ static int journal_read(struct super_block *sb)
 static struct reiserfs_journal_list *alloc_journal_list(struct super_block *s)
 {
 	struct reiserfs_journal_list *jl;
-	jl = kzalloc(sizeof(struct reiserfs_journal_list),
-		     GFP_NOFS | __GFP_NOFAIL);
+
+	do {
+		jl = kzalloc(sizeof(struct reiserfs_journal_list), GFP_NOFS);
+		if (unlikely(!jl))
+			congestion_wait(BLK_RW_ASYNC, HZ/50);
+	} while (!jl)
+
 	INIT_LIST_HEAD(&jl->j_list);
 	INIT_LIST_HEAD(&jl->j_working_list);
 	INIT_LIST_HEAD(&jl->j_tail_bh_list);
-- 
1.8.4.5

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[Andrew Morton]

On Fri, 21 Mar 2014 17:18:30 +0100 Fabian Frederick <fabf@skynet.be> wrote:

> Loop around congestion_wait on allocation failure/alloc_journal_list
> like already fixed in other FS.
> 
> ...
>
> --- a/fs/reiserfs/journal.c
> +++ b/fs/reiserfs/journal.c
> @@ -2487,8 +2487,13 @@ static int journal_read(struct super_block *sb)
>  static struct reiserfs_journal_list *alloc_journal_list(struct super_block *s)
>  {
>  	struct reiserfs_journal_list *jl;
> -	jl = kzalloc(sizeof(struct reiserfs_journal_list),
> -		     GFP_NOFS | __GFP_NOFAIL);
> +
> +	do {
> +		jl = kzalloc(sizeof(struct reiserfs_journal_list), GFP_NOFS);
> +		if (unlikely(!jl))
> +			congestion_wait(BLK_RW_ASYNC, HZ/50);
> +	} while (!jl)
> +

Dammit, who has been running around converting __GFP_NOFAIL into
open-coded congestion_wait() loops?

The whole point of __GFP_NOFAIL is to centralise this
wait-for-memory-for-ever operation.  So it is implemented in a common
(core) place and so that we can easily locate these problematic
callers.

This comment in ext4:

			/*
			 * If __GFP_FS is not present, then we may be
			 * being called from inside the fs writeback
			 * layer, so we MUST NOT fail.  Since
			 * __GFP_NOFAIL is going away, we will arrange
			 * to retry the allocation ourselves.
			 */

is exactly wrong.  Yes, we'd like __GFP_NOFAIL to go away, but it
cannot go away until buggy callsites such as this one are *fixed*. 
Removing the __GFP_NOFAIL usage simply hides the buggy code from casual
searchers.

argh.

What we should do is to fix all these call sites so they can handle
memory exhaustion.  That's hard so in the interim they should be using
__GFP_NOFAIL.

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[Fabian Frederick]

On Fri, 21 Mar 2014 13:00:55 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Fri, 21 Mar 2014 17:18:30 +0100 Fabian Frederick <fabf@skynet.be> wrote:
> 
> > Loop around congestion_wait on allocation failure/alloc_journal_list
> > like already fixed in other FS.
> > 
> > ...
> >
> > --- a/fs/reiserfs/journal.c
> > +++ b/fs/reiserfs/journal.c
> > @@ -2487,8 +2487,13 @@ static int journal_read(struct super_block *sb)
> >  static struct reiserfs_journal_list *alloc_journal_list(struct super_block *s)
> >  {
> >  	struct reiserfs_journal_list *jl;
> > -	jl = kzalloc(sizeof(struct reiserfs_journal_list),
> > -		     GFP_NOFS | __GFP_NOFAIL);
> > +
> > +	do {
> > +		jl = kzalloc(sizeof(struct reiserfs_journal_list), GFP_NOFS);
> > +		if (unlikely(!jl))
> > +			congestion_wait(BLK_RW_ASYNC, HZ/50);
> > +	} while (!jl)
> > +
> 
> Dammit, who has been running around converting __GFP_NOFAIL into
> open-coded congestion_wait() loops?
> 
> The whole point of __GFP_NOFAIL is to centralise this
> wait-for-memory-for-ever operation.  So it is implemented in a common
> (core) place and so that we can easily locate these problematic
> callers.
> 
> This comment in ext4:
> 
> 			/*
> 			 * If __GFP_FS is not present, then we may be
> 			 * being called from inside the fs writeback
> 			 * layer, so we MUST NOT fail.  Since
> 			 * __GFP_NOFAIL is going away, we will arrange
> 			 * to retry the allocation ourselves.
> 			 */
> 
> is exactly wrong.  Yes, we'd like __GFP_NOFAIL to go away, but it
> cannot go away until buggy callsites such as this one are *fixed*. 
> Removing the __GFP_NOFAIL usage simply hides the buggy code from casual
> searchers.
> 
> argh.
> 
> What we should do is to fix all these call sites so they can handle
> memory exhaustion.  That's hard so in the interim they should be using
> __GFP_NOFAIL.
> 

Ok, if even ext4 comments are wrong, things gonna be very difficult :)
Any sample of a callsite transition done well ?  (git id ?)

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[Andrew Morton]

On Sat, 22 Mar 2014 00:21:59 +0100 Fabian Frederick <fabf@skynet.be> wrote:

> > What we should do is to fix all these call sites so they can handle
> > memory exhaustion.  That's hard so in the interim they should be using
> > __GFP_NOFAIL.
> > 
> 
> Ok, if even ext4 comments are wrong, things gonna be very difficult :)
> Any sample of a callsite transition done well ?  (git id ?)

grep NOFAIL fs/jbd/*.c

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[tytso]

On Fri, Mar 21, 2014 at 01:00:55PM -0700, Andrew Morton wrote:
> 
> The whole point of __GFP_NOFAIL is to centralise this
> wait-for-memory-for-ever operation.  So it is implemented in a common
> (core) place and so that we can easily locate these problematic
> callers.
> 
> is exactly wrong.  Yes, we'd like __GFP_NOFAIL to go away, but it
> cannot go away until buggy callsites such as this one are *fixed*. 
> Removing the __GFP_NOFAIL usage simply hides the buggy code from casual
> searchers.

The change to jbd2 was made in July 2010, back when the "we must
exterminate GFP_NOFAIL at all costs" brigade was in high gear, and the
folks claiming that GFP_FAIL *would* go away, come hell or high water,
was a bit more emphatic.

I'll note that since 2011, there has been precious little movement on
removing the final few callers of GFP_NOFAIL, and we still have a bit
under two dozen of them, including a new one in fs/buffer.c that was
added in 2013.

In any case, __GFP_NOFAIL is in the code comments, so a casual
searcher would find it pretty quickly with a "git grep".

	       	       	      	      	   - Ted

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[Andrew Morton]

On Sat, 22 Mar 2014 13:03:22 -0400 tytso@mit.edu wrote:

> On Fri, Mar 21, 2014 at 01:00:55PM -0700, Andrew Morton wrote:
> > 
> > The whole point of __GFP_NOFAIL is to centralise this
> > wait-for-memory-for-ever operation.  So it is implemented in a common
> > (core) place and so that we can easily locate these problematic
> > callers.
> > 
> > is exactly wrong.  Yes, we'd like __GFP_NOFAIL to go away, but it
> > cannot go away until buggy callsites such as this one are *fixed*. 
> > Removing the __GFP_NOFAIL usage simply hides the buggy code from casual
> > searchers.
> 
> The change to jbd2 was made in July 2010, back when the "we must
> exterminate GFP_NOFAIL at all costs" brigade was in high gear, and the
> folks claiming that GFP_FAIL *would* go away, come hell or high water,
> was a bit more emphatic.

Whoever was saying that had the wrong end of the stick.  It's all very
odd.

> I'll note that since 2011, there has been precious little movement on
> removing the final few callers of GFP_NOFAIL, and we still have a bit
> under two dozen of them, including a new one in fs/buffer.c that was
> added in 2013.

Well.  Converting an existing retry-for-ever caller to GFP_NOFAIL is
good.  Adding new retry-for-ever code is not good.

> In any case, __GFP_NOFAIL is in the code comments, so a casual
> searcher would find it pretty quickly with a "git grep".

There's that.  But retry-for-ever is a common operation which the core
allocator can implement and maintain better than remote callsites.

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[tytso]

On Sat, Mar 22, 2014 at 10:15:12AM -0700, Andrew Morton wrote:
> > I'll note that since 2011, there has been precious little movement on
> > removing the final few callers of GFP_NOFAIL, and we still have a bit
> > under two dozen of them, including a new one in fs/buffer.c that was
> > added in 2013.
> 
> Well.  Converting an existing retry-for-ever caller to GFP_NOFAIL is
> good.  Adding new retry-for-ever code is not good.

Actually, it wasn't converting an existing loop; it was adding a new
GFP_NOFAIL to fix a reclaim livelock (commit 84235de394d9775bf).

I agree that in ideal world, we'd get rid of all of these.  But
sometimes, the cure can be worse than the disesae, and so the whole
"all callers of GFP_NOFAIL are MUST FIX BUGGGY and the maintainers
should be shamed into fixing it" attitude is one that I find a bit odd
myself.

	      	    	    	     	     - Ted

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[tytso]

On Sat, Mar 22, 2014 at 01:26:06PM -0400, tytso@MIT.EDU wrote:
> > Well.  Converting an existing retry-for-ever caller to GFP_NOFAIL is
> > good.  Adding new retry-for-ever code is not good.

Oh, and BTW --- now that checkpatch.pl now flags an warning whenever
GFP_NOFAIL is used, because it is deprecated, patches that convert a
loop to use GFP_NOFAIL will get flagged with checkpatch, and this will
also incentivize people writing new code and who can't find any other
way to deal with an allocation failure to simply open code the loop...

  	 	    	   	     	 - Ted




     	    	   

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[Andrew Morton]

On Sat, 22 Mar 2014 13:32:07 -0400 tytso@mit.edu wrote:

> On Sat, Mar 22, 2014 at 01:26:06PM -0400, tytso@MIT.EDU wrote:
> > > Well.  Converting an existing retry-for-ever caller to GFP_NOFAIL is
> > > good.  Adding new retry-for-ever code is not good.
> 
> Oh, and BTW --- now that checkpatch.pl now flags an warning whenever
> GFP_NOFAIL is used

I don't know what the basis for this NOFAIL-is-going-away theory could
have been.  What's the point in taking a centrally implemented piece of
logic and splattering its implementation out to tens of different
callsites?

Obviously I was asleep when I merged that checkpatch change.



From: Andrew Morton <akpm@linux-foundation.org>
Subject: scripts/checkpatch.pl: __GFP_NOFAIL isn't going away

Revert 7e4915e78992eb ("checkpatch: add warning of future __GFP_NOFAIL use").

There are no plans to remove __GFP_NOFAIL.

__GFP_NOFAIL exists to

a) centralise the retry-allocation-for-ever operation into the core
   allocator, which is the appropriate implementation site and

b) permit us to identify code sites which aren't handling memory
   exhaustion appropriately.

Cc: David Rientjes <rientjes@google.com>
Cc: Joe Perches <joe@perches.com>
Cc: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 scripts/checkpatch.pl |    6 ------
 1 file changed, 6 deletions(-)

diff -puN scripts/checkpatch.pl~scripts-checkpatchpl-__gfp_nofail-isnt-going-away scripts/checkpatch.pl
--- a/scripts/checkpatch.pl~scripts-checkpatchpl-__gfp_nofail-isnt-going-away
+++ a/scripts/checkpatch.pl
@@ -4240,12 +4240,6 @@ sub process {
 			     "$1 uses number as first arg, sizeof is generally wrong\n" . $herecurr);
 		}
 
-# check for GFP_NOWAIT use
-		if ($line =~ /\b__GFP_NOFAIL\b/) {
-			WARN("__GFP_NOFAIL",
-			     "Use of __GFP_NOFAIL is deprecated, no new users should be added\n" . $herecurr);
-		}
-
 # check for multiple semicolons
 		if ($line =~ /;\s*;\s*$/) {
 			if (WARN("ONE_SEMICOLON",
_

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[tytso]

On Sat, Mar 22, 2014 at 10:55:24AM -0700, Andrew Morton wrote:
> 
> From: Andrew Morton <akpm@linux-foundation.org>
> Subject: scripts/checkpatch.pl: __GFP_NOFAIL isn't going away
> 
> Revert 7e4915e78992eb ("checkpatch: add warning of future __GFP_NOFAIL use").
> 
> There are no plans to remove __GFP_NOFAIL.
> 
> __GFP_NOFAIL exists to
> 
> a) centralise the retry-allocation-for-ever operation into the core
>    allocator, which is the appropriate implementation site and
> 
> b) permit us to identify code sites which aren't handling memory
>    exhaustion appropriately.
> 
> Cc: David Rientjes <rientjes@google.com>
> Cc: Joe Perches <joe@perches.com>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

How about also making the following change which inspired the
checkpatch warning?

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 0437439..d189872 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -58,9 +58,11 @@ struct vm_area_struct;
  * __GFP_REPEAT: Try hard to allocate the memory, but the allocation attempt
  * _might_ fail.  This depends upon the particular VM implementation.
  *
- * __GFP_NOFAIL: The VM implementation _must_ retry infinitely: the caller
- * cannot handle allocation failures.  This modifier is deprecated and no new
- * users should be added.
+ * __GFP_NOFAIL: The VM implementation _must_ retry infinitely: the
+ * caller cannot handle allocation failures.  Callers are strongly
+ * encouraged not to use __GFP_NOFAIL unless the alternative is worse
+ * than OOM killing some random process (i.e., corruption or loss of
+ * some innocent user's data, etc).
  *
  * __GFP_NORETRY: The VM implementation must not retry indefinitely.
  *

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[Joe Perches]

On Sat, 2014-03-22 at 10:55 -0700, Andrew Morton wrote:
> On Sat, 22 Mar 2014 13:32:07 -0400 tytso@mit.edu wrote:
> 
> > On Sat, Mar 22, 2014 at 01:26:06PM -0400, tytso@MIT.EDU wrote:
> > > > Well.  Converting an existing retry-for-ever caller to GFP_NOFAIL is
> > > > good.  Adding new retry-for-ever code is not good.
> > 
> > Oh, and BTW --- now that checkpatch.pl now flags an warning whenever
> > GFP_NOFAIL is used
> 
> I don't know what the basis for this NOFAIL-is-going-away theory could
> have been.  What's the point in taking a centrally implemented piece of
> logic and splattering its implementation out to tens of different
> callsites?
[]
> diff -puN scripts/checkpatch.pl~scripts-checkpatchpl-__gfp_nofail-isnt-going-away scripts/checkpatch.pl
[]
> @@ -4240,12 +4240,6 @@ sub process {
>  			     "$1 uses number as first arg, sizeof is generally wrong\n" . $herecurr);
>  		}
>  
> -# check for GFP_NOWAIT use
> -		if ($line =~ /\b__GFP_NOFAIL\b/) {
> -			WARN("__GFP_NOFAIL",
> -			     "Use of __GFP_NOFAIL is deprecated, no new users should be added\n" . $herecurr);
> -		}

How about just changing this message to something like:

			WARN("__GFP_NOFAIL",
			     "Use of __GFP_NOFAIL may cause the OOM handler to kill a random process\n" . $herecurr);

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[Dave Jones]

On Sat, Mar 22, 2014 at 10:55:24AM -0700, Andrew Morton wrote:
 > On Sat, 22 Mar 2014 13:32:07 -0400 tytso@mit.edu wrote:
 > 
 > > On Sat, Mar 22, 2014 at 01:26:06PM -0400, tytso@MIT.EDU wrote:
 > > > > Well.  Converting an existing retry-for-ever caller to GFP_NOFAIL is
 > > > > good.  Adding new retry-for-ever code is not good.
 > > 
 > > Oh, and BTW --- now that checkpatch.pl now flags an warning whenever
 > > GFP_NOFAIL is used
 > 
 > I don't know what the basis for this NOFAIL-is-going-away theory could
 > have been.  What's the point in taking a centrally implemented piece of
 > logic and splattering its implementation out to tens of different
 > callsites?

I wonder if some of that thinking came from this..

commit dab48dab37d2770824420d1e01730a107fade1aa
Author: Andrew Morton <akpm@linux-foundation.org>
Date:   Tue Jun 16 15:32:37 2009 -0700

    page-allocator: warn if __GFP_NOFAIL is used for a large allocation
    
    __GFP_NOFAIL is a bad fiction.  Allocations _can_ fail, and callers should
    detect and suitably handle this (and not by lamely moving the infinite
    loop up to the caller level either).


Perhaps some of the commentary in that changeset should be updated too.
Linus changed it from single page to > order 1 in 4923abf9f1a4c1864af438a57c1f3686548230e9
but there's still this..

+                        * __GFP_NOFAIL is not to be used in new code.
+                        *
+                        * All __GFP_NOFAIL callers should be fixed so that they
+                        * properly detect and handle allocation failures.


Additionally, I don't recall seeing that warn_on trigger in a while.
I have vague memories that e1000e used to step on it from time to time, but
maybe that got fixed.

	Dave

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[Fabian Frederick]

On Sat, 22 Mar 2014 10:55:24 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Sat, 22 Mar 2014 13:32:07 -0400 tytso@mit.edu wrote:
> 
> > On Sat, Mar 22, 2014 at 01:26:06PM -0400, tytso@MIT.EDU wrote:
> > > > Well.  Converting an existing retry-for-ever caller to GFP_NOFAIL is
> > > > good.  Adding new retry-for-ever code is not good.
> > 
> > Oh, and BTW --- now that checkpatch.pl now flags an warning whenever
> > GFP_NOFAIL is used
> 
> I don't know what the basis for this NOFAIL-is-going-away theory could
> have been.  What's the point in taking a centrally implemented piece of
> logic and splattering its implementation out to tens of different
> callsites?
> 
> Obviously I was asleep when I merged that checkpatch change.

Exactly where confusion came from, thanks !

Fabian

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[One Thousand Gnomes]

> The whole point of __GFP_NOFAIL is to centralise this
> wait-for-memory-for-ever operation.  So it is implemented in a common
> (core) place and so that we can easily locate these problematic
> callers.

For reiserfs I'm not sure it's worth bothering. If you get an out of
memory or disk caused block read failure in the wrong location it'll copy
random data over low physical memory (its one of several filesystems
which happily memcpy to errno values)

Making reiserfs go away would be a much better long term option IMHO

Alan

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[David Rientjes]

On Sat, 22 Mar 2014, Dave Jones wrote:

> On Sat, Mar 22, 2014 at 10:55:24AM -0700, Andrew Morton wrote:
>  > On Sat, 22 Mar 2014 13:32:07 -0400 tytso@mit.edu wrote:
>  > 
>  > > On Sat, Mar 22, 2014 at 01:26:06PM -0400, tytso@MIT.EDU wrote:
>  > > > > Well.  Converting an existing retry-for-ever caller to GFP_NOFAIL is
>  > > > > good.  Adding new retry-for-ever code is not good.
>  > > 
>  > > Oh, and BTW --- now that checkpatch.pl now flags an warning whenever
>  > > GFP_NOFAIL is used
>  > 
>  > I don't know what the basis for this NOFAIL-is-going-away theory could
>  > have been.  What's the point in taking a centrally implemented piece of
>  > logic and splattering its implementation out to tens of different
>  > callsites?
> 
> I wonder if some of that thinking came from this..
> 
> commit dab48dab37d2770824420d1e01730a107fade1aa
> Author: Andrew Morton <akpm@linux-foundation.org>
> Date:   Tue Jun 16 15:32:37 2009 -0700
> 
>     page-allocator: warn if __GFP_NOFAIL is used for a large allocation
>     
>     __GFP_NOFAIL is a bad fiction.  Allocations _can_ fail, and callers should
>     detect and suitably handle this (and not by lamely moving the infinite
>     loop up to the caller level either).
> 

It came from me pointing out the fact that __GFP_NOFAIL requires 
__GFP_WAIT to actually never fail in the page allocator's implementation.  
I wanted to fix that, Andrew said nobody is currently doing 
GFP_NOWAIT | __GFP_NOFAIL or GFP_ATOMIC | __GFP_NOFAIL so let's warn 
against new callers being added and hopefully eventually get rid of it.  
In those cases, we also don't invoke the oom killer because we don't have 
__GFP_FS so we livelock.

The point is not to add new callers and new code should handle NULL 
correctly, not that we should run around changing current users to just do 
infinite retries.  Checkpatch should have nothing to do with that.

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[David Rientjes]

On Sat, 22 Mar 2014, Joe Perches wrote:

> How about just changing this message to something like:
> 
> 			WARN("__GFP_NOFAIL",
> 			     "Use of __GFP_NOFAIL may cause the OOM handler to kill a random process\n" . $herecurr);

Because it doesn't, the GFP_NOFS allocation in question already cannot 
invoke the oom killer.

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[tytso]

On Tue, Mar 25, 2014 at 06:06:17PM -0700, David Rientjes wrote:
> 
> The point is not to add new callers and new code should handle NULL 
> correctly, not that we should run around changing current users to just do 
> infinite retries.  Checkpatch should have nothing to do with that.

My problem with this doctrinaire "there should never be any new users"
is that sometiems there *are* worse things than infinite retries.  If
the alternative is bringing the entire system down, or livelocking the
entire system, or corrupting user data, __GFP_NOFAIL *is* the more
appropriate option.

If you try to tell those of us outside of the mm layer, "thou shalt
never use __GFP_NOFAIL in new code", and we have some new code where
the alternative is worse, we can either open-code the loop, or have
some mm hackers and/or checkpatch whine at us.

Andrew has declared that he'd prefer that we not open code the retry
loop; if you want to disagree with Andrew, feel free to pursuade him
otherwise.  If you want to tell me that I should accept user data
corruption, I'm going to ignore you (and/or checkpatch).

Regards,

						- Ted

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[Andrew Morton]

On Wed, 26 Mar 2014 02:19:04 -0400 tytso@mit.edu wrote:

> On Tue, Mar 25, 2014 at 06:06:17PM -0700, David Rientjes wrote:
> > 
> > The point is not to add new callers and new code should handle NULL 
> > correctly, not that we should run around changing current users to just do 
> > infinite retries.  Checkpatch should have nothing to do with that.
> 
> My problem with this doctrinaire "there should never be any new users"
> is that sometiems there *are* worse things than infinite retries.  If
> the alternative is bringing the entire system down, or livelocking the
> entire system, or corrupting user data, __GFP_NOFAIL *is* the more
> appropriate option.

Well, there are always alternatives.  For example ext3 could
preallocate a single transaction_t and a single IO page and fall back
to synchronous page-at-a-time journal writes.  But I can totally see
that such things are unattractive: heaps of new code which is never
tested in real life.  The page allocator works so damn well that it
doesn't make sense to implement it.

> If you try to tell those of us outside of the mm layer, "thou shalt
> never use __GFP_NOFAIL in new code", and we have some new code where
> the alternative is worse, we can either open-code the loop, or have
> some mm hackers and/or checkpatch whine at us.
> 
> Andrew has declared that he'd prefer that we not open code the retry
> loop; if you want to disagree with Andrew, feel free to pursuade him
> otherwise.  If you want to tell me that I should accept user data
> corruption, I'm going to ignore you (and/or checkpatch).

Please use NOFAIL ;) The core page allocator will always be able to
implement this better than callers.

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[tytso]

On Tue, Mar 25, 2014 at 11:32:09PM -0700, Andrew Morton wrote:
> Well, there are always alternatives.  For example ext3 could
> preallocate a single transaction_t and a single IO page and fall back
> to synchronous page-at-a-time journal writes.  But I can totally see
> that such things are unattractive: heaps of new code which is never
> tested in real life.  The page allocator works so damn well that it
> doesn't make sense to implement it.

I'm not sure that there are _always_ solutions.  For example, ext3
still needs to use buffer cache to do the I/O, and that may require
allocations as well.  Fortunately, this was patched around other ways
to avoid livelock issues in the page cleaner in 2013: 84235de394d977

But that's another new user of GFP_NOFAIL (and one added three years
after David tried to declare There Shalt Be No New Users of
GFP_NOFAIL), and sure, we could probably patch around that by having
places where there's no other alternaive to keep a preallocated batch
of pages and/or allocated structures at each code site.  But that's as
bad as looping at each code site; in fact, wouldn't it be better if
the allocator wants to avoid looping, to have a separate batch of
pages which (ala GFP_ATOMIC) which is reserved for just for GFP_NOFAIL
allocations when all else fails?

(BTW, we have a similar issue with bio's in the block layer, but
fortunately, those structures are relatively small, and since they are
in their own slab cache, and are constantly being used and then
recycled, it's in practice rare that allocations will fail when we are
in a desparate low memory situation.  But technically there will be
places where we should pass a gfp_t down to the block layers, and use
GFP_NOFAIL if we really want to make sure that memory allocations
needed to try to clean pages and/or avoid user data corruption are
needed.)

> Please use NOFAIL ;) The core page allocator will always be able to
> implement this better than callers.

I'll convert jbd2 to use NOFAIL.  :-)

Cheers,

						- Ted

Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __GFP_NOFAIL

[David Rientjes]

On Wed, 26 Mar 2014, tytso@mit.edu wrote:

> But that's another new user of GFP_NOFAIL (and one added three years
> after David tried to declare There Shalt Be No New Users of
> GFP_NOFAIL), and sure, we could probably patch around that by having
> places where there's no other alternaive to keep a preallocated batch
> of pages and/or allocated structures at each code site.  But that's as
> bad as looping at each code site; in fact, wouldn't it be better if
> the allocator wants to avoid looping, to have a separate batch of
> pages which (ala GFP_ATOMIC) which is reserved for just for GFP_NOFAIL
> allocations when all else fails?
> 

I didn't declare nobody should be adding __GFP_NOFAIL three years ago, 
rather three months ago I proposed a patch to fix __GFP_NOFAIL for 
GFP_ATOMIC allocations you're talking about above since, guess what, 
GPF_ATOMIC | __GFP_NOFAIL today easily returns NULL.  I tried fixing that 
failable-__GFP_NOFAIL problem with 
http://marc.info/?l=linux-kernel&m=138662620812698 but Andrew requested a 
WARN_ON_ONCE() instead since nobody is currently doing that and we agreed 
to warn against new users.

So we should either return to my earlier patch to actually make 
__GFP_NOFAIL not fail, or improve (but not remove) the checkpatch warning 
for these failable cases.  I couldn't care less if we add 5,000 new 
__GFP_NOFAIL users tomorrow, I just care that it does what is expected if 
people are going to be adding them.