Patchwork jbd2:Make journal transaction allocations come from its own cache.

login
register
mail settings
Submitter Manish Katiyar
Date June 5, 2011, 8:28 a.m.
Message ID <1307262510-16376-1-git-send-email-mkatiyar@gmail.com>
Download mbox | patch
Permalink /patch/98753/
State Superseded
Headers show

Comments

Manish Katiyar - June 5, 2011, 8:28 a.m.
Add a cache for jbd2 journal transaction allocations. This also
helps to leverage fault-injection framework to test various memory
allocation failures in the jbd2 layer.

Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
---
 fs/jbd2/checkpoint.c  |    2 +-
 fs/jbd2/commit.c      |    2 +-
 fs/jbd2/journal.c     |   25 +++++++++++++++++++++++++
 fs/jbd2/transaction.c |    7 ++++---
 include/linux/jbd2.h  |   21 +++++++++++++++++++++
 5 files changed, 52 insertions(+), 5 deletions(-)
Jan Kara - June 8, 2011, 4:49 p.m.
On Sun 05-06-11 01:28:30, Manish Katiyar wrote:
> Add a cache for jbd2 journal transaction allocations. This also
> helps to leverage fault-injection framework to test various memory
> allocation failures in the jbd2 layer.
> 
> Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
> ---
>  fs/jbd2/checkpoint.c  |    2 +-
>  fs/jbd2/commit.c      |    2 +-
>  fs/jbd2/journal.c     |   25 +++++++++++++++++++++++++
>  fs/jbd2/transaction.c |    7 ++++---
>  include/linux/jbd2.h  |   21 +++++++++++++++++++++
>  5 files changed, 52 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> index 6a79fd0..6f554ce 100644
> --- a/fs/jbd2/checkpoint.c
> +++ b/fs/jbd2/checkpoint.c
> @@ -716,7 +716,7 @@ int __jbd2_journal_remove_checkpoint(struct journal_head *jh)
>  				    transaction->t_tid, stats);
>  
>  	__jbd2_journal_drop_transaction(journal, transaction);
> -	kfree(transaction);
> +	jbd2_free_transaction(transaction);
>  
>  	/* Just in case anybody was waiting for more transactions to be
>             checkpointed... */
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 7f21cf3..8e33d84 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -1037,7 +1037,7 @@ restart_loop:
>  	jbd_debug(1, "JBD: commit %d complete, head %d\n",
>  		  journal->j_commit_sequence, journal->j_tail_sequence);
>  	if (to_free)
> -		kfree(commit_transaction);
> +		jbd2_free_transaction(commit_transaction);
>  
>  	wake_up(&journal->j_wait_done_commit);
>  }
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 9a78269..c0ec463 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -95,6 +95,7 @@ EXPORT_SYMBOL(jbd2_journal_init_jbd_inode);
>  EXPORT_SYMBOL(jbd2_journal_release_jbd_inode);
>  EXPORT_SYMBOL(jbd2_journal_begin_ordered_truncate);
>  EXPORT_SYMBOL(jbd2_inode_cache);
> +EXPORT_SYMBOL(jbd2_transaction_cache);
>  
>  static int journal_convert_superblock_v1(journal_t *, journal_superblock_t *);
>  static void __journal_abort_soft (journal_t *journal, int errno);
> @@ -2371,6 +2372,27 @@ static void jbd2_journal_destroy_handle_cache(void)
>  
>  }
>  
> +struct kmem_cache *jbd2_transaction_cache;
> +
> +static int journal_init_transaction_cache(void)
> +{
> +	J_ASSERT(jbd2_transaction_cache == NULL);
> +	jbd2_transaction_cache = kmem_cache_create("jbd2_transaction",
> +						   sizeof(transaction_t),
> +						   0, SLAB_TEMPORARY, NULL);
  Transactions are not really short-lived in the memory-management sense I
think. They usually live for seconds while I'd understand 'short-lived' to
mean a milisecond or less. So just drop this flag (it doesn't do anything
these days anyway).

> +	if (jbd2_transaction_cache == NULL) {
> +		printk(KERN_EMERG "JBD2: failed to create transaction cache\n");
> +		return -ENOMEM;
> +	}
> +	return 0;
> +}
> +
> +static void jbd2_journal_destroy_transaction_cache(void)
> +{
> +	if (jbd2_transaction_cache)
  How can this happen?

> +		kmem_cache_destroy(jbd2_transaction_cache);
> +}
> +
>  /*
>   * Module startup and shutdown
>   */
> @@ -2384,6 +2406,8 @@ static int __init journal_init_caches(void)
>  		ret = journal_init_jbd2_journal_head_cache();
>  	if (ret == 0)
>  		ret = journal_init_handle_cache();
> +	if (ret == 0)
> +		ret = journal_init_transaction_cache();
>  	return ret;
>  }
>  
> @@ -2392,6 +2416,7 @@ static void jbd2_journal_destroy_caches(void)
>  	jbd2_journal_destroy_revoke_caches();
>  	jbd2_journal_destroy_jbd2_journal_head_cache();
>  	jbd2_journal_destroy_handle_cache();
> +	jbd2_journal_destroy_transaction_cache();
>  	jbd2_journal_destroy_slabs();
>  }
>  
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 3eec82d..58fbec7 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -131,7 +131,7 @@ static int start_this_handle(journal_t *journal, handle_t *handle,
>  
>  alloc_transaction:
>  	if (!journal->j_running_transaction) {
> -		new_transaction = kzalloc(sizeof(*new_transaction), gfp_mask);
> +		new_transaction = jbd2_alloc_transaction(gfp_mask);
>  		if (!new_transaction) {
>  			/*
>  			 * If __GFP_FS is not present, then we may be
> @@ -160,7 +160,7 @@ repeat:
>  	if (is_journal_aborted(journal) ||
>  	    (journal->j_errno != 0 && !(journal->j_flags & JBD2_ACK_ERR))) {
>  		read_unlock(&journal->j_state_lock);
> -		kfree(new_transaction);
> +		jbd2_free_transaction(new_transaction);
>  		return -EROFS;
>  	}
>  
> @@ -282,7 +282,8 @@ repeat:
>  	read_unlock(&journal->j_state_lock);
>  
>  	lock_map_acquire(&handle->h_lockdep_map);
> -	kfree(new_transaction);
> +	jbd2_free_transaction(new_transaction);
> +
>  	return 0;
>  }
>  
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 4ecb7b1..df7f101 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1184,6 +1184,27 @@ static inline void jbd2_free_handle(handle_t *handle)
>  }
>  
>  /*
> + * transaction management
> + */
> +extern struct kmem_cache *jbd2_transaction_cache;
> +
> +static inline transaction_t *jbd2_alloc_transaction(gfp_t gfp_flags)
> +{
> +	return kmem_cache_zalloc(jbd2_transaction_cache, gfp_flags);
> +}
> +
> +static inline void jbd2_free_transaction(transaction_t *transaction)
> +{
> +	if (transaction) {
> +#ifdef CONFIG_JBD2_DEBUG
> +		memset(transaction, JBD2_POISON_FREE, sizeof(*transaction));
> +#endif
  No need for this - CONFIG_DEBUG_SLAB does this for you...

> +		kmem_cache_free(jbd2_transaction_cache, transaction);
> +	}
> +}
> +
> +
> +/*
>   * jbd2_inode management (optional, for those file systems that want to use
>   * dynamically allocated jbd2_inode structures)
>   */
  Otherwise the patch looks OK to me.

								Honza
Manish Katiyar - June 9, 2011, 6:35 a.m.
On Wed, Jun 8, 2011 at 9:49 AM, Jan Kara <jack@suse.cz> wrote:
> On Sun 05-06-11 01:28:30, Manish Katiyar wrote:
>> Add a cache for jbd2 journal transaction allocations. This also
>> helps to leverage fault-injection framework to test various memory
>> allocation failures in the jbd2 layer.
>>
>> Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
>> ---
>>  fs/jbd2/checkpoint.c  |    2 +-
>>  fs/jbd2/commit.c      |    2 +-
>>  fs/jbd2/journal.c     |   25 +++++++++++++++++++++++++
>>  fs/jbd2/transaction.c |    7 ++++---
>>  include/linux/jbd2.h  |   21 +++++++++++++++++++++
>>  5 files changed, 52 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
>> index 6a79fd0..6f554ce 100644
>> --- a/fs/jbd2/checkpoint.c
>> +++ b/fs/jbd2/checkpoint.c
>> @@ -716,7 +716,7 @@ int __jbd2_journal_remove_checkpoint(struct journal_head *jh)
>>                                   transaction->t_tid, stats);
>>
>>       __jbd2_journal_drop_transaction(journal, transaction);
>> -     kfree(transaction);
>> +     jbd2_free_transaction(transaction);
>>
>>       /* Just in case anybody was waiting for more transactions to be
>>             checkpointed... */
>> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
>> index 7f21cf3..8e33d84 100644
>> --- a/fs/jbd2/commit.c
>> +++ b/fs/jbd2/commit.c
>> @@ -1037,7 +1037,7 @@ restart_loop:
>>       jbd_debug(1, "JBD: commit %d complete, head %d\n",
>>                 journal->j_commit_sequence, journal->j_tail_sequence);
>>       if (to_free)
>> -             kfree(commit_transaction);
>> +             jbd2_free_transaction(commit_transaction);
>>
>>       wake_up(&journal->j_wait_done_commit);
>>  }
>> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
>> index 9a78269..c0ec463 100644
>> --- a/fs/jbd2/journal.c
>> +++ b/fs/jbd2/journal.c
>> @@ -95,6 +95,7 @@ EXPORT_SYMBOL(jbd2_journal_init_jbd_inode);
>>  EXPORT_SYMBOL(jbd2_journal_release_jbd_inode);
>>  EXPORT_SYMBOL(jbd2_journal_begin_ordered_truncate);
>>  EXPORT_SYMBOL(jbd2_inode_cache);
>> +EXPORT_SYMBOL(jbd2_transaction_cache);
>>
>>  static int journal_convert_superblock_v1(journal_t *, journal_superblock_t *);
>>  static void __journal_abort_soft (journal_t *journal, int errno);
>> @@ -2371,6 +2372,27 @@ static void jbd2_journal_destroy_handle_cache(void)
>>
>>  }
>>
>> +struct kmem_cache *jbd2_transaction_cache;
>> +
>> +static int journal_init_transaction_cache(void)
>> +{
>> +     J_ASSERT(jbd2_transaction_cache == NULL);
>> +     jbd2_transaction_cache = kmem_cache_create("jbd2_transaction",
>> +                                                sizeof(transaction_t),
>> +                                                0, SLAB_TEMPORARY, NULL);
>  Transactions are not really short-lived in the memory-management sense I
> think. They usually live for seconds while I'd understand 'short-lived' to
> mean a milisecond or less. So just drop this flag (it doesn't do anything
> these days anyway).
>
>> +     if (jbd2_transaction_cache == NULL) {
>> +             printk(KERN_EMERG "JBD2: failed to create transaction cache\n");
>> +             return -ENOMEM;
>> +     }
>> +     return 0;
>> +}
>> +
>> +static void jbd2_journal_destroy_transaction_cache(void)
>> +{
>> +     if (jbd2_transaction_cache)
>  How can this happen?

Hi Jan,

In start_this_handle() we can pass a NULL value here. Since we had
kfree() earlier which could silently handle NULL, either I can check
before calling or handle it here. To verify I removed the if condition
and it immediately panic'd on boot.

(gdb) do
#20 start_this_handle (journal=0x17cd7c00, handle=0x1790e000,
gfp_mask=80) at fs/jbd2/transaction.c:285
285		jbd2_free_transaction(new_transaction);
(gdb) l
280			  atomic_read(&transaction->t_outstanding_credits),
281			  __jbd2_log_space_left(journal));
282		read_unlock(&journal->j_state_lock);
283	
284		lock_map_acquire(&handle->h_lockdep_map);
285		jbd2_free_transaction(new_transaction);
286	
287		return 0;
288	}
289	
(gdb) p new_transaction
$1 = (transaction_t *) 0x0


