[PATCH] ext4: annotate unhandled kmem_cache_alloc() error

[PATCH] ext4: annotate unhandled kmem_cache_alloc() error

[Akinobu Mita]

kmem_cache_alloc() error in ext4_mb_free_metadata is not handled.
I don't know how to fix it. But I can put BUG_ON to let others
know the problem.

Cc: Theodore Tso <tytso@mit.edu>
Cc: adilger@sun.com
Cc: linux-ext4@vger.kernel.org
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 fs/ext4/mballoc.c |    1 +
 1 file changed, 1 insertion(+)

Index: 2.6-rc/fs/ext4/mballoc.c
===================================================================
--- 2.6-rc.orig/fs/ext4/mballoc.c
+++ 2.6-rc/fs/ext4/mballoc.c
@@ -4417,6 +4417,7 @@ ext4_mb_free_metadata(handle_t *handle, 
 	BUG_ON(e4b->bd_buddy_page == NULL);
 
 	new_entry  = kmem_cache_alloc(ext4_free_ext_cachep, GFP_NOFS);
+	BUG_ON(!new_entry);
 	new_entry->start_blk = block;
 	new_entry->group  = group;
 	new_entry->count = count;

Re: [PATCH] ext4: annotate unhandled kmem_cache_alloc() error

[Josef Bacik]

On Tue, Dec 23, 2008 at 07:40:18PM +0900, Akinobu Mita wrote:
> kmem_cache_alloc() error in ext4_mb_free_metadata is not handled.
> I don't know how to fix it. But I can put BUG_ON to let others
> know the problem.
> 
> Cc: Theodore Tso <tytso@mit.edu>
> Cc: adilger@sun.com
> Cc: linux-ext4@vger.kernel.org
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  fs/ext4/mballoc.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> Index: 2.6-rc/fs/ext4/mballoc.c
> ===================================================================
> --- 2.6-rc.orig/fs/ext4/mballoc.c
> +++ 2.6-rc/fs/ext4/mballoc.c
> @@ -4417,6 +4417,7 @@ ext4_mb_free_metadata(handle_t *handle, 
>  	BUG_ON(e4b->bd_buddy_page == NULL);
>  
>  	new_entry  = kmem_cache_alloc(ext4_free_ext_cachep, GFP_NOFS);
> +	BUG_ON(!new_entry);
>  	new_entry->start_blk = block;
>  	new_entry->group  = group;
>  	new_entry->count = count;

BUG_ON is good for devel things, but for stable stuff it's better to let
somebody know an error occured rather than panic'ing the box.  The proper
solution would be to do

if (!new_entry)
	return -ENOMEM;

or if there is some out: label set ret to -ENOMEM and goto out, whatever is
appropriate in the context.  Thanks,

Josef

Re: [PATCH] ext4: annotate unhandled kmem_cache_alloc() error

[Akinobu Mita]

> BUG_ON is good for devel things, but for stable stuff it's better to let
> somebody know an error occured rather than panic'ing the box.  The proper
> solution would be to do
>
> if (!new_entry)
>        return -ENOMEM;
>
> or if there is some out: label set ret to -ENOMEM and goto out, whatever is
> appropriate in the context.  Thanks,

I don't understand the code around here well.
But I think it is not that simple.

The "new_entry" is needed to free blocks. If it just returns error,
it leaks something.

[PATCH] ext4: fix unhandled ext4_free_data allocation failure

[Akinobu Mita]

In ext4_mb_free_blocks() ext4_free_data allocation failure
is not handled. This error handling cannot be simple error return because
ext4_mb_free_blocks() cannot fail.

This patch add __GFP_NOFAIL to gfp mask for the allocation.

Cc: Theodore Tso <tytso@mit.edu>
Cc: adilger@sun.com
Cc: linux-ext4@vger.kernel.org
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 fs/ext4/mballoc.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: 2.6-git/fs/ext4/mballoc.c
===================================================================
--- 2.6-git.orig/fs/ext4/mballoc.c
+++ 2.6-git/fs/ext4/mballoc.c
@@ -4885,7 +4885,8 @@ do_more:
 		 * blocks being freed are metadata. these blocks shouldn't
 		 * be used until this transaction is committed
 		 */
-		new_entry  = kmem_cache_alloc(ext4_free_ext_cachep, GFP_NOFS);
+		new_entry  = kmem_cache_alloc(ext4_free_ext_cachep,
+						GFP_NOFS | __GFP_NOFAIL);
 		new_entry->start_blk = bit;
 		new_entry->group  = block_group;
 		new_entry->count = count;

Re: [PATCH] ext4: fix unhandled ext4_free_data allocation failure

[Josef Bacik]

On Sun, Jan 11, 2009 at 11:03:53AM +0900, Akinobu Mita wrote:
> In ext4_mb_free_blocks() ext4_free_data allocation failure
> is not handled. This error handling cannot be simple error return because
> ext4_mb_free_blocks() cannot fail.
> 
> This patch add __GFP_NOFAIL to gfp mask for the allocation.
> 
> Cc: Theodore Tso <tytso@mit.edu>
> Cc: adilger@sun.com
> Cc: linux-ext4@vger.kernel.org
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

Sorry but thats still not right, the fs should never force the box to come up
with memory, it should be able to gracefully handle ENOMEM cases.  This patch
does this properly.  Thanks,

Signed-off-by: Josef Bacik <jbacik@redhat.com>

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 918aec0..e97ea09 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -4886,6 +4886,10 @@ do_more:
 		 * be used until this transaction is committed
 		 */
 		new_entry  = kmem_cache_alloc(ext4_free_ext_cachep, GFP_NOFS);
+		if (!new_entry) {
+			err = -ENOMEM;
+			goto error_return;
+		}
 		new_entry->start_blk = bit;
 		new_entry->group  = block_group;
 		new_entry->count = count;

Re: [PATCH] ext4: fix unhandled ext4_free_data allocation failure

[Eric Sandeen]

Josef Bacik wrote:
> On Sun, Jan 11, 2009 at 11:03:53AM +0900, Akinobu Mita wrote:
>> In ext4_mb_free_blocks() ext4_free_data allocation failure
>> is not handled. This error handling cannot be simple error return because
>> ext4_mb_free_blocks() cannot fail.
>>
>> This patch add __GFP_NOFAIL to gfp mask for the allocation.
>>
>> Cc: Theodore Tso <tytso@mit.edu>
>> Cc: adilger@sun.com
>> Cc: linux-ext4@vger.kernel.org
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> 
> Sorry but thats still not right, the fs should never force the box to come up
> with memory, it should be able to gracefully handle ENOMEM cases.  This patch
> does this properly.  Thanks,
> 
> Signed-off-by: Josef Bacik <jbacik@redhat.com>
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 918aec0..e97ea09 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -4886,6 +4886,10 @@ do_more:
>  		 * be used until this transaction is committed
>  		 */
>  		new_entry  = kmem_cache_alloc(ext4_free_ext_cachep, GFP_NOFS);
> +		if (!new_entry) {
> +			err = -ENOMEM;
> +			goto error_return;
> +		}
>  		new_entry->start_blk = bit;
>  		new_entry->group  = block_group;
>  		new_entry->count = count;

Well, this will now force a filesystem error (then remount-ro or panic
(or ignore) if the allocation fails.  I'm not sure that's better...?

-Eric

Re: [PATCH] ext4: fix unhandled ext4_free_data allocation failure

[Theodore Tso]

On Sun, Jan 11, 2009 at 08:46:32AM -0600, Eric Sandeen wrote:
> 
> Well, this will now force a filesystem error (then remount-ro or panic
> (or ignore) if the allocation fails.  I'm not sure that's better...?
> 

Well, our choices basically are:

1)  Force a filesystem error
2)  Sleep and retry the allocation
3)  Don't add the freed blocks to the list regions that mballoc should
    be allowed to allocate from after the transaction commits.  This 
    results in the blocks getting "leaked" until the filesystem is
    mounted/unounted.

						- Ted

Re: [PATCH] ext4: fix unhandled ext4_free_data allocation failure

[Theodore Tso]

On Sun, Jan 11, 2009 at 10:58:35PM -0500, Theodore Tso wrote:
> On Sun, Jan 11, 2009 at 08:46:32AM -0600, Eric Sandeen wrote:
> > 
> > Well, this will now force a filesystem error (then remount-ro or panic
> > (or ignore) if the allocation fails.  I'm not sure that's better...?
> > 
> 
> Well, our choices basically are:
> 
> 1)  Force a filesystem error
> 2)  Sleep and retry the allocation
> 3)  Don't add the freed blocks to the list regions that mballoc should
>     be allowed to allocate from after the transaction commits.  This 
>     results in the blocks getting "leaked" until the filesystem is
>     mounted/unounted.

I just thought of another alternative:

4) Mark the buddy cache has being in need of being completely rebuilt
after the transaction commits. 

Someone want to try coding that up?

						- Ted

Re: [PATCH] ext4: fix unhandled ext4_free_data allocation failure

[Theodore Tso]

On Sun, Jan 11, 2009 at 10:58:35PM -0500, Theodore Tso wrote:
> On Sun, Jan 11, 2009 at 08:46:32AM -0600, Eric Sandeen wrote:
> > 
> > Well, this will now force a filesystem error (then remount-ro or panic
> > (or ignore) if the allocation fails.  I'm not sure that's better...?
> > 
> 
> Well, our choices basically are:
> 
> 1)  Force a filesystem error
> 2)  Sleep and retry the allocation
> 3)  Don't add the freed blocks to the list regions that mballoc should
>     be allowed to allocate from after the transaction commits.  This 
>     results in the blocks getting "leaked" until the filesystem is
>     mounted/unounted.

I just thought of another alternative:

4) Mark the buddy cache has being in need of being completely rebuilt
after the transaction commits. 

Someone want to try coding that up?

						- Ted