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] [day] [month] [year] [list]

Message-ID: <20110613133100.GB4907@quack.suse.cz>
Date:	Mon, 13 Jun 2011 15:31:00 +0200
From:	Jan Kara <jack@...e.cz>
To:	Manish Katiyar <mkatiyar@...il.com>
Cc:	Jan Kara <jack@...e.cz>, tytso@....edu, linux-ext4@...r.kernel.org
Subject: Re: [PATCH] jbd2:Make journal transaction allocations come from
 its own cache.

On Thu 09-06-11 23:21:17, Manish Katiyar 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,
> 
> 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.
  Ah, good point. Thanks for checking this.

								Honza
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
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