I'll send a patch shortly with other comments incorporated.
Manish Katiyar - June 9, 2011, 7:20 a.m.
On Wed, Jun 8, 2011 at 11:35 PM, Manish Katiyar <mkatiyar@gmail.com> wrote:
> On Wed, Jun 8, 2011 at 9:49 AM, Jan Kara <jack@suse.cz> wrote:
>> On Sun 05-06-11 01:28:30, Manish Katiyar wrote:
>>> Add a cache for jbd2 journal transaction allocations. This also
>>> helps to leverage fault-injection framework to test various memory
>>> allocation failures in the jbd2 layer.
>>>
>>> Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
>>> ---
>>>  fs/jbd2/checkpoint.c  |    2 +-
>>>  fs/jbd2/commit.c      |    2 +-
>>>  fs/jbd2/journal.c     |   25 +++++++++++++++++++++++++
>>>  fs/jbd2/transaction.c |    7 ++++---
>>>  include/linux/jbd2.h  |   21 +++++++++++++++++++++
>>>  5 files changed, 52 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
>>> index 6a79fd0..6f554ce 100644
>>> --- a/fs/jbd2/checkpoint.c
>>> +++ b/fs/jbd2/checkpoint.c
>>> @@ -716,7 +716,7 @@ int __jbd2_journal_remove_checkpoint(struct journal_head *jh)
>>>                                   transaction->t_tid, stats);
>>>
>>>       __jbd2_journal_drop_transaction(journal, transaction);
>>> -     kfree(transaction);
>>> +     jbd2_free_transaction(transaction);
>>>
>>>       /* Just in case anybody was waiting for more transactions to be
>>>             checkpointed... */
>>> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
>>> index 7f21cf3..8e33d84 100644
>>> --- a/fs/jbd2/commit.c
>>> +++ b/fs/jbd2/commit.c
>>> @@ -1037,7 +1037,7 @@ restart_loop:
>>>       jbd_debug(1, "JBD: commit %d complete, head %d\n",
>>>                 journal->j_commit_sequence, journal->j_tail_sequence);
>>>       if (to_free)
>>> -             kfree(commit_transaction);
>>> +             jbd2_free_transaction(commit_transaction);
>>>
>>>       wake_up(&journal->j_wait_done_commit);
>>>  }
>>> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
>>> index 9a78269..c0ec463 100644
>>> --- a/fs/jbd2/journal.c
>>> +++ b/fs/jbd2/journal.c
>>> @@ -95,6 +95,7 @@ EXPORT_SYMBOL(jbd2_journal_init_jbd_inode);
>>>  EXPORT_SYMBOL(jbd2_journal_release_jbd_inode);
>>>  EXPORT_SYMBOL(jbd2_journal_begin_ordered_truncate);
>>>  EXPORT_SYMBOL(jbd2_inode_cache);
>>> +EXPORT_SYMBOL(jbd2_transaction_cache);
>>>
>>>  static int journal_convert_superblock_v1(journal_t *, journal_superblock_t *);
>>>  static void __journal_abort_soft (journal_t *journal, int errno);
>>> @@ -2371,6 +2372,27 @@ static void jbd2_journal_destroy_handle_cache(void)
>>>
>>>  }
>>>
>>> +struct kmem_cache *jbd2_transaction_cache;
>>> +
>>> +static int journal_init_transaction_cache(void)
>>> +{
>>> +     J_ASSERT(jbd2_transaction_cache == NULL);
>>> +     jbd2_transaction_cache = kmem_cache_create("jbd2_transaction",
>>> +                                                sizeof(transaction_t),
>>> +                                                0, SLAB_TEMPORARY, NULL);
>>  Transactions are not really short-lived in the memory-management sense I
>> think. They usually live for seconds while I'd understand 'short-lived' to
>> mean a milisecond or less. So just drop this flag (it doesn't do anything
>> these days anyway).
>>
>>> +     if (jbd2_transaction_cache == NULL) {
>>> +             printk(KERN_EMERG "JBD2: failed to create transaction cache\n");
>>> +             return -ENOMEM;
>>> +     }
>>> +     return 0;
>>> +}
>>> +
>>> +static void jbd2_journal_destroy_transaction_cache(void)
>>> +{
>>> +     if (jbd2_transaction_cache)
>>  How can this happen?
>
> Hi Jan,
>
> In start_this_handle() we can pass a NULL value here. Since we had
> kfree() earlier which could silently handle NULL, either I can check
> before calling or handle it here. To verify I removed the if condition
> and it immediately panic'd on boot.
>
> (gdb) do
> #20 start_this_handle (journal=0x17cd7c00, handle=0x1790e000,
> gfp_mask=80) at fs/jbd2/transaction.c:285
> 285             jbd2_free_transaction(new_transaction);
> (gdb) l
> 280                       atomic_read(&transaction->t_outstanding_credits),
> 281                       __jbd2_log_space_left(journal));
> 282             read_unlock(&journal->j_state_lock);
> 283
> 284             lock_map_acquire(&handle->h_lockdep_map);
> 285             jbd2_free_transaction(new_transaction);
> 286
> 287             return 0;
> 288     }
> 289
> (gdb) p new_transaction
> $1 = (transaction_t *) 0x0
>
>
> I'll send a patch shortly with other comments incorporated.

