* Re: [patch 6/6] jbd2: remove dependency on __GFP_NOFAIL [not found] ` <alpine.DEB.2.00.1007221618001.4856@chino.kir.corp.google.com> @ 2010-07-23 14:10 ` Ted Ts'o 2010-07-23 14:57 ` Jan Kara 0 siblings, 1 reply; 6+ messages in thread From: Ted Ts'o @ 2010-07-23 14:10 UTC (permalink / raw) To: David Rientjes Cc: Jan Kara, Andrew Morton, Andreas Dilger, Jiri Kosina, linux-mm, linux-ext4 On Thu, Jul 22, 2010 at 04:24:23PM -0700, David Rientjes wrote: > > I didn't think about converting the existing GFP_NOFS | __GFP_NOFAIL > callers into the do-while loop above until you mentioned it, thanks. I'll > send patches to do that shortly. Here's what I'm planning on queueing for the next merge window, along with patches to ext4 to use jbd2__journal_start(..., GFP_KERNEL) in places where we can afford to fail. After doing some analysis, the places where we can afford to fail are also the places where we can use GFP_KERNEL instead of GFP_NOFS, so conveniently, I'm using the lack of __GFP_FS to indicate that we should do the retry loop in start_this_handle(). I also added the congestion_wait() call since there's no point busy-looping the CPU while we're waiting for pages to get swapped or paged out. Comments would be appreciated. - Ted >From 814be805d5e3d12343e590631ff9bc2d65c8f60a Mon Sep 17 00:00:00 2001 From: Theodore Ts'o <tytso@mit.edu> Date: Fri, 23 Jul 2010 10:06:53 -0400 Subject: [PATCH] jbd2: Remove __GFP_NOFAIL from jbd2 layer __GFP_NOFAIL is going away, so add our own retry loop. Also add jbd2__journal_start() and jbd2__journal_restart() which take a gfp mask, so that file systems can optionally (re)start transaction handles using GFP_KERNEL. If they do this, then they need to be prepared to handle receiving an PTR_ERR(-ENOMEM) error, and be ready to reflect that error up to userspace. Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> --- fs/jbd2/journal.c | 14 +++++++++-- fs/jbd2/transaction.c | 60 +++++++++++++++++++++++++++++++++--------------- include/linux/jbd2.h | 4 ++- 3 files changed, 55 insertions(+), 23 deletions(-) diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index f7bf157..eb84fbd 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -48,8 +48,6 @@ #include <asm/uaccess.h> #include <asm/page.h> -EXPORT_SYMBOL(jbd2_journal_start); -EXPORT_SYMBOL(jbd2_journal_restart); EXPORT_SYMBOL(jbd2_journal_extend); EXPORT_SYMBOL(jbd2_journal_stop); EXPORT_SYMBOL(jbd2_journal_lock_updates); @@ -311,7 +309,17 @@ int jbd2_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); +retry_alloc: + new_bh = alloc_buffer_head(GFP_NOFS); + if (!new_bh) { + /* + * Failure is not an option, but __GFP_NOFAIL is going + * away; so we retry ourselves here. + */ + congestion_wait(BLK_RW_ASYNC, HZ/50); + goto retry_alloc; + } + /* keep subsequent assertions sane */ new_bh->b_state = 0; init_buffer(new_bh, NULL, NULL); diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c index e214d68..43241c0 100644 --- a/fs/jbd2/transaction.c +++ b/fs/jbd2/transaction.c @@ -83,30 +83,39 @@ jbd2_get_transaction(journal_t *journal, transaction_t *transaction) * transaction's buffer credits. */ -static int start_this_handle(journal_t *journal, handle_t *handle) +static int start_this_handle(journal_t *journal, handle_t *handle, + int gfp_mask) { transaction_t *transaction; int needed; int nblocks = handle->h_buffer_credits; transaction_t *new_transaction = NULL; - int ret = 0; unsigned long ts = jiffies; if (nblocks > journal->j_max_transaction_buffers) { printk(KERN_ERR "JBD: %s wants too many credits (%d > %d)\n", current->comm, nblocks, journal->j_max_transaction_buffers); - ret = -ENOSPC; - goto out; + return -ENOSPC; } alloc_transaction: if (!journal->j_running_transaction) { - new_transaction = kzalloc(sizeof(*new_transaction), - GFP_NOFS|__GFP_NOFAIL); + retry_alloc: + new_transaction = kzalloc(sizeof(*new_transaction), gfp_mask); if (!new_transaction) { - ret = -ENOMEM; - goto out; + /* + * If __GFP_FS is not present, then we may be + * being called from inside the fs writeback + * layer, so we MUST NOT fail. Since + * __GFP_NOFAIL is going away, we will arrange + * to retry the allocation ourselves. + */ + if ((gfp & __GFP_FS) == 0) { + congestion_wait(BLK_RW_ASYNC, HZ/50); + goto retry_alloc; + } + return -ENOMEM; } } @@ -123,8 +132,8 @@ repeat_locked: if (is_journal_aborted(journal) || (journal->j_errno != 0 && !(journal->j_flags & JBD2_ACK_ERR))) { spin_unlock(&journal->j_state_lock); - ret = -EROFS; - goto out; + kfree(new_transaction); + return -EROFS; } /* Wait on the journal's transaction barrier if necessary */ @@ -240,10 +249,8 @@ repeat_locked: spin_unlock(&journal->j_state_lock); lock_map_acquire(&handle->h_lockdep_map); -out: - if (unlikely(new_transaction)) /* It's usually NULL */ - kfree(new_transaction); - return ret; + kfree(new_transaction); + return 0; } static struct lock_class_key jbd2_handle_key; @@ -278,7 +285,7 @@ static handle_t *new_handle(int nblocks) * * Return a pointer to a newly allocated handle, or NULL on failure */ -handle_t *jbd2_journal_start(journal_t *journal, int nblocks) +handle_t *jbd2__journal_start(journal_t *journal, int nblocks, int gfp_mask) { handle_t *handle = journal_current_handle(); int err; @@ -298,7 +305,7 @@ handle_t *jbd2_journal_start(journal_t *journal, int nblocks) current->journal_info = handle; - err = start_this_handle(journal, handle); + err = start_this_handle(journal, handle, GFP_NOFS); if (err < 0) { jbd2_free_handle(handle); current->journal_info = NULL; @@ -308,6 +315,15 @@ handle_t *jbd2_journal_start(journal_t *journal, int nblocks) out: return handle; } +EXPORT_SYMBOL(jbd2__journal_start); + + +handle_t *jbd2_journal_start(journal_t *journal, int nblocks) +{ + return jbd2__journal_start(journal, nblocks, GFP_NOFS); +} +EXPORT_SYMBOL(jbd2_journal_start); + /** * int jbd2_journal_extend() - extend buffer credits. @@ -394,8 +410,7 @@ out: * transaction capabable of guaranteeing the requested number of * credits. */ - -int jbd2_journal_restart(handle_t *handle, int nblocks) +int jbd2__journal_restart(handle_t *handle, int nblocks, int gfp_mask) { transaction_t *transaction = handle->h_transaction; journal_t *journal = transaction->t_journal; @@ -428,10 +443,17 @@ int jbd2_journal_restart(handle_t *handle, int nblocks) lock_map_release(&handle->h_lockdep_map); handle->h_buffer_credits = nblocks; - ret = start_this_handle(journal, handle); + ret = start_this_handle(journal, handle, GFP_NOFS); return ret; } +EXPORT_SYMBOL(jbd2__journal_restart); + +int jbd2_journal_restart(handle_t *handle, int nblocks) +{ + return jbd2__journal_restart(handle, nblocks, GFP_NOFS); +} +EXPORT_SYMBOL(jbd2_journal_restart); /** * void jbd2_journal_lock_updates () - establish a transaction barrier. diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index a4d2e9f..5a72bc7 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -1081,7 +1081,9 @@ static inline handle_t *journal_current_handle(void) */ extern handle_t *jbd2_journal_start(journal_t *, int nblocks); -extern int jbd2_journal_restart (handle_t *, int nblocks); +extern handle_t *jbd2__journal_start(journal_t *, int nblocks, int gfp_mask); +extern int jbd2_journal_restart(handle_t *, int nblocks); +extern int jbd2__journal_restart(handle_t *, int nblocks, int gfp_mask); extern int jbd2_journal_extend (handle_t *, int nblocks); extern int jbd2_journal_get_write_access(handle_t *, struct buffer_head *); extern int jbd2_journal_get_create_access (handle_t *, struct buffer_head *); -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [patch 6/6] jbd2: remove dependency on __GFP_NOFAIL 2010-07-23 14:10 ` [patch 6/6] jbd2: remove dependency on __GFP_NOFAIL Ted Ts'o @ 2010-07-23 14:57 ` Jan Kara 2010-07-23 15:05 ` Ted Ts'o 0 siblings, 1 reply; 6+ messages in thread From: Jan Kara @ 2010-07-23 14:57 UTC (permalink / raw) To: Ted Ts'o Cc: David Rientjes, Jan Kara, Andrew Morton, Andreas Dilger, Jiri Kosina, linux-mm, linux-ext4 On Fri 23-07-10 10:10:54, Ted Ts'o wrote: > On Thu, Jul 22, 2010 at 04:24:23PM -0700, David Rientjes wrote: > > > > I didn't think about converting the existing GFP_NOFS | __GFP_NOFAIL > > callers into the do-while loop above until you mentioned it, thanks. I'll > > send patches to do that shortly. ... > From 814be805d5e3d12343e590631ff9bc2d65c8f60a Mon Sep 17 00:00:00 2001 > From: Theodore Ts'o <tytso@mit.edu> > Date: Fri, 23 Jul 2010 10:06:53 -0400 > Subject: [PATCH] jbd2: Remove __GFP_NOFAIL from jbd2 layer > > __GFP_NOFAIL is going away, so add our own retry loop. Also add > jbd2__journal_start() and jbd2__journal_restart() which take a gfp > mask, so that file systems can optionally (re)start transaction > handles using GFP_KERNEL. If they do this, then they need to be > prepared to handle receiving an PTR_ERR(-ENOMEM) error, and be ready > to reflect that error up to userspace. > > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> > --- > fs/jbd2/journal.c | 14 +++++++++-- > fs/jbd2/transaction.c | 60 +++++++++++++++++++++++++++++++++--------------- > include/linux/jbd2.h | 4 ++- > 3 files changed, 55 insertions(+), 23 deletions(-) > > diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c > index e214d68..43241c0 100644 > --- a/fs/jbd2/transaction.c > +++ b/fs/jbd2/transaction.c > @@ -83,30 +83,39 @@ jbd2_get_transaction(journal_t *journal, transaction_t *transaction) > * transaction's buffer credits. > */ > > -static int start_this_handle(journal_t *journal, handle_t *handle) > +static int start_this_handle(journal_t *journal, handle_t *handle, > + int gfp_mask) > { > transaction_t *transaction; > int needed; > int nblocks = handle->h_buffer_credits; > transaction_t *new_transaction = NULL; > - int ret = 0; > unsigned long ts = jiffies; > > if (nblocks > journal->j_max_transaction_buffers) { > printk(KERN_ERR "JBD: %s wants too many credits (%d > %d)\n", > current->comm, nblocks, > journal->j_max_transaction_buffers); > - ret = -ENOSPC; > - goto out; > + return -ENOSPC; > } > > alloc_transaction: > if (!journal->j_running_transaction) { > - new_transaction = kzalloc(sizeof(*new_transaction), > - GFP_NOFS|__GFP_NOFAIL); > + retry_alloc: > + new_transaction = kzalloc(sizeof(*new_transaction), gfp_mask); > if (!new_transaction) { > - ret = -ENOMEM; > - goto out; > + /* > + * If __GFP_FS is not present, then we may be > + * being called from inside the fs writeback > + * layer, so we MUST NOT fail. Since > + * __GFP_NOFAIL is going away, we will arrange > + * to retry the allocation ourselves. > + */ > + if ((gfp & __GFP_FS) == 0) { > + congestion_wait(BLK_RW_ASYNC, HZ/50); > + goto retry_alloc; You could as well go to alloc_transaction label above... > + } > + return -ENOMEM; > } > } > ... > @@ -278,7 +285,7 @@ static handle_t *new_handle(int nblocks) > * > * Return a pointer to a newly allocated handle, or NULL on failure > */ > -handle_t *jbd2_journal_start(journal_t *journal, int nblocks) > +handle_t *jbd2__journal_start(journal_t *journal, int nblocks, int gfp_mask) > { > handle_t *handle = journal_current_handle(); > int err; > @@ -298,7 +305,7 @@ handle_t *jbd2_journal_start(journal_t *journal, int nblocks) > > current->journal_info = handle; > > - err = start_this_handle(journal, handle); > + err = start_this_handle(journal, handle, GFP_NOFS); Here you want to use gfp_mask I guess. > if (err < 0) { > jbd2_free_handle(handle); > current->journal_info = NULL; > @@ -308,6 +315,15 @@ handle_t *jbd2_journal_start(journal_t *journal, int nblocks) > out: > return handle; > } > +EXPORT_SYMBOL(jbd2__journal_start); > + > + > +handle_t *jbd2_journal_start(journal_t *journal, int nblocks) > +{ > + return jbd2__journal_start(journal, nblocks, GFP_NOFS); > +} > +EXPORT_SYMBOL(jbd2_journal_start); > + > > /** > * int jbd2_journal_extend() - extend buffer credits. > @@ -394,8 +410,7 @@ out: > * transaction capabable of guaranteeing the requested number of > * credits. > */ > - > -int jbd2_journal_restart(handle_t *handle, int nblocks) > +int jbd2__journal_restart(handle_t *handle, int nblocks, int gfp_mask) > { > transaction_t *transaction = handle->h_transaction; > journal_t *journal = transaction->t_journal; > @@ -428,10 +443,17 @@ int jbd2_journal_restart(handle_t *handle, int nblocks) > > lock_map_release(&handle->h_lockdep_map); > handle->h_buffer_credits = nblocks; > - ret = start_this_handle(journal, handle); > + ret = start_this_handle(journal, handle, GFP_NOFS); And here you want to use gfp_mask as well. > return ret; > } > +EXPORT_SYMBOL(jbd2__journal_restart); > + > > +int jbd2_journal_restart(handle_t *handle, int nblocks) > +{ > + return jbd2__journal_restart(handle, nblocks, GFP_NOFS); > +} > +EXPORT_SYMBOL(jbd2_journal_restart); > > /** > * void jbd2_journal_lock_updates () - establish a transaction barrier. Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 6/6] jbd2: remove dependency on __GFP_NOFAIL 2010-07-23 14:57 ` Jan Kara @ 2010-07-23 15:05 ` Ted Ts'o 2010-07-23 15:32 ` Jan Kara 2010-07-23 19:40 ` David Rientjes 0 siblings, 2 replies; 6+ messages in thread From: Ted Ts'o @ 2010-07-23 15:05 UTC (permalink / raw) To: Jan Kara Cc: David Rientjes, Andrew Morton, Andreas Dilger, Jiri Kosina, linux-mm, linux-ext4 Yeah, oops. Nice catches. I also hadn't done a test compile, so there were some missing #include's. So once more, this time with feeling... - Ted >From d24408e1b50e47b21b7d2ec5857b710e9b752dc9 Mon Sep 17 00:00:00 2001 From: Theodore Ts'o <tytso@mit.edu> Date: Fri, 23 Jul 2010 11:03:45 -0400 Subject: [PATCH] jbd2: Remove __GFP_NOFAIL from jbd2 layer __GFP_NOFAIL is going away, so add our own retry loop. Also add jbd2__journal_start() and jbd2__journal_restart() which take a gfp mask, so that file systems can optionally (re)start transaction handles using GFP_KERNEL. If they do this, then they need to be prepared to handle receiving an PTR_ERR(-ENOMEM) error, and be ready to reflect that error up to userspace. Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> --- fs/jbd2/journal.c | 15 +++++++++-- fs/jbd2/transaction.c | 61 +++++++++++++++++++++++++++++++++--------------- include/linux/jbd2.h | 4 ++- 3 files changed, 57 insertions(+), 23 deletions(-) diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index f7bf157..a79d334 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -41,6 +41,7 @@ #include <linux/hash.h> #include <linux/log2.h> #include <linux/vmalloc.h> +#include <linux/backing-dev.h> #define CREATE_TRACE_POINTS #include <trace/events/jbd2.h> @@ -48,8 +49,6 @@ #include <asm/uaccess.h> #include <asm/page.h> -EXPORT_SYMBOL(jbd2_journal_start); -EXPORT_SYMBOL(jbd2_journal_restart); EXPORT_SYMBOL(jbd2_journal_extend); EXPORT_SYMBOL(jbd2_journal_stop); EXPORT_SYMBOL(jbd2_journal_lock_updates); @@ -311,7 +310,17 @@ int jbd2_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); +retry_alloc: + new_bh = alloc_buffer_head(GFP_NOFS); + if (!new_bh) { + /* + * Failure is not an option, but __GFP_NOFAIL is going + * away; so we retry ourselves here. + */ + congestion_wait(BLK_RW_ASYNC, HZ/50); + goto retry_alloc; + } + /* keep subsequent assertions sane */ new_bh->b_state = 0; init_buffer(new_bh, NULL, NULL); diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c index e214d68..001e95f 100644 --- a/fs/jbd2/transaction.c +++ b/fs/jbd2/transaction.c @@ -26,6 +26,8 @@ #include <linux/mm.h> #include <linux/highmem.h> #include <linux/hrtimer.h> +#include <linux/backing-dev.h> +#include <linux/module.h> static void __jbd2_journal_temp_unlink_buffer(struct journal_head *jh); @@ -83,30 +85,38 @@ jbd2_get_transaction(journal_t *journal, transaction_t *transaction) * transaction's buffer credits. */ -static int start_this_handle(journal_t *journal, handle_t *handle) +static int start_this_handle(journal_t *journal, handle_t *handle, + int gfp_mask) { transaction_t *transaction; int needed; int nblocks = handle->h_buffer_credits; transaction_t *new_transaction = NULL; - int ret = 0; unsigned long ts = jiffies; if (nblocks > journal->j_max_transaction_buffers) { printk(KERN_ERR "JBD: %s wants too many credits (%d > %d)\n", current->comm, nblocks, journal->j_max_transaction_buffers); - ret = -ENOSPC; - goto out; + return -ENOSPC; } alloc_transaction: if (!journal->j_running_transaction) { - new_transaction = kzalloc(sizeof(*new_transaction), - GFP_NOFS|__GFP_NOFAIL); + new_transaction = kzalloc(sizeof(*new_transaction), gfp_mask); if (!new_transaction) { - ret = -ENOMEM; - goto out; + /* + * If __GFP_FS is not present, then we may be + * being called from inside the fs writeback + * layer, so we MUST NOT fail. Since + * __GFP_NOFAIL is going away, we will arrange + * to retry the allocation ourselves. + */ + if ((gfp_mask & __GFP_FS) == 0) { + congestion_wait(BLK_RW_ASYNC, HZ/50); + goto alloc_transaction; + } + return -ENOMEM; } } @@ -123,8 +133,8 @@ repeat_locked: if (is_journal_aborted(journal) || (journal->j_errno != 0 && !(journal->j_flags & JBD2_ACK_ERR))) { spin_unlock(&journal->j_state_lock); - ret = -EROFS; - goto out; + kfree(new_transaction); + return -EROFS; } /* Wait on the journal's transaction barrier if necessary */ @@ -240,10 +250,8 @@ repeat_locked: spin_unlock(&journal->j_state_lock); lock_map_acquire(&handle->h_lockdep_map); -out: - if (unlikely(new_transaction)) /* It's usually NULL */ - kfree(new_transaction); - return ret; + kfree(new_transaction); + return 0; } static struct lock_class_key jbd2_handle_key; @@ -278,7 +286,7 @@ static handle_t *new_handle(int nblocks) * * Return a pointer to a newly allocated handle, or NULL on failure */ -handle_t *jbd2_journal_start(journal_t *journal, int nblocks) +handle_t *jbd2__journal_start(journal_t *journal, int nblocks, int gfp_mask) { handle_t *handle = journal_current_handle(); int err; @@ -298,7 +306,7 @@ handle_t *jbd2_journal_start(journal_t *journal, int nblocks) current->journal_info = handle; - err = start_this_handle(journal, handle); + err = start_this_handle(journal, handle, gfp_mask); if (err < 0) { jbd2_free_handle(handle); current->journal_info = NULL; @@ -308,6 +316,15 @@ handle_t *jbd2_journal_start(journal_t *journal, int nblocks) out: return handle; } +EXPORT_SYMBOL(jbd2__journal_start); + + +handle_t *jbd2_journal_start(journal_t *journal, int nblocks) +{ + return jbd2__journal_start(journal, nblocks, GFP_NOFS); +} +EXPORT_SYMBOL(jbd2_journal_start); + /** * int jbd2_journal_extend() - extend buffer credits. @@ -394,8 +411,7 @@ out: * transaction capabable of guaranteeing the requested number of * credits. */ - -int jbd2_journal_restart(handle_t *handle, int nblocks) +int jbd2__journal_restart(handle_t *handle, int nblocks, int gfp_mask) { transaction_t *transaction = handle->h_transaction; journal_t *journal = transaction->t_journal; @@ -428,10 +444,17 @@ int jbd2_journal_restart(handle_t *handle, int nblocks) lock_map_release(&handle->h_lockdep_map); handle->h_buffer_credits = nblocks; - ret = start_this_handle(journal, handle); + ret = start_this_handle(journal, handle, gfp_mask); return ret; } +EXPORT_SYMBOL(jbd2__journal_restart); + +int jbd2_journal_restart(handle_t *handle, int nblocks) +{ + return jbd2__journal_restart(handle, nblocks, GFP_NOFS); +} +EXPORT_SYMBOL(jbd2_journal_restart); /** * void jbd2_journal_lock_updates () - establish a transaction barrier. diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index a4d2e9f..5a72bc7 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -1081,7 +1081,9 @@ static inline handle_t *journal_current_handle(void) */ extern handle_t *jbd2_journal_start(journal_t *, int nblocks); -extern int jbd2_journal_restart (handle_t *, int nblocks); +extern handle_t *jbd2__journal_start(journal_t *, int nblocks, int gfp_mask); +extern int jbd2_journal_restart(handle_t *, int nblocks); +extern int jbd2__journal_restart(handle_t *, int nblocks, int gfp_mask); extern int jbd2_journal_extend (handle_t *, int nblocks); extern int jbd2_journal_get_write_access(handle_t *, struct buffer_head *); extern int jbd2_journal_get_create_access (handle_t *, struct buffer_head *); -- 1.7.0.4 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [patch 6/6] jbd2: remove dependency on __GFP_NOFAIL 2010-07-23 15:05 ` Ted Ts'o @ 2010-07-23 15:32 ` Jan Kara 2010-07-23 19:40 ` David Rientjes 1 sibling, 0 replies; 6+ messages in thread From: Jan Kara @ 2010-07-23 15:32 UTC (permalink / raw) To: Ted Ts'o Cc: Jan Kara, David Rientjes, Andrew Morton, Andreas Dilger, Jiri Kosina, linux-mm, linux-ext4 On Fri 23-07-10 11:05:43, Ted Ts'o wrote: > Yeah, oops. Nice catches. I also hadn't done a test compile, so > there were some missing #include's. > > So once more, this time with feeling... > > - Ted > > From d24408e1b50e47b21b7d2ec5857b710e9b752dc9 Mon Sep 17 00:00:00 2001 > From: Theodore Ts'o <tytso@mit.edu> > Date: Fri, 23 Jul 2010 11:03:45 -0400 > Subject: [PATCH] jbd2: Remove __GFP_NOFAIL from jbd2 layer > > __GFP_NOFAIL is going away, so add our own retry loop. Also add > jbd2__journal_start() and jbd2__journal_restart() which take a gfp > mask, so that file systems can optionally (re)start transaction > handles using GFP_KERNEL. If they do this, then they need to be > prepared to handle receiving an PTR_ERR(-ENOMEM) error, and be ready > to reflect that error up to userspace. > > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> Now the patch looks good. Acked-by: Jan Kara <jack@suse.cz> Honza > --- > fs/jbd2/journal.c | 15 +++++++++-- > fs/jbd2/transaction.c | 61 +++++++++++++++++++++++++++++++++--------------- > include/linux/jbd2.h | 4 ++- > 3 files changed, 57 insertions(+), 23 deletions(-) > > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c > index f7bf157..a79d334 100644 > --- a/fs/jbd2/journal.c > +++ b/fs/jbd2/journal.c > @@ -41,6 +41,7 @@ > #include <linux/hash.h> > #include <linux/log2.h> > #include <linux/vmalloc.h> > +#include <linux/backing-dev.h> > > #define CREATE_TRACE_POINTS > #include <trace/events/jbd2.h> > @@ -48,8 +49,6 @@ > #include <asm/uaccess.h> > #include <asm/page.h> > > -EXPORT_SYMBOL(jbd2_journal_start); > -EXPORT_SYMBOL(jbd2_journal_restart); > EXPORT_SYMBOL(jbd2_journal_extend); > EXPORT_SYMBOL(jbd2_journal_stop); > EXPORT_SYMBOL(jbd2_journal_lock_updates); > @@ -311,7 +310,17 @@ int jbd2_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); > +retry_alloc: > + new_bh = alloc_buffer_head(GFP_NOFS); > + if (!new_bh) { > + /* > + * Failure is not an option, but __GFP_NOFAIL is going > + * away; so we retry ourselves here. > + */ > + congestion_wait(BLK_RW_ASYNC, HZ/50); > + goto retry_alloc; > + } > + > /* keep subsequent assertions sane */ > new_bh->b_state = 0; > init_buffer(new_bh, NULL, NULL); > diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c > index e214d68..001e95f 100644 > --- a/fs/jbd2/transaction.c > +++ b/fs/jbd2/transaction.c > @@ -26,6 +26,8 @@ > #include <linux/mm.h> > #include <linux/highmem.h> > #include <linux/hrtimer.h> > +#include <linux/backing-dev.h> > +#include <linux/module.h> > > static void __jbd2_journal_temp_unlink_buffer(struct journal_head *jh); > > @@ -83,30 +85,38 @@ jbd2_get_transaction(journal_t *journal, transaction_t *transaction) > * transaction's buffer credits. > */ > > -static int start_this_handle(journal_t *journal, handle_t *handle) > +static int start_this_handle(journal_t *journal, handle_t *handle, > + int gfp_mask) > { > transaction_t *transaction; > int needed; > int nblocks = handle->h_buffer_credits; > transaction_t *new_transaction = NULL; > - int ret = 0; > unsigned long ts = jiffies; > > if (nblocks > journal->j_max_transaction_buffers) { > printk(KERN_ERR "JBD: %s wants too many credits (%d > %d)\n", > current->comm, nblocks, > journal->j_max_transaction_buffers); > - ret = -ENOSPC; > - goto out; > + return -ENOSPC; > } > > alloc_transaction: > if (!journal->j_running_transaction) { > - new_transaction = kzalloc(sizeof(*new_transaction), > - GFP_NOFS|__GFP_NOFAIL); > + new_transaction = kzalloc(sizeof(*new_transaction), gfp_mask); > if (!new_transaction) { > - ret = -ENOMEM; > - goto out; > + /* > + * If __GFP_FS is not present, then we may be > + * being called from inside the fs writeback > + * layer, so we MUST NOT fail. Since > + * __GFP_NOFAIL is going away, we will arrange > + * to retry the allocation ourselves. > + */ > + if ((gfp_mask & __GFP_FS) == 0) { > + congestion_wait(BLK_RW_ASYNC, HZ/50); > + goto alloc_transaction; > + } > + return -ENOMEM; > } > } > > @@ -123,8 +133,8 @@ repeat_locked: > if (is_journal_aborted(journal) || > (journal->j_errno != 0 && !(journal->j_flags & JBD2_ACK_ERR))) { > spin_unlock(&journal->j_state_lock); > - ret = -EROFS; > - goto out; > + kfree(new_transaction); > + return -EROFS; > } > > /* Wait on the journal's transaction barrier if necessary */ > @@ -240,10 +250,8 @@ repeat_locked: > spin_unlock(&journal->j_state_lock); > > lock_map_acquire(&handle->h_lockdep_map); > -out: > - if (unlikely(new_transaction)) /* It's usually NULL */ > - kfree(new_transaction); > - return ret; > + kfree(new_transaction); > + return 0; > } > > static struct lock_class_key jbd2_handle_key; > @@ -278,7 +286,7 @@ static handle_t *new_handle(int nblocks) > * > * Return a pointer to a newly allocated handle, or NULL on failure > */ > -handle_t *jbd2_journal_start(journal_t *journal, int nblocks) > +handle_t *jbd2__journal_start(journal_t *journal, int nblocks, int gfp_mask) > { > handle_t *handle = journal_current_handle(); > int err; > @@ -298,7 +306,7 @@ handle_t *jbd2_journal_start(journal_t *journal, int nblocks) > > current->journal_info = handle; > > - err = start_this_handle(journal, handle); > + err = start_this_handle(journal, handle, gfp_mask); > if (err < 0) { > jbd2_free_handle(handle); > current->journal_info = NULL; > @@ -308,6 +316,15 @@ handle_t *jbd2_journal_start(journal_t *journal, int nblocks) > out: > return handle; > } > +EXPORT_SYMBOL(jbd2__journal_start); > + > + > +handle_t *jbd2_journal_start(journal_t *journal, int nblocks) > +{ > + return jbd2__journal_start(journal, nblocks, GFP_NOFS); > +} > +EXPORT_SYMBOL(jbd2_journal_start); > + > > /** > * int jbd2_journal_extend() - extend buffer credits. > @@ -394,8 +411,7 @@ out: > * transaction capabable of guaranteeing the requested number of > * credits. > */ > - > -int jbd2_journal_restart(handle_t *handle, int nblocks) > +int jbd2__journal_restart(handle_t *handle, int nblocks, int gfp_mask) > { > transaction_t *transaction = handle->h_transaction; > journal_t *journal = transaction->t_journal; > @@ -428,10 +444,17 @@ int jbd2_journal_restart(handle_t *handle, int nblocks) > > lock_map_release(&handle->h_lockdep_map); > handle->h_buffer_credits = nblocks; > - ret = start_this_handle(journal, handle); > + ret = start_this_handle(journal, handle, gfp_mask); > return ret; > } > +EXPORT_SYMBOL(jbd2__journal_restart); > + > > +int jbd2_journal_restart(handle_t *handle, int nblocks) > +{ > + return jbd2__journal_restart(handle, nblocks, GFP_NOFS); > +} > +EXPORT_SYMBOL(jbd2_journal_restart); > > /** > * void jbd2_journal_lock_updates () - establish a transaction barrier. > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h > index a4d2e9f..5a72bc7 100644 > --- a/include/linux/jbd2.h > +++ b/include/linux/jbd2.h > @@ -1081,7 +1081,9 @@ static inline handle_t *journal_current_handle(void) > */ > > extern handle_t *jbd2_journal_start(journal_t *, int nblocks); > -extern int jbd2_journal_restart (handle_t *, int nblocks); > +extern handle_t *jbd2__journal_start(journal_t *, int nblocks, int gfp_mask); > +extern int jbd2_journal_restart(handle_t *, int nblocks); > +extern int jbd2__journal_restart(handle_t *, int nblocks, int gfp_mask); > extern int jbd2_journal_extend (handle_t *, int nblocks); > extern int jbd2_journal_get_write_access(handle_t *, struct buffer_head *); > extern int jbd2_journal_get_create_access (handle_t *, struct buffer_head *); > -- > 1.7.0.4 > -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 6/6] jbd2: remove dependency on __GFP_NOFAIL 2010-07-23 15:05 ` Ted Ts'o 2010-07-23 15:32 ` Jan Kara @ 2010-07-23 19:40 ` David Rientjes 2010-07-23 19:52 ` Ted Ts'o 1 sibling, 1 reply; 6+ messages in thread From: David Rientjes @ 2010-07-23 19:40 UTC (permalink / raw) To: Ted Ts'o Cc: Jan Kara, Andrew Morton, Andreas Dilger, Jiri Kosina, linux-mm, linux-ext4 On Fri, 23 Jul 2010, Ted Ts'o wrote: > __GFP_NOFAIL is going away, so add our own retry loop. Also add > jbd2__journal_start() and jbd2__journal_restart() which take a gfp > mask, so that file systems can optionally (re)start transaction > handles using GFP_KERNEL. If they do this, then they need to be > prepared to handle receiving an PTR_ERR(-ENOMEM) error, and be ready > to reflect that error up to userspace. > > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> Acked-by: David Rientjes <rientjes@google.com> Will you be pushing the equivalent patch for jbd? -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 6/6] jbd2: remove dependency on __GFP_NOFAIL 2010-07-23 19:40 ` David Rientjes @ 2010-07-23 19:52 ` Ted Ts'o 0 siblings, 0 replies; 6+ messages in thread From: Ted Ts'o @ 2010-07-23 19:52 UTC (permalink / raw) To: David Rientjes Cc: Jan Kara, Andrew Morton, Andreas Dilger, Jiri Kosina, linux-mm, linux-ext4 On Fri, Jul 23, 2010 at 12:40:50PM -0700, David Rientjes wrote: > On Fri, 23 Jul 2010, Ted Ts'o wrote: > > > __GFP_NOFAIL is going away, so add our own retry loop. Also add > > jbd2__journal_start() and jbd2__journal_restart() which take a gfp > > mask, so that file systems can optionally (re)start transaction > > handles using GFP_KERNEL. If they do this, then they need to be > > prepared to handle receiving an PTR_ERR(-ENOMEM) error, and be ready > > to reflect that error up to userspace. > > > > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> > > Acked-by: David Rientjes <rientjes@google.com> > > Will you be pushing the equivalent patch for jbd? I imagine Jan (as the person who has been primarily handling ext3 patches) is waiting to see how invasive the patches are to ext4 before deciding whether he's willing backport the equivalent changes to ext3 so that some of the calls that end up calling start_this_handle() end up passing GFP_KERNEL when it's OK for them to fail --- since ext3 is in maintenance mode, so we tend not to backport the more disruptive patches to ext3. It's really a cost/benefit tradeoff, really. I am certain that some application programmers will complain when their programs start breaking because they're not prepared to handle an ENOMEM failure from certain system calls that have never failed that way before. (I should introduce you to some Japanese enterprise programmers who believe that if a system call ever starts returning an error code not documented in a Linux manpage, it's a ABI compatibility bug. They're on crack, of course, but it's been hard convincing them that it's their fault for writing brittle/fragile applications.) So I could imagine that Jan and some of the enterprise distributions that are shipping ext3 might not want this change, given the "better the devil you know (random lockups in the case of extreme memory pressure)" is better than the devil you don't (more system calls might return ENOMEM, with undetermined application impacts, in the case of extreme memory pressure). So in the jbd layer, they might want to simply unconditionally loop, so it would be bug-for-bug compatible with existing ext3 behavior. - Ted ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-07-23 19:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <alpine.DEB.2.00.1007201936210.8728@chino.kir.corp.google.com>
[not found] ` <alpine.DEB.2.00.1007201943340.8728@chino.kir.corp.google.com>
[not found] ` <20100722141437.GA14882@thunk.org>
[not found] ` <alpine.DEB.2.00.1007221108360.30080@chino.kir.corp.google.com>
[not found] ` <20100722230935.GB16373@thunk.org>
[not found] ` <alpine.DEB.2.00.1007221618001.4856@chino.kir.corp.google.com>
2010-07-23 14:10 ` [patch 6/6] jbd2: remove dependency on __GFP_NOFAIL Ted Ts'o
2010-07-23 14:57 ` Jan Kara
2010-07-23 15:05 ` Ted Ts'o
2010-07-23 15:32 ` Jan Kara
2010-07-23 19:40 ` David Rientjes
2010-07-23 19:52 ` Ted Ts'o
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).