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

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

[Manish Katiyar]

[-- Attachment #1: Type: text/plain, Size: 2618 bytes --]

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.


Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
---
 fs/jbd2/transaction.c |   12 +++++++-----
 include/linux/jbd2.h  |    4 ++--
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 05fa77a..b02d6a4 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -349,9 +349,10 @@ handle_t *jbd2__journal_start(journal_t *journal,
int nblocks, int gfp_mask)
 EXPORT_SYMBOL(jbd2__journal_start);


-handle_t *jbd2_journal_start(journal_t *journal, int nblocks)
+handle_t *jbd2_journal_start(journal_t *journal, int nblocks, bool errok)
 {
-	return jbd2__journal_start(journal, nblocks, GFP_NOFS);
+	return jbd2__journal_start(journal, nblocks,
+				   errok ? GFP_KERNEL : GFP_NOFS);
 }
 EXPORT_SYMBOL(jbd2_journal_start);

@@ -483,9 +484,10 @@ int jbd2__journal_restart(handle_t *handle, int
nblocks, int gfp_mask)
 EXPORT_SYMBOL(jbd2__journal_restart);


-int jbd2_journal_restart(handle_t *handle, int nblocks)
+int jbd2_journal_restart(handle_t *handle, int nblocks, bool errok)
 {
-	return jbd2__journal_restart(handle, nblocks, GFP_NOFS);
+	return jbd2__journal_restart(handle, nblocks,
+				     errok ? GFP_KERNEL : GFP_NOFS);
 }
 EXPORT_SYMBOL(jbd2_journal_restart);

@@ -1436,7 +1438,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/include/linux/jbd2.h b/include/linux/jbd2.h
index a32dcae..a64aced 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1103,9 +1103,9 @@ 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, bool errok);
 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, bool errok);
 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 *);
-- 
1.7.1


-- 
Thanks -
Manish

[-- Attachment #2: 0001-Pass-extra-bool-parameter-in-journal-routines-to-spe.patch --]
[-- Type: text/x-patch, Size: 2752 bytes --]

From c9edbd8d1da02ada2615acec3e3083b4669f6d9e Mon Sep 17 00:00:00 2001
From: Manish Katiyar <mkatiyar@gmail.com>
Date: Sun, 24 Apr 2011 00:14:54 -0700
Subject: [PATCH 1/5] 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 else GFP_NOFS is used.

Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
---
 fs/jbd2/transaction.c |   12 +++++++-----
 include/linux/jbd2.h  |    4 ++--
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 05fa77a..b02d6a4 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -349,9 +349,10 @@ handle_t *jbd2__journal_start(journal_t *journal, int nblocks, int gfp_mask)
 EXPORT_SYMBOL(jbd2__journal_start);
 
 
-handle_t *jbd2_journal_start(journal_t *journal, int nblocks)
+handle_t *jbd2_journal_start(journal_t *journal, int nblocks, bool errok)
 {
-	return jbd2__journal_start(journal, nblocks, GFP_NOFS);
+	return jbd2__journal_start(journal, nblocks,
+				   errok ? GFP_KERNEL : GFP_NOFS);
 }
 EXPORT_SYMBOL(jbd2_journal_start);
 
@@ -483,9 +484,10 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, int gfp_mask)
 EXPORT_SYMBOL(jbd2__journal_restart);
 
 
-int jbd2_journal_restart(handle_t *handle, int nblocks)
+int jbd2_journal_restart(handle_t *handle, int nblocks, bool errok)
 {
-	return jbd2__journal_restart(handle, nblocks, GFP_NOFS);
+	return jbd2__journal_restart(handle, nblocks,
+				     errok ? GFP_KERNEL : GFP_NOFS);
 }
 EXPORT_SYMBOL(jbd2_journal_restart);
 
@@ -1436,7 +1438,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/include/linux/jbd2.h b/include/linux/jbd2.h
index a32dcae..a64aced 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1103,9 +1103,9 @@ 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, bool errok);
 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, bool errok);
 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 *);
-- 
1.7.1

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

[Jan Kara]

  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". Use GFP_NOFS gfp mask for start_this_handle(). Noone
uses jbd2__journal_start() and it was mainly added to make it possible to
specify that start_this_handle() can fail but IMHO it's a hack.

Also your patch series should be bisectable - that means it must compile
and run after any of the patches. So you cannot really change
jbd2_journal_start() prototype without changing all the filesystems using
it. In this case, just include in this patch a simple change for ext4 and
ocfs2 to pass 'false' in the additional argument.

								Honza

> Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
> ---
>  fs/jbd2/transaction.c |   12 +++++++-----
>  include/linux/jbd2.h  |    4 ++--
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 05fa77a..b02d6a4 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -349,9 +349,10 @@ handle_t *jbd2__journal_start(journal_t *journal,
> int nblocks, int gfp_mask)
>  EXPORT_SYMBOL(jbd2__journal_start);
> 
> 
> -handle_t *jbd2_journal_start(journal_t *journal, int nblocks)
> +handle_t *jbd2_journal_start(journal_t *journal, int nblocks, bool errok)
>  {
> -	return jbd2__journal_start(journal, nblocks, GFP_NOFS);
> +	return jbd2__journal_start(journal, nblocks,
> +				   errok ? GFP_KERNEL : GFP_NOFS);
>  }
>  EXPORT_SYMBOL(jbd2_journal_start);
> 
> @@ -483,9 +484,10 @@ int jbd2__journal_restart(handle_t *handle, int
> nblocks, int gfp_mask)
>  EXPORT_SYMBOL(jbd2__journal_restart);
> 
> 
> -int jbd2_journal_restart(handle_t *handle, int nblocks)
> +int jbd2_journal_restart(handle_t *handle, int nblocks, bool errok)
>  {
> -	return jbd2__journal_restart(handle, nblocks, GFP_NOFS);
> +	return jbd2__journal_restart(handle, nblocks,
> +				     errok ? GFP_KERNEL : GFP_NOFS);
>  }
>  EXPORT_SYMBOL(jbd2_journal_restart);
> 
> @@ -1436,7 +1438,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/include/linux/jbd2.h b/include/linux/jbd2.h
> index a32dcae..a64aced 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1103,9 +1103,9 @@ 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, bool errok);
>  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, bool errok);
>  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 *);
> -- 
> 1.7.1
> 
> 
> -- 
> Thanks -
> Manish

> From c9edbd8d1da02ada2615acec3e3083b4669f6d9e Mon Sep 17 00:00:00 2001
> From: Manish Katiyar <mkatiyar@gmail.com>
> Date: Sun, 24 Apr 2011 00:14:54 -0700
> Subject: [PATCH 1/5] 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 else GFP_NOFS is used.
> 
> Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
> ---
>  fs/jbd2/transaction.c |   12 +++++++-----
>  include/linux/jbd2.h  |    4 ++--
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 05fa77a..b02d6a4 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -349,9 +349,10 @@ handle_t *jbd2__journal_start(journal_t *journal, int nblocks, int gfp_mask)
>  EXPORT_SYMBOL(jbd2__journal_start);
>  
>  
> -handle_t *jbd2_journal_start(journal_t *journal, int nblocks)
> +handle_t *jbd2_journal_start(journal_t *journal, int nblocks, bool errok)
>  {
> -	return jbd2__journal_start(journal, nblocks, GFP_NOFS);
> +	return jbd2__journal_start(journal, nblocks,
> +				   errok ? GFP_KERNEL : GFP_NOFS);
>  }
>  EXPORT_SYMBOL(jbd2_journal_start);
>  
> @@ -483,9 +484,10 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, int gfp_mask)
>  EXPORT_SYMBOL(jbd2__journal_restart);
>  
>  
> -int jbd2_journal_restart(handle_t *handle, int nblocks)
> +int jbd2_journal_restart(handle_t *handle, int nblocks, bool errok)
>  {
> -	return jbd2__journal_restart(handle, nblocks, GFP_NOFS);
> +	return jbd2__journal_restart(handle, nblocks,
> +				     errok ? GFP_KERNEL : GFP_NOFS);
>  }
>  EXPORT_SYMBOL(jbd2_journal_restart);
>  
> @@ -1436,7 +1438,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/include/linux/jbd2.h b/include/linux/jbd2.h
> index a32dcae..a64aced 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1103,9 +1103,9 @@ 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, bool errok);
>  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, bool errok);
>  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 *);
> -- 
> 1.7.1
> 

-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

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]

Hi Jan,

Thanks for your patient feedback.

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 ?

> Also your patch series should be bisectable - that means it must compile
> and run after any of the patches. So you cannot really change
> jbd2_journal_start() prototype without changing all the filesystems using
> it. In this case, just include in this patch a simple change for ext4 and
> ocfs2 to pass 'false' in the additional argument.

ok.. Will submit the first patch as everyone passing false as the
errok argument.

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

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

[Jan Kara]

  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.