Sorry, I misread your comment. You are right...this should never happen.
Manish Katiyar - June 10, 2011, 6:21 a.m.
On Wed, Jun 8, 2011 at 9:49 AM, Jan Kara <jack@suse.cz> wrote:
> On Sun 05-06-11 01:28:30, Manish Katiyar wrote:
>> Add a cache for jbd2 journal transaction allocations. This also
>> helps to leverage fault-injection framework to test various memory
>> allocation failures in the jbd2 layer.
>>
>> Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
>> ---
>>  fs/jbd2/checkpoint.c  |    2 +-
>>  fs/jbd2/commit.c      |    2 +-
>>  fs/jbd2/journal.c     |   25 +++++++++++++++++++++++++
>>  fs/jbd2/transaction.c |    7 ++++---
>>  include/linux/jbd2.h  |   21 +++++++++++++++++++++
>>  5 files changed, 52 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
>> index 6a79fd0..6f554ce 100644
>> --- a/fs/jbd2/checkpoint.c
>> +++ b/fs/jbd2/checkpoint.c
>> @@ -716,7 +716,7 @@ int __jbd2_journal_remove_checkpoint(struct journal_head *jh)
>>                                   transaction->t_tid, stats);
>>
>>       __jbd2_journal_drop_transaction(journal, transaction);
>> -     kfree(transaction);
>> +     jbd2_free_transaction(transaction);
>>
>>       /* Just in case anybody was waiting for more transactions to be
>>             checkpointed... */
>> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
>> index 7f21cf3..8e33d84 100644
>> --- a/fs/jbd2/commit.c
>> +++ b/fs/jbd2/commit.c
>> @@ -1037,7 +1037,7 @@ restart_loop:
>>       jbd_debug(1, "JBD: commit %d complete, head %d\n",
>>                 journal->j_commit_sequence, journal->j_tail_sequence);
>>       if (to_free)
>> -             kfree(commit_transaction);
>> +             jbd2_free_transaction(commit_transaction);
>>
>>       wake_up(&journal->j_wait_done_commit);
>>  }
>> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
>> index 9a78269..c0ec463 100644
>> --- a/fs/jbd2/journal.c
>> +++ b/fs/jbd2/journal.c
>> @@ -95,6 +95,7 @@ EXPORT_SYMBOL(jbd2_journal_init_jbd_inode);
>>  EXPORT_SYMBOL(jbd2_journal_release_jbd_inode);
>>  EXPORT_SYMBOL(jbd2_journal_begin_ordered_truncate);
>>  EXPORT_SYMBOL(jbd2_inode_cache);
>> +EXPORT_SYMBOL(jbd2_transaction_cache);
>>
>>  static int journal_convert_superblock_v1(journal_t *, journal_superblock_t *);
>>  static void __journal_abort_soft (journal_t *journal, int errno);
>> @@ -2371,6 +2372,27 @@ static void jbd2_journal_destroy_handle_cache(void)
>>
>>  }
>>
>> +struct kmem_cache *jbd2_transaction_cache;
>> +
>> +static int journal_init_transaction_cache(void)
>> +{
>> +     J_ASSERT(jbd2_transaction_cache == NULL);
>> +     jbd2_transaction_cache = kmem_cache_create("jbd2_transaction",
>> +                                                sizeof(transaction_t),
>> +                                                0, SLAB_TEMPORARY, NULL);
>  Transactions are not really short-lived in the memory-management sense I
> think. They usually live for seconds while I'd understand 'short-lived' to
> mean a milisecond or less. So just drop this flag (it doesn't do anything
> these days anyway).
>
>> +     if (jbd2_transaction_cache == NULL) {
>> +             printk(KERN_EMERG "JBD2: failed to create transaction cache\n");
>> +             return -ENOMEM;
>> +     }
>> +     return 0;
>> +}
>> +
>> +static void jbd2_journal_destroy_transaction_cache(void)
>> +{
>> +     if (jbd2_transaction_cache)
>  How can this happen?

Hi Jan,

re-reading the code, I think its possible and we should be checking
for it. Since in journal_init_caches() if any of the initialization
fails we would call jbd2_journal_destroy_caches() on all the caches.
So we should be destroying it only if its initialized.

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
Jan Kara - June 13, 2011, 1:31 p.m.
On Thu 09-06-11 23:21:17, Manish Katiyar wrote:
> On Wed, Jun 8, 2011 at 9:49 AM, Jan Kara <jack@suse.cz> wrote:
> > On Sun 05-06-11 01:28:30, Manish Katiyar wrote:
> >> Add a cache for jbd2 journal transaction allocations. This also
> >> helps to leverage fault-injection framework to test various memory
> >> allocation failures in the jbd2 layer.
> >>
> >> Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
> >> ---
> >>  fs/jbd2/checkpoint.c  |    2 +-
> >>  fs/jbd2/commit.c      |    2 +-
> >>  fs/jbd2/journal.c     |   25 +++++++++++++++++++++++++
> >>  fs/jbd2/transaction.c |    7 ++++---
> >>  include/linux/jbd2.h  |   21 +++++++++++++++++++++
> >>  5 files changed, 52 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> >> index 6a79fd0..6f554ce 100644
> >> --- a/fs/jbd2/checkpoint.c
> >> +++ b/fs/jbd2/checkpoint.c
> >> @@ -716,7 +716,7 @@ int __jbd2_journal_remove_checkpoint(struct journal_head *jh)
> >>                                   transaction->t_tid, stats);
> >>
> >>       __jbd2_journal_drop_transaction(journal, transaction);
> >> -     kfree(transaction);
> >> +     jbd2_free_transaction(transaction);
> >>
> >>       /* Just in case anybody was waiting for more transactions to be
> >>             checkpointed... */
> >> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> >> index 7f21cf3..8e33d84 100644
> >> --- a/fs/jbd2/commit.c
> >> +++ b/fs/jbd2/commit.c
> >> @@ -1037,7 +1037,7 @@ restart_loop:
> >>       jbd_debug(1, "JBD: commit %d complete, head %d\n",
> >>                 journal->j_commit_sequence, journal->j_tail_sequence);
> >>       if (to_free)
> >> -             kfree(commit_transaction);
> >> +             jbd2_free_transaction(commit_transaction);
> >>
> >>       wake_up(&journal->j_wait_done_commit);
> >>  }
> >> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> >> index 9a78269..c0ec463 100644
> >> --- a/fs/jbd2/journal.c
> >> +++ b/fs/jbd2/journal.c
> >> @@ -95,6 +95,7 @@ EXPORT_SYMBOL(jbd2_journal_init_jbd_inode);
> >>  EXPORT_SYMBOL(jbd2_journal_release_jbd_inode);
> >>  EXPORT_SYMBOL(jbd2_journal_begin_ordered_truncate);
> >>  EXPORT_SYMBOL(jbd2_inode_cache);
> >> +EXPORT_SYMBOL(jbd2_transaction_cache);
> >>
> >>  static int journal_convert_superblock_v1(journal_t *, journal_superblock_t *);
> >>  static void __journal_abort_soft (journal_t *journal, int errno);
> >> @@ -2371,6 +2372,27 @@ static void jbd2_journal_destroy_handle_cache(void)
> >>
> >>  }
> >>
> >> +struct kmem_cache *jbd2_transaction_cache;
> >> +
> >> +static int journal_init_transaction_cache(void)
> >> +{
> >> +     J_ASSERT(jbd2_transaction_cache == NULL);
> >> +     jbd2_transaction_cache = kmem_cache_create("jbd2_transaction",
> >> +                                                sizeof(transaction_t),
> >> +                                                0, SLAB_TEMPORARY, NULL);
> >  Transactions are not really short-lived in the memory-management sense I
> > think. They usually live for seconds while I'd understand 'short-lived' to
> > mean a milisecond or less. So just drop this flag (it doesn't do anything
> > these days anyway).
> >
> >> +     if (jbd2_transaction_cache == NULL) {
> >> +             printk(KERN_EMERG "JBD2: failed to create transaction cache\n");
> >> +             return -ENOMEM;
> >> +     }
> >> +     return 0;
> >> +}
> >> +
> >> +static void jbd2_journal_destroy_transaction_cache(void)
> >> +{
> >> +     if (jbd2_transaction_cache)
> >  How can this happen?
> 
> Hi Jan,
> 
> re-reading the code, I think its possible and we should be checking
> for it. Since in journal_init_caches() if any of the initialization
> fails we would call jbd2_journal_destroy_caches() on all the caches.
> So we should be destroying it only if its initialized.
  Ah, good point. Thanks for checking this.

								Honza

Patch

diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
index 6a79fd0..6f554ce 100644
--- a/fs/jbd2/checkpoint.c
+++ b/fs/jbd2/checkpoint.c
@@ -716,7 +716,7 @@  int __jbd2_journal_remove_checkpoint(struct journal_head *jh)
 				    transaction->t_tid, stats);
 
 	__jbd2_journal_drop_transaction(journal, transaction);
