On Mon 14-11-11 07:22:03, Manish Katiyar wrote: > On Mon, Nov 14, 2011 at 4:53 AM, Jan Kara <jack@xxxxxxx> wrote: > > On Sun 13-11-11 19:29:31, Yongqiang Yang wrote: > >> transaction_t is 100 bytes under 32-bit systems. Transactions > >> allocated from general cache comsume 128 bytes. This patch > >> lets jbd allocates transacion from special cache. > >> > >> Signed-off-by: Yongqiang Yang <xiaoqiangnk@xxxxxxxxx> > > Space saving isn't probably a good argument here since the number of > > transactions on the system is going to be rather low (a couple per ext3 > > filesystem). The reason why I wanted this change for JBD2 is that it makes > > debugging of memory corruption of a transaction structure much easier (and > > I've been tracking down issues like this several times already). > > > > That being said, I don't find strong need to have this in ext3 and thus > > I'll take the change in jbd/ext3 > > I had submitted a similar patch for ext3, but it was rejected by > Andrew Morton. http://lists.openwall.net/linux-ext4/2011/06/13/15 Now I remember :) Anyway, "it makes debugging simpler" is really the reason which would change Andrew's mind I guess. I actually used a patch like this once or twice exactly to debug some stuff... That's why I think the patch has a value for ext4/jbd2 and I'll follow their lead with ext3. Honza > > if Ted takes the similar change to > > jbd2/ext4 to keep compatibility. Manish Katiyar was working on it > > (http://comments.gmane.org/gmane.comp.file-systems.ext4/25884) but it seems > > it wasn't merged yet. > > > > Honza > > > >> --- > >> fs/jbd/checkpoint.c | 2 +- > >> fs/jbd/journal.c | 3 +++ > >> fs/jbd/transaction.c | 31 +++++++++++++++++++++++++++++-- > >> include/linux/jbd.h | 5 +++++ > >> 4 files changed, 38 insertions(+), 3 deletions(-) > >> > >> diff --git a/fs/jbd/checkpoint.c b/fs/jbd/checkpoint.c > >> index f94fc48..4d98046 100644 > >> --- a/fs/jbd/checkpoint.c > >> +++ b/fs/jbd/checkpoint.c > >> @@ -764,5 +764,5 @@ void __journal_drop_transaction(journal_t *journal, transaction_t *transaction) > >> > >> trace_jbd_drop_transaction(journal, transaction); > >> jbd_debug(1, "Dropping transaction %d, all done\n", transaction->t_tid); > >> - kfree(transaction); > >> + journal_free_transaction(transaction); > >> } > >> diff --git a/fs/jbd/journal.c b/fs/jbd/journal.c > >> index 9fe061f..45ca982 100644 > >> --- a/fs/jbd/journal.c > >> +++ b/fs/jbd/journal.c > >> @@ -2004,6 +2004,8 @@ static int __init journal_init_caches(void) > >> ret = journal_init_journal_head_cache(); > >> if (ret == 0) > >> ret = journal_init_handle_cache(); > >> + if (ret == 0) > >> + ret = journal_init_transaction_cache(); > >> return ret; > >> } > >> > >> @@ -2012,6 +2014,7 @@ static void journal_destroy_caches(void) > >> journal_destroy_revoke_caches(); > >> journal_destroy_journal_head_cache(); > >> journal_destroy_handle_cache(); > >> + journal_destroy_transaction_cache(); > >> } > >> > >> static int __init journal_init(void) > >> diff --git a/fs/jbd/transaction.c b/fs/jbd/transaction.c > >> index 7e59c6e..20d76f1 100644 > >> --- a/fs/jbd/transaction.c > >> +++ b/fs/jbd/transaction.c > >> @@ -30,6 +30,32 @@ > >> > >> static void __journal_temp_unlink_buffer(struct journal_head *jh); > >> > >> +static struct kmem_cache *transaction_cache; > >> +int __init journal_init_transaction_cache(void) > >> +{ > >> + J_ASSERT(!transaction_cache); > >> + transaction_cache = KMEM_CACHE(transaction_s, > >> + SLAB_HWCACHE_ALIGN|SLAB_TEMPORARY); > >> + if (transaction_cache) > >> + return 0; > >> + return -ENOMEM; > >> +} > >> + > >> +void __init journal_destroy_transaction_cache(void) > >> +{ > >> + if (transaction_cache) { > >> + kmem_cache_destroy(transaction_cache); > >> + transaction_cache = NULL; > >> + } > >> +} > >> + > >> +void journal_free_transaction(transaction_t *transaction) > >> +{ > >> + if (unlikely(ZERO_OR_NULL_PTR(transaction))) > >> + return; > >> + kmem_cache_free(transaction_cache, transaction); > >> +} > >> + > >> /* > >> * get_transaction: obtain a new transaction_t object. > >> * > >> @@ -100,11 +126,12 @@ static int start_this_handle(journal_t *journal, handle_t *handle) > >> > >> alloc_transaction: > >> if (!journal->j_running_transaction) { > >> - new_transaction = kzalloc(sizeof(*new_transaction), GFP_NOFS); > >> + new_transaction = kmem_cache_alloc(transaction_cache, GFP_NOFS); > >> if (!new_transaction) { > >> congestion_wait(BLK_RW_ASYNC, HZ/50); > >> goto alloc_transaction; > >> } > >> + memset(new_transaction, 0, sizeof(*new_transaction)); > >> } > >> > >> jbd_debug(3, "New handle %p going live.\n", handle); > >> @@ -233,7 +260,7 @@ repeat_locked: > >> lock_map_acquire(&handle->h_lockdep_map); > >> out: > >> if (unlikely(new_transaction)) /* It's usually NULL */ > >> - kfree(new_transaction); > >> + journal_free_transaction(new_transaction); > >> return ret; > >> } > >> > >> diff --git a/include/linux/jbd.h b/include/linux/jbd.h > >> index c7acdde..0f9f0b6 100644 > >> --- a/include/linux/jbd.h > >> +++ b/include/linux/jbd.h > >> @@ -807,6 +807,11 @@ journal_write_metadata_buffer(transaction_t *transaction, > >> /* Transaction locking */ > >> extern void __wait_on_journal (journal_t *); > >> > >> +/* Transaction cache support */ > >> +extern void journal_destroy_transaction_cache(void); > >> +extern int journal_init_transaction_cache(void); > >> +extern void journal_free_transaction(transaction_t *); > >> + > >> /* > >> * Journal locking. > >> * > >> -- > >> 1.7.5.1 > >> > > -- > > Jan Kara <jack@xxxxxxx> > > SUSE Labs, CR > > -- > > 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 > > > > > > -- > Thanks - > Manish -- Jan Kara <jack@xxxxxxx> SUSE Labs, CR -- 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