> > Also your patch series should be bisectable - that means it must compile
> > and run after any of the patches. So you cannot really change
> > jbd2_journal_start() prototype without changing all the filesystems using
> > it. In this case, just include in this patch a simple change for ext4 and
> > ocfs2 to pass 'false' in the additional argument.
> 
> ok.. Will submit the first patch as everyone passing false as the
> errok argument.
  Thanks.

									Honza

-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--
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

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]

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.

Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
---
 fs/ext4/ext4_jbd2.h   |    2 +-
 fs/ext4/super.c       |    3 ++-
 fs/jbd2/transaction.c |   24 +++++-------------------
 fs/ocfs2/journal.c    |    6 +++---
 include/linux/jbd2.h  |    6 ++----
 5 files changed, 13 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..c165ffe 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -279,9 +279,10 @@ 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);
 }

+
 /*
  * The only special thing we need to do here is to make sure that all
  * jbd2_journal_stop calls result in the superblock being marked dirty, so
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



>
>> > Also your patch series should be bisectable - that means it must compile
>> > and run after any of the patches. So you cannot really change
>> > jbd2_journal_start() prototype without changing all the filesystems using
>> > it. In this case, just include in this patch a simple change for ext4 and
>> > ocfs2 to pass 'false' in the additional argument.
>>
>> ok.. Will submit the first patch as everyone passing false as the
>> errok argument.
>  Thanks.
>
>                                                                        Honza
>
> --
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR
>



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

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

[Jan Kara]

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:

> 
> Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
> ---
>  fs/ext4/ext4_jbd2.h   |    2 +-
>  fs/ext4/super.c       |    3 ++-
>  fs/jbd2/transaction.c |   24 +++++-------------------
>  fs/ocfs2/journal.c    |    6 +++---
>  include/linux/jbd2.h  |    6 ++----
>  5 files changed, 13 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 8553dfb..c165ffe 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -279,9 +279,10 @@ 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);
>  }
> 
> +
  This empty line was added by accident.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--
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

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]

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.

Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
---
 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

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

[Jan Kara]

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>
> ---
>  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
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--
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

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]

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

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

[Ted Ts'o]

On Sun, Apr 24, 2011 at 05:10:41PM -0700, 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.
> 
> Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>

Hi Manish,

I really apologize for not having time to follow this patch series.
I've been rather overloaded at the moment.

A couple things.  First of all, when you repost a patch which is part
of patch series, I would really appreciated if you did the following:

*) Resend all of the patches in the patch series, each time.
*) The patches should be in their own mail thread, with either a 0/N
      introductory message which describes what the patch series does
      at a high level --- this is also a good place to put benchmark
      numbers or other high level detail that doesn't belong in the git
      history.  If you don't need a introductory message, then make the
      1/N, 2/N, etc. messages be chained to the first patch in the
      patch series.  This keeps the patch together and easier for
      people to find in their inbox.   If you use git, the commands
      "git format-patch" and "git send-email" will do this for you
      automatically.
*) Please put a short summary of the differences between the
   vN and vN+1 patch after the "---" which separates commit description
   from the rest of the patch.  Maintain this as a change log before the
   diffstat information:
       v4 -> v5   Fix up whitespaces and added reviewed-by: XXXXX
       v3 -> v4   Fixed lock ordering issue pointed out by Eric
       v2 -> v3   Clarified comments in ext4_foobie_bletch()
       v1 -> v2   Pulled out common code and created a helper function,
       	     	     ext4_foobie_bletch()
   (If there is no change in a particular patch; you're just reposting
   because other patches in the patch series changed, that's fine.  Just
   leave the commit log empty for that version, but bump the version
   number so that all of the patches in a reposting of patch series have
   the same version number.)

For a good example of what this might look like, take a look at Amir's
snapshot patches, here:

	 http://thread.gmane.org/gmane.comp.file-systems.ext4/24974

The other thing which I've noticed with these patches is that you made
changes to functions in the jbd2 layer, without also immediately
fixing up all of the callers in the ext4 and ocfs2 file systems.  This
is critically important, because the patches need to be bisectable.
Note what happened just recently with the punch series; it turns out
there was a regression that was introduced between patch 2/5 and 3/5
of that series.  Because the tree was fully buildable and would work
correctly between each patch, this allowed me to use git bisect to
find the problem patch.  If you add an extra parameter to a function,
and then don't fix up the call sites, the kernel won't be buildable
after the first patch.

Finally, try to keep the short description of the commit to less than
72 character.  "jbd2: Pass extra bool parameter in journal routines to
specify if its ok to fail the journal transaction allocation." is just
way too long.

Sorry to dump all of these nit picky things on you at all once, but
because of these issues, it's actually pretty hard to review the rest
of the patches (since it's hard for me to simply find the latest
version of the patches in the mail threads), and I'd have to
completely refactor all of these patches to keep them bisectable, and
that's more work than I'm prepared to take on at this point in the
merge window.  (And in the future, I'm going to be pushing back on
this sort of thing more, just so that I scale better.)

Regards,

						- Ted

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]

On Tue, May 24, 2011 at 3:56 PM, Ted Ts'o <tytso@mit.edu> wrote:
> On Sun, Apr 24, 2011 at 05:10:41PM -0700, 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.
>>
>> Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
>
> Hi Manish,
>
> I really apologize for not having time to follow this patch series.
> I've been rather overloaded at the moment.
>
> A couple things.  First of all, when you repost a patch which is part
> of patch series, I would really appreciated if you did the following:
>
> *) Resend all of the patches in the patch series, each time.
> *) The patches should be in their own mail thread, with either a 0/N
>      introductory message which describes what the patch series does
>      at a high level --- this is also a good place to put benchmark
>      numbers or other high level detail that doesn't belong in the git
>      history.  If you don't need a introductory message, then make the
>      1/N, 2/N, etc. messages be chained to the first patch in the
>      patch series.  This keeps the patch together and easier for
>      people to find in their inbox.   If you use git, the commands
>      "git format-patch" and "git send-email" will do this for you
>      automatically.
> *) Please put a short summary of the differences between the
>   vN and vN+1 patch after the "---" which separates commit description
>   from the rest of the patch.  Maintain this as a change log before the
>   diffstat information:
>       v4 -> v5   Fix up whitespaces and added reviewed-by: XXXXX
>       v3 -> v4   Fixed lock ordering issue pointed out by Eric
>       v2 -> v3   Clarified comments in ext4_foobie_bletch()
>       v1 -> v2   Pulled out common code and created a helper function,
>                     ext4_foobie_bletch()
>   (If there is no change in a particular patch; you're just reposting
>   because other patches in the patch series changed, that's fine.  Just
>   leave the commit log empty for that version, but bump the version
>   number so that all of the patches in a reposting of patch series have
>   the same version number.)
>
> For a good example of what this might look like, take a look at Amir's
> snapshot patches, here:
>
>         http://thread.gmane.org/gmane.comp.file-systems.ext4/24974
>
> The other thing which I've noticed with these patches is that you made
> changes to functions in the jbd2 layer, without also immediately
> fixing up all of the callers in the ext4 and ocfs2 file systems.  This
> is critically important, because the patches need to be bisectable.
> Note what happened just recently with the punch series; it turns out
> there was a regression that was introduced between patch 2/5 and 3/5
> of that series.  Because the tree was fully buildable and would work
> correctly between each patch, this allowed me to use git bisect to
> find the problem patch.  If you add an extra parameter to a function,
> and then don't fix up the call sites, the kernel won't be buildable
> after the first patch.
>
> Finally, try to keep the short description of the commit to less than
> 72 character.  "jbd2: Pass extra bool parameter in journal routines to
> specify if its ok to fail the journal transaction allocation." is just
> way too long.
>
> Sorry to dump all of these nit picky things on you at all once, but
> because of these issues, it's actually pretty hard to review the rest
> of the patches (since it's hard for me to simply find the latest
> version of the patches in the mail threads), and I'd have to
> completely refactor all of these patches to keep them bisectable, and
> that's more work than I'm prepared to take on at this point in the
> merge window.  (And in the future, I'm going to be pushing back on
> this sort of thing more, just so that I scale better.)

Hi Ted,

Thanks for your feedback. I'm still learning how to get my somewhat
useful change into ext4 :-) so will keep these points in mind. Yes,
Jan had also pointed that these patches need to be bisectable, so
actually the last version of the patch in the thread should be
complete in itself. But if you say, I can resend the two patches which
have been reviewed and ack'd by Jan in a new separate mail thread with
appropriate changelog and you can ignore these then. Is that ok ?

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

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

[Ted Ts'o]

On Tue, May 24, 2011 at 05:14:22PM -0700, Manish Katiyar wrote:
> Thanks for your feedback. I'm still learning how to get my somewhat
> useful change into ext4 :-) so will keep these points in mind. Yes,
> Jan had also pointed that these patches need to be bisectable, so
> actually the last version of the patch in the thread should be
> complete in itself. But if you say, I can resend the two patches which
> have been reviewed and ack'd by Jan in a new separate mail thread with
> appropriate changelog and you can ignore these then. Is that ok ?

Yes please.  Since that will cause the other patches to get
renumbered, it's best if you resend them all as a separate mail
thread.

							- Ted