Re: [PATCH] jbd2:Make journal transaction allocations come from its own cache.

[Manish Katiyar <mkatiyar@gmail.com>]

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