Re: [PATCH] JBD: use GFP_NOFS in kmalloc

[Mingming Cao <cmm@us.ibm.com>]

On Wed, 2007-09-19 at 14:34 -0700, Andrew Morton wrote:
> On Wed, 19 Sep 2007 12:22:09 -0700
> Mingming Cao <cmm@us.ibm.com> wrote:
> 
> > Convert the GFP_KERNEL flag used in JBD/JBD2 to GFP_NOFS, consistent
> > with the rest of kmalloc flag used in the JBD/JBD2 layer.
> > 
> > Signed-off-by: Mingming Cao <cmm@us.ibm.com>
> > 
> > ---
> >  fs/jbd/journal.c  |    6 +++---
> >  fs/jbd/revoke.c   |    8 ++++----
> >  fs/jbd2/journal.c |    6 +++---
> >  fs/jbd2/revoke.c  |    8 ++++----
> >  4 files changed, 14 insertions(+), 14 deletions(-)
> > 
> > Index: linux-2.6.23-rc6/fs/jbd/journal.c
> > ===================================================================
> > --- linux-2.6.23-rc6.orig/fs/jbd/journal.c	2007-09-19 11:51:10.000000000 -0700
> > +++ linux-2.6.23-rc6/fs/jbd/journal.c	2007-09-19 11:51:57.000000000 -0700
> > @@ -653,7 +653,7 @@ static journal_t * journal_init_common (
> >  	journal_t *journal;
> >  	int err;
> >  
> > -	journal = kmalloc(sizeof(*journal), GFP_KERNEL);
> > +	journal = kmalloc(sizeof(*journal), GFP_NOFS);
> >  	if (!journal)
> >  		goto fail;
> >  	memset(journal, 0, sizeof(*journal));
> > @@ -723,7 +723,7 @@ journal_t * journal_init_dev(struct bloc
> >  	journal->j_blocksize = blocksize;
> >  	n = journal->j_blocksize / sizeof(journal_block_tag_t);
> >  	journal->j_wbufsize = n;
> > -	journal->j_wbuf = kmalloc(n * sizeof(struct buffer_head*), GFP_KERNEL);
> > +	journal->j_wbuf = kmalloc(n * sizeof(struct buffer_head*), GFP_NOFS);
> >  	if (!journal->j_wbuf) {
> >  		printk(KERN_ERR "%s: Cant allocate bhs for commit thread\n",
> >  			__FUNCTION__);
> > @@ -777,7 +777,7 @@ journal_t * journal_init_inode (struct i
> >  	/* journal descriptor can store up to n blocks -bzzz */
> >  	n = journal->j_blocksize / sizeof(journal_block_tag_t);
> >  	journal->j_wbufsize = n;
> > -	journal->j_wbuf = kmalloc(n * sizeof(struct buffer_head*), GFP_KERNEL);
> > +	journal->j_wbuf = kmalloc(n * sizeof(struct buffer_head*), GFP_NOFS);
> >  	if (!journal->j_wbuf) {
> >  		printk(KERN_ERR "%s: Cant allocate bhs for commit thread\n",
> >  			__FUNCTION__);
> > Index: linux-2.6.23-rc6/fs/jbd/revoke.c
> > ===================================================================
> > --- linux-2.6.23-rc6.orig/fs/jbd/revoke.c	2007-09-19 11:51:30.000000000 -0700
> > +++ linux-2.6.23-rc6/fs/jbd/revoke.c	2007-09-19 11:52:34.000000000 -0700
> > @@ -206,7 +206,7 @@ int journal_init_revoke(journal_t *journ
> >  	while((tmp >>= 1UL) != 0UL)
> >  		shift++;
> >  
> > -	journal->j_revoke_table[0] = kmem_cache_alloc(revoke_table_cache, GFP_KERNEL);
> > +	journal->j_revoke_table[0] = kmem_cache_alloc(revoke_table_cache, GFP_NOFS);
> >  	if (!journal->j_revoke_table[0])
> >  		return -ENOMEM;
> >  	journal->j_revoke = journal->j_revoke_table[0];
> > @@ -219,7 +219,7 @@ int journal_init_revoke(journal_t *journ
> >  	journal->j_revoke->hash_shift = shift;
> >  
> >  	journal->j_revoke->hash_table =
> > -		kmalloc(hash_size * sizeof(struct list_head), GFP_KERNEL);
> > +		kmalloc(hash_size * sizeof(struct list_head), GFP_NOFS);
> >  	if (!journal->j_revoke->hash_table) {
> >  		kmem_cache_free(revoke_table_cache, journal->j_revoke_table[0]);
> >  		journal->j_revoke = NULL;
> > @@ -229,7 +229,7 @@ int journal_init_revoke(journal_t *journ
> >  	for (tmp = 0; tmp < hash_size; tmp++)
> >  		INIT_LIST_HEAD(&journal->j_revoke->hash_table[tmp]);
> >  
> > -	journal->j_revoke_table[1] = kmem_cache_alloc(revoke_table_cache, GFP_KERNEL);
> > +	journal->j_revoke_table[1] = kmem_cache_alloc(revoke_table_cache, GFP_NOFS);
> >  	if (!journal->j_revoke_table[1]) {
> >  		kfree(journal->j_revoke_table[0]->hash_table);
> >  		kmem_cache_free(revoke_table_cache, journal->j_revoke_table[0]);
> > @@ -246,7 +246,7 @@ int journal_init_revoke(journal_t *journ
> >  	journal->j_revoke->hash_shift = shift;
> >  
> >  	journal->j_revoke->hash_table =
> > -		kmalloc(hash_size * sizeof(struct list_head), GFP_KERNEL);
> > +		kmalloc(hash_size * sizeof(struct list_head), GFP_NOFS);
> >  	if (!journal->j_revoke->hash_table) {
> >  		kfree(journal->j_revoke_table[0]->hash_table);
> >  		kmem_cache_free(revoke_table_cache, journal->j_revoke_table[0]);
> 
> These were all OK using GFP_KERNEL.
> 
> GFP_NOFS should only be used when the caller is holding some fs locks which
> might cause a deadlock if that caller reentered the fs in ->writepage (and
> maybe put_inode and such).  That isn't the case in any of the above code,
> which is all mount time stuff (I think).
> 

You are right they are all occur at initialization time.

> ext3/4 should be using GFP_NOFS when the caller has a transaction open, has
> a page locked, is holding i_mutex, etc.
> 

Thanks for your feedback.

Mingming