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]

Message-ID: <BANLkTin4KYRZcmyiEeV2QL9PWOz3rVAhsQ@mail.gmail.com>
Date:	Wed, 8 Jun 2011 23:35:50 -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 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.

-- 
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