diff mbox

ext4: fix unhandled ext4_free_data allocation failure

Message ID 20090111143942.GA28488@unused.rdu.redhat.com
State Accepted, archived
Headers show

Commit Message

Josef Bacik Jan. 11, 2009, 2:39 p.m. UTC
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>

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Eric Sandeen Jan. 11, 2009, 2:46 p.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Y. Ts'o Jan. 12, 2009, 3:58 a.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Y. Ts'o Jan. 12, 2009, 3:03 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;