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 <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Akinobu Mita <[email protected]>
---
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;
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 <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Akinobu Mita <[email protected]>
> ---
> 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
> 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.
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 <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Akinobu Mita <[email protected]>
---
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;
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 <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Akinobu Mita <[email protected]>
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 <[email protected]>
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;
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 <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Akinobu Mita <[email protected]>
>
> 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 <[email protected]>
>
> 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
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
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