-	kfree(transaction);
+	jbd2_free_transaction(transaction);
 
 	/* Just in case anybody was waiting for more transactions to be
            checkpointed... */
diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 7f21cf3..8e33d84 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -1037,7 +1037,7 @@  restart_loop:
 	jbd_debug(1, "JBD: commit %d complete, head %d\n",
 		  journal->j_commit_sequence, journal->j_tail_sequence);
 	if (to_free)
-		kfree(commit_transaction);
+		jbd2_free_transaction(commit_transaction);
 
 	wake_up(&journal->j_wait_done_commit);
 }
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 9a78269..c0ec463 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -95,6 +95,7 @@  EXPORT_SYMBOL(jbd2_journal_init_jbd_inode);
 EXPORT_SYMBOL(jbd2_journal_release_jbd_inode);
 EXPORT_SYMBOL(jbd2_journal_begin_ordered_truncate);
 EXPORT_SYMBOL(jbd2_inode_cache);
+EXPORT_SYMBOL(jbd2_transaction_cache);
 
 static int journal_convert_superblock_v1(journal_t *, journal_superblock_t *);
 static void __journal_abort_soft (journal_t *journal, int errno);
@@ -2371,6 +2372,27 @@  static void jbd2_journal_destroy_handle_cache(void)
 
 }
 
