Re: [PATCH 1/5] jbd2: Pass extra bool parameter in journal routines to specify if its ok to fail the journal transaction allocation.

[Manish Katiyar <mkatiyar@gmail.com>]

On Wed, May 18, 2011 at 5:44 AM, Jan Kara <jack@suse.cz> wrote:
> On Wed 18-05-11 01:36:48, Manish Katiyar wrote:
>> On Wed, May 18, 2011 at 1:09 AM, Jan Kara <jack@suse.cz> wrote:
>> > On Tue 17-05-11 23:38:05, Manish Katiyar wrote:
>> >> On Mon, May 16, 2011 at 8:51 AM, Jan Kara <jack@suse.cz> wrote:
>> >> >  Hello,
>> >> >
>> >> > On Thu 12-05-11 23:37:05, Manish Katiyar wrote:
>> >> >> On Wed, May 11, 2011 at 8:54 AM, Jan Kara <jack@suse.cz> wrote:
>> >> >> >  Hi,
>> >> >> >
>> >> >> >  sorry I got to your patches with a delay. One general note - please do
>> >> >> > not attach patches. It is enough to have them in the email...
>> >> >> >
>> >> >> > On Sun 24-04-11 17:10:41, Manish Katiyar wrote:
>> >> >> >> Pass extra bool parameter in journal routines to specify if its ok to
>> >> >> >> fail the journal transaction allocation. If 'true' is passed
>> >> >> >> transaction allocation is done through GFP_KERNEL  and ENOMEM is
>> >> >> >> returned else GFP_NOFS is used.
>> >> >> >  Please, do not mix error handling with gfp masks. Instead just rename
>> >> >> > jbd2__journal_start() to jbd2_journal_start() and change gfp_mask parameter
>> >> >> > to "bool errok".
>> >> >>
>> >> >> ok.
>> >> >>
>> >> >> > Use GFP_NOFS gfp mask for start_this_handle().
>> >> >> I think I didn't completely understand this line. You meant passing
>> >> >> GFP_KERNEL or GFP_NOFS based on errok right ?
>> >> >  No, I meant passing GFP_NOFS always. Currently, GFP_NOFS is used in all
>> >> > the cases (noone uses GFP_KERNEL variant) and GFP_KERNEL can really be used
>> >> > only when we do not hold other filesystem locks (as GFP_KERNEL allocation
>> >> > can recurse back into filesystem to reclaim memory). So using GFP_KERNEL
>> >> > would need more auditting and is a separate issue anyway.
>> >>
>> >> Hi Jan,
>> >>
>> >> How about this ? As suggested I have removed special handlers for
>> >> ocfs2, always pass false as default for allocation and have removed
>> >> jbd2__journal_start and collapsed it in jbd2_journal_start.
>> >>
>> >>
>> >> Pass extra bool parameter in journal routines to specify if its ok to
>> >> fail the journal transaction allocation.  Update ocfs2 and ext4 routines to
>> >> pass false for the updated journal interface.
>> >  Yes, now the patch looks good! Thanks. Just one nit below:
>>
>> Thanks a lot. Fixed the newline issue. Here is the updated diff.
>>
>> Pass extra bool parameter in journal routines to specify if its ok to
>> fail the journal transaction allocation.  Update ocfs2 and ext4 routines to
>> pass false for the updated journal interface.
>  The patch look good now. You can add:
> Acked-by: Jan Kara <jack@suse.cz>
>
>                                                                        Honza

Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
Acked-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ext4_jbd2.h   |    2 +-
 fs/ext4/super.c       |    2 +-
 fs/jbd2/transaction.c |   24 +++++-------------------
 fs/ocfs2/journal.c    |    6 +++---
 include/linux/jbd2.h  |    6 ++----
 5 files changed, 12 insertions(+), 28 deletions(-)

diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
index d0f5353..0abee1b 100644
--- a/fs/ext4/ext4_jbd2.h
+++ b/fs/ext4/ext4_jbd2.h
@@ -225,7 +225,7 @@ static inline int ext4_journal_extend(handle_t
*handle, int nblocks)
 static inline int ext4_journal_restart(handle_t *handle, int nblocks)
 {
 	if (ext4_handle_valid(handle))
-		return jbd2_journal_restart(handle, nblocks);
+		return jbd2_journal_restart(handle, nblocks, false);
 	return 0;
 }

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 8553dfb..4e4c17f 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -279,7 +279,7 @@ handle_t *ext4_journal_start_sb(struct super_block
*sb, int nblocks)
 		ext4_abort(sb, "Detected aborted journal");
 		return ERR_PTR(-EROFS);
 	}
-	return jbd2_journal_start(journal, nblocks);
+	return jbd2_journal_start(journal, nblocks, false);
 }

 /*
diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 05fa77a..b5c2550 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -318,7 +318,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, int gfp_mask)
+handle_t *jbd2_journal_start(journal_t *journal, int nblocks, bool errok)
 {
 	handle_t *handle = journal_current_handle();
 	int err;
@@ -338,7 +338,7 @@ handle_t *jbd2__journal_start(journal_t *journal,
int nblocks, int gfp_mask)

 	current->journal_info = handle;

-	err = start_this_handle(journal, handle, gfp_mask);
+	err = start_this_handle(journal, handle, GFP_NOFS);
 	if (err < 0) {
 		jbd2_free_handle(handle);
 		current->journal_info = NULL;
@@ -346,13 +346,6 @@ handle_t *jbd2__journal_start(journal_t *journal,
int nblocks, int gfp_mask)
 	}
 	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);


@@ -441,7 +434,7 @@ out:
  * transaction capabable of guaranteeing the requested number of
  * credits.
  */
-int jbd2__journal_restart(handle_t *handle, int nblocks, int gfp_mask)
+int jbd2_journal_restart(handle_t *handle, int nblocks, bool errok)
 {
 	transaction_t *transaction = handle->h_transaction;
 	journal_t *journal = transaction->t_journal;
@@ -477,16 +470,9 @@ int jbd2__journal_restart(handle_t *handle, int
nblocks, int gfp_mask)

 	lock_map_release(&handle->h_lockdep_map);
 	handle->h_buffer_credits = nblocks;
-	ret = start_this_handle(journal, handle, gfp_mask);
+	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);

 /**
@@ -1436,7 +1422,7 @@ int jbd2_journal_force_commit(journal_t *journal)
 	handle_t *handle;
 	int ret;

-	handle = jbd2_journal_start(journal, 1);
+	handle = jbd2_journal_start(journal, 1, false);
 	if (IS_ERR(handle)) {
 		ret = PTR_ERR(handle);
 	} else {
diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index b141a44..cca0f76 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -353,11 +353,11 @@ handle_t *ocfs2_start_trans(struct ocfs2_super
*osb, int max_buffs)

 	/* Nested transaction? Just return the handle... */
 	if (journal_current_handle())
-		return jbd2_journal_start(journal, max_buffs);
+		return jbd2_journal_start(journal, max_buffs, false);

 	down_read(&osb->journal->j_trans_barrier);

-	handle = jbd2_journal_start(journal, max_buffs);
+	handle = jbd2_journal_start(journal, max_buffs, false);
 	if (IS_ERR(handle)) {
 		up_read(&osb->journal->j_trans_barrier);

@@ -438,7 +438,7 @@ int ocfs2_extend_trans(handle_t *handle, int nblocks)
 	if (status > 0) {
 		trace_ocfs2_extend_trans_restart(old_nblocks + nblocks);
 		status = jbd2_journal_restart(handle,
-					      old_nblocks + nblocks);
+					      old_nblocks + nblocks, false);
 		if (status < 0) {
 			mlog_errno(status);
 			goto bail;
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index a32dcae..67f0f4f 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1103,10 +1103,8 @@ static inline handle_t *journal_current_handle(void)
  * Register buffer modifications against the current transaction.
  */

-extern handle_t *jbd2_journal_start(journal_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 handle_t *jbd2_journal_start(journal_t *, int nblocks, bool errok);
+extern int	 jbd2_journal_restart(handle_t *, int nblocks, bool errok);
 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.4.1


-- 
Thanks -
Manish
--
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