* [patch 4/6] jbd: remove dependency on __GFP_NOFAIL [not found] <alpine.DEB.2.00.1008161953430.17924@chino.kir.corp.google.com> @ 2010-08-17 2:58 ` David Rientjes 2010-08-17 9:51 ` Jan Kara 0 siblings, 1 reply; 8+ messages in thread From: David Rientjes @ 2010-08-17 2:58 UTC (permalink / raw) To: Andrew Morton; +Cc: Jan Kara, linux-ext4, linux-kernel Removes the dependency on __GFP_NOFAIL by looping indefinitely in the caller. The error handling when kzalloc() returns NULL in start_this_handle() was removed since it was unreachable. Signed-off-by: David Rientjes <rientjes@google.com> --- fs/jbd/journal.c | 5 ++++- fs/jbd/transaction.c | 14 ++++++-------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/fs/jbd/journal.c b/fs/jbd/journal.c --- a/fs/jbd/journal.c +++ b/fs/jbd/journal.c @@ -301,7 +301,10 @@ int journal_write_metadata_buffer(transaction_t *transaction, */ J_ASSERT_BH(bh_in, buffer_jbddirty(bh_in)); - new_bh = alloc_buffer_head(GFP_NOFS|__GFP_NOFAIL); + do { + /* FIXME: this may potentially loop forever */ + new_bh = alloc_buffer_head(GFP_NOFS); + } while (!new_bh); /* keep subsequent assertions sane */ new_bh->b_state = 0; init_buffer(new_bh, NULL, NULL); diff --git a/fs/jbd/transaction.c b/fs/jbd/transaction.c --- a/fs/jbd/transaction.c +++ b/fs/jbd/transaction.c @@ -98,14 +98,12 @@ static int start_this_handle(journal_t *journal, handle_t *handle) } alloc_transaction: - if (!journal->j_running_transaction) { - new_transaction = kzalloc(sizeof(*new_transaction), - GFP_NOFS|__GFP_NOFAIL); - if (!new_transaction) { - ret = -ENOMEM; - goto out; - } - } + if (!journal->j_running_transaction) + do { + /* FIXME: this may potentially loop forever */ + new_transaction = kzalloc(sizeof(*new_transaction), + GFP_NOFS); + } while (!new_transaction); jbd_debug(3, "New handle %p going live.\n", handle); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch 4/6] jbd: remove dependency on __GFP_NOFAIL 2010-08-17 2:58 ` [patch 4/6] jbd: remove dependency on __GFP_NOFAIL David Rientjes @ 2010-08-17 9:51 ` Jan Kara 2010-08-17 17:48 ` David Rientjes 2010-08-23 19:28 ` Andrew Morton 0 siblings, 2 replies; 8+ messages in thread From: Jan Kara @ 2010-08-17 9:51 UTC (permalink / raw) To: David Rientjes; +Cc: Andrew Morton, Jan Kara, linux-ext4, linux-kernel On Mon 16-08-10 19:58:01, David Rientjes wrote: > Removes the dependency on __GFP_NOFAIL by looping indefinitely in the > caller. > > The error handling when kzalloc() returns NULL in start_this_handle() > was removed since it was unreachable. Thanks! I've added the patch to my tree. Since rc1 is over, I think this is a material for the next merge window, right? I can take care of pushing it. If you want to push the change yourself, feel free to add Acked-by: Jan Kara <jack@suse.cz> Honza > > Signed-off-by: David Rientjes <rientjes@google.com> > --- > fs/jbd/journal.c | 5 ++++- > fs/jbd/transaction.c | 14 ++++++-------- > 2 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/fs/jbd/journal.c b/fs/jbd/journal.c > --- a/fs/jbd/journal.c > +++ b/fs/jbd/journal.c > @@ -301,7 +301,10 @@ int journal_write_metadata_buffer(transaction_t *transaction, > */ > J_ASSERT_BH(bh_in, buffer_jbddirty(bh_in)); > > - new_bh = alloc_buffer_head(GFP_NOFS|__GFP_NOFAIL); > + do { > + /* FIXME: this may potentially loop forever */ > + new_bh = alloc_buffer_head(GFP_NOFS); > + } while (!new_bh); > /* keep subsequent assertions sane */ > new_bh->b_state = 0; > init_buffer(new_bh, NULL, NULL); > diff --git a/fs/jbd/transaction.c b/fs/jbd/transaction.c > --- a/fs/jbd/transaction.c > +++ b/fs/jbd/transaction.c > @@ -98,14 +98,12 @@ static int start_this_handle(journal_t *journal, handle_t *handle) > } > > alloc_transaction: > - if (!journal->j_running_transaction) { > - new_transaction = kzalloc(sizeof(*new_transaction), > - GFP_NOFS|__GFP_NOFAIL); > - if (!new_transaction) { > - ret = -ENOMEM; > - goto out; > - } > - } > + if (!journal->j_running_transaction) > + do { > + /* FIXME: this may potentially loop forever */ > + new_transaction = kzalloc(sizeof(*new_transaction), > + GFP_NOFS); > + } while (!new_transaction); > > jbd_debug(3, "New handle %p going live.\n", handle); > -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch 4/6] jbd: remove dependency on __GFP_NOFAIL 2010-08-17 9:51 ` Jan Kara @ 2010-08-17 17:48 ` David Rientjes 2010-08-23 19:28 ` Andrew Morton 1 sibling, 0 replies; 8+ messages in thread From: David Rientjes @ 2010-08-17 17:48 UTC (permalink / raw) To: Jan Kara; +Cc: Andrew Morton, linux-ext4, linux-kernel On Tue, 17 Aug 2010, Jan Kara wrote: > > Removes the dependency on __GFP_NOFAIL by looping indefinitely in the > > caller. > > > > The error handling when kzalloc() returns NULL in start_this_handle() > > was removed since it was unreachable. > Thanks! I've added the patch to my tree. Since rc1 is over, I think this > is a material for the next merge window, right? Yes, we still need to switch over GFP_KERNEL callers and remove the flag completely, so there's no hurry for this to go into 2.6.36. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch 4/6] jbd: remove dependency on __GFP_NOFAIL 2010-08-17 9:51 ` Jan Kara 2010-08-17 17:48 ` David Rientjes @ 2010-08-23 19:28 ` Andrew Morton 2010-08-23 22:03 ` Jan Kara 1 sibling, 1 reply; 8+ messages in thread From: Andrew Morton @ 2010-08-23 19:28 UTC (permalink / raw) To: Jan Kara; +Cc: David Rientjes, linux-ext4, linux-kernel On Tue, 17 Aug 2010 11:51:03 +0200 Jan Kara <jack@suse.cz> wrote: > On Mon 16-08-10 19:58:01, David Rientjes wrote: > > Removes the dependency on __GFP_NOFAIL by looping indefinitely in the > > caller. > > > > The error handling when kzalloc() returns NULL in start_this_handle() > > was removed since it was unreachable. > Thanks! I've added the patch to my tree. Please unadd it. JBD should be fixed so that it can appropriately handle out-of-memory conditions. Until that time we shouldn't hide its shortcomings with this open-coded equivalent. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch 4/6] jbd: remove dependency on __GFP_NOFAIL 2010-08-23 19:28 ` Andrew Morton @ 2010-08-23 22:03 ` Jan Kara 2010-08-23 22:11 ` Andrew Morton 0 siblings, 1 reply; 8+ messages in thread From: Jan Kara @ 2010-08-23 22:03 UTC (permalink / raw) To: Andrew Morton; +Cc: Jan Kara, David Rientjes, linux-ext4, linux-kernel On Mon 23-08-10 12:28:13, Andrew Morton wrote: > On Tue, 17 Aug 2010 11:51:03 +0200 > Jan Kara <jack@suse.cz> wrote: > > > On Mon 16-08-10 19:58:01, David Rientjes wrote: > > > Removes the dependency on __GFP_NOFAIL by looping indefinitely in the > > > caller. > > > > > > The error handling when kzalloc() returns NULL in start_this_handle() > > > was removed since it was unreachable. > > Thanks! I've added the patch to my tree. > > Please unadd it. JBD should be fixed so that it can appropriately > handle out-of-memory conditions. Until that time we shouldn't hide its > shortcomings with this open-coded equivalent. Well, I wanted to make it easy for David so that he can proceed with his removal of __GFP_NOFAIL. I agree that pushing the looping from the allocator to the callers seems of a disputable value to me as well. So do you think that we should keep __GFP_NOFAIL as long as all callers are not able to handle allocation failures in more reasonable way? Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch 4/6] jbd: remove dependency on __GFP_NOFAIL 2010-08-23 22:03 ` Jan Kara @ 2010-08-23 22:11 ` Andrew Morton 2010-08-23 22:21 ` Jan Kara 2010-08-23 22:22 ` David Rientjes 0 siblings, 2 replies; 8+ messages in thread From: Andrew Morton @ 2010-08-23 22:11 UTC (permalink / raw) To: Jan Kara; +Cc: David Rientjes, linux-ext4, linux-kernel On Tue, 24 Aug 2010 00:03:47 +0200 Jan Kara <jack@suse.cz> wrote: > So do > you think that we should keep __GFP_NOFAIL as long as all callers are not > able to handle allocation failures in more reasonable way? The concept should be encapsulated in _some_ centralised fashion. Helper functions would work as well as __GFP_NOFAIL, and will move any runtime cost away from the good code and push it onto the bad code. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch 4/6] jbd: remove dependency on __GFP_NOFAIL 2010-08-23 22:11 ` Andrew Morton @ 2010-08-23 22:21 ` Jan Kara 2010-08-23 22:22 ` David Rientjes 1 sibling, 0 replies; 8+ messages in thread From: Jan Kara @ 2010-08-23 22:21 UTC (permalink / raw) To: Andrew Morton; +Cc: Jan Kara, David Rientjes, linux-ext4, linux-kernel On Mon 23-08-10 15:11:29, Andrew Morton wrote: > On Tue, 24 Aug 2010 00:03:47 +0200 > Jan Kara <jack@suse.cz> wrote: > > > So do > > you think that we should keep __GFP_NOFAIL as long as all callers are not > > able to handle allocation failures in more reasonable way? > > The concept should be encapsulated in _some_ centralised fashion. > > Helper functions would work as well as __GFP_NOFAIL, and will move any > runtime cost away from the good code and push it onto the bad code. Makes sense. Removed the patch. David, could you provide a function for non-failing allocation and then use this from JBD and whatever else code is also affected? That looks like a cleaner solution as Andrew points out... Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch 4/6] jbd: remove dependency on __GFP_NOFAIL 2010-08-23 22:11 ` Andrew Morton 2010-08-23 22:21 ` Jan Kara @ 2010-08-23 22:22 ` David Rientjes 1 sibling, 0 replies; 8+ messages in thread From: David Rientjes @ 2010-08-23 22:22 UTC (permalink / raw) To: Andrew Morton; +Cc: Jan Kara, linux-ext4, linux-kernel On Mon, 23 Aug 2010, Andrew Morton wrote: > > So do > > you think that we should keep __GFP_NOFAIL as long as all callers are not > > able to handle allocation failures in more reasonable way? > > The concept should be encapsulated in _some_ centralised fashion. > > Helper functions would work as well as __GFP_NOFAIL, and will move any > runtime cost away from the good code and push it onto the bad code. > There's no runtime cost on the bad code, the calls never loop since the page allocator already loops itself. Regardless, I'll add a helper function to include/linux/gfp.h to do this with a WARN_ON_ONCE() inside the loop in case any order > PAGE_ALLOC_COSTLY_ORDER callers are ever added (and I really hope nobody merges those). ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2010-08-23 22:22 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <alpine.DEB.2.00.1008161953430.17924@chino.kir.corp.google.com>
2010-08-17 2:58 ` [patch 4/6] jbd: remove dependency on __GFP_NOFAIL David Rientjes
2010-08-17 9:51 ` Jan Kara
2010-08-17 17:48 ` David Rientjes
2010-08-23 19:28 ` Andrew Morton
2010-08-23 22:03 ` Jan Kara
2010-08-23 22:11 ` Andrew Morton
2010-08-23 22:21 ` Jan Kara
2010-08-23 22:22 ` David Rientjes
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).