+struct kmem_cache *jbd2_transaction_cache;
+
+static int journal_init_transaction_cache(void)
+{
+	J_ASSERT(jbd2_transaction_cache == NULL);
+	jbd2_transaction_cache = kmem_cache_create("jbd2_transaction",
+						   sizeof(transaction_t),
+						   0, SLAB_TEMPORARY, NULL);
+	if (jbd2_transaction_cache == NULL) {
+		printk(KERN_EMERG "JBD2: failed to create transaction cache\n");
+		return -ENOMEM;
+	}
+	return 0;
+}
+
+static void jbd2_journal_destroy_transaction_cache(void)
+{
+	if (jbd2_transaction_cache)
+		kmem_cache_destroy(jbd2_transaction_cache);
+}
+
 /*
  * Module startup and shutdown
  */
@@ -2384,6 +2406,8 @@  static int __init journal_init_caches(void)
 		ret = journal_init_jbd2_journal_head_cache();
 	if (ret == 0)
 		ret = journal_init_handle_cache();
+	if (ret == 0)
+		ret = journal_init_transaction_cache();
 	return ret;
 }
 
@@ -2392,6 +2416,7 @@  static void jbd2_journal_destroy_caches(void)
 	jbd2_journal_destroy_revoke_caches();
 	jbd2_journal_destroy_jbd2_journal_head_cache();
 	jbd2_journal_destroy_handle_cache();
