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@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.

--
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/