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@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Fri, 21 Mar 2014 17:18:30 +0100 Fabian Frederick <fabf@xxxxxxxxx> 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 ?)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/