+	jbd2_journal_destroy_transaction_cache();
 	jbd2_journal_destroy_slabs();
 }
 
diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 3eec82d..58fbec7 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -131,7 +131,7 @@  static int start_this_handle(journal_t *journal, handle_t *handle,
 
 alloc_transaction:
 	if (!journal->j_running_transaction) {
-		new_transaction = kzalloc(sizeof(*new_transaction), gfp_mask);
+		new_transaction = jbd2_alloc_transaction(gfp_mask);
 		if (!new_transaction) {
 			/*
 			 * If __GFP_FS is not present, then we may be
@@ -160,7 +160,7 @@  repeat:
 	if (is_journal_aborted(journal) ||
 	    (journal->j_errno != 0 && !(journal->j_flags & JBD2_ACK_ERR))) {
 		read_unlock(&journal->j_state_lock);
-		kfree(new_transaction);
+		jbd2_free_transaction(new_transaction);
 		return -EROFS;
 	}
 
@@ -282,7 +282,8 @@  repeat:
 	read_unlock(&journal->j_state_lock);
 
 	lock_map_acquire(&handle->h_lockdep_map);
-	kfree(new_transaction);
+	jbd2_free_transaction(new_transaction);
+
 	return 0;
 }
 
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 4ecb7b1..df7f101 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1184,6 +1184,27 @@  static inline void jbd2_free_handle(handle_t *handle)
 }
 
 /*
+ * transaction management
+ */
+extern struct kmem_cache *jbd2_transaction_cache;
+
+static inline transaction_t *jbd2_alloc_transaction(gfp_t gfp_flags)
+{
+	return kmem_cache_zalloc(jbd2_transaction_cache, gfp_flags);
+}
+
+static inline void jbd2_free_transaction(transaction_t *transaction)
+{
+	if (transaction) {
+#ifdef CONFIG_JBD2_DEBUG
+		memset(transaction, JBD2_POISON_FREE, sizeof(*transaction));
+#endif
+		kmem_cache_free(jbd2_transaction_cache, transaction);
+	}
+}
+
+
+/*
  * jbd2_inode management (optional, for those file systems that want to use
  * dynamically allocated jbd2_inode structures)
  */