On Wed, Jun 8, 2011 at 9:49 AM, Jan Kara <jack@xxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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.
--
Thanks -
Manish
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
