ext4: annotate unhandled kmem_cache_alloc() error
PATCH 1/1 ext4: annotate unhandled kmem_cache_alloc() error
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.
--- fs/ext4/mballoc.c | 1 + 1 file changed, 1 insertion(+)
fs/ext4/mballoc.c +1 -0
@@ -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;
Quoted 17 lines · Akinobu Mita
kmem_cache_alloc()error inext4_mb_free_metadatais not handled. I don't know how to fix it. But I can putBUG_ONto let others know the problem.Cc Theodore Tso <[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
===================================================================
fs/ext4/mballoc.c ext4_mb_free_metadata(handle_t *handle, · line 4423
Quoted 7 lines
fs/ext4/mballoc.c +1
-0@@ -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
Quoted 9 lines
BUG_ONis 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 doif (!new_entry)
return -ENOMEM;or if there is some out: label set ret to
-ENOMEMand 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 1/1 ext4: fix unhandled ext4_free_data allocation failure
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.
--- fs/ext4/mballoc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
fs/ext4/mballoc.c +2 -1
@@ -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
Quoted 10 lines · Akinobu Mita
In
ext4_mb_free_blocks()ext4_free_dataallocation failure
is not handled. This error handling cannot be simple error return becauseext4_mb_free_blocks()cannot fail.This patch add
__GFP_NOFAILto gfp mask for the allocation.Cc Theodore Tso <[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,
fs/ext4/mballoc.c +4 -0
@@ -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
Quoted 23 lines · Josef Bacik
Quoted 10 lines · Akinobu Mita
In
ext4_mb_free_blocks()ext4_free_dataallocation failure
is not handled. This error handling cannot be simple error return becauseext4_mb_free_blocks()cannot fail.This patch add
__GFP_NOFAILto gfp mask for the allocation.Cc Theodore Tso <[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]>fs/ext4/mballoc.c +0
-0
fs/ext4/mballoc.c do_more: · line 4895
Quoted 10 lines
fs/ext4/mballoc.c +4
-0@@ -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
Theodore Tso
maintainerRe: [PATCH] ext4: fix unhandled ext4_free_data allocation failure
Quoted 4 lines · Eric Sandeen
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
Theodore Tso
maintainerRe: [PATCH] ext4: fix unhandled ext4_free_data allocation failure
Quoted 6 lines · Theodore Tso
Quoted 4 lines · Eric Sandeen
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...?
Quoted 8 lines
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 shouldbe 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