On Wed, Jun 8, 2011 at 11:35 PM, Manish Katiyar <mkatiyar@xxxxxxxxx> wrote:
> 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.
Sorry, I misread your comment. You are right...this should never happen.
--
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
