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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 9 Jun 2011 00:20:02 -0700
From:	Manish Katiyar <mkatiyar@...il.com>
To:	Jan Kara <jack@...e.cz>
Cc:	tytso@....edu, linux-ext4@...r.kernel.org
Subject: Re: [PATCH] jbd2:Make journal transaction allocations come from its
 own cache.

On Wed, Jun 8, 2011 at 11:35 PM, Manish Katiyar <mkatiyar@...il.com> wrote:
> On Wed, Jun 8, 2011 at 9:49 AM, Jan Kara <jack@...e.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@...il.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.

-- 
Thanks -
Manish
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html