| Message ID | 20090111143942.GA28488@unused.rdu.redhat.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
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
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
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 --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;