From: Jan Kara Subject: Re: [PATCH 1/2] jbd: allocate transacion from special cache Date: Mon, 14 Nov 2011 16:35:57 +0100 Message-ID: <20111114153557.GM5230@quack.suse.cz> References: <1321183772-6181-1-git-send-email-xiaoqiangnk@gmail.com> <20111114125357.GH5230@quack.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Jan Kara , Yongqiang Yang , linux-ext4@vger.kernel.org To: Manish Katiyar Return-path: Received: from cantor2.suse.de ([195.135.220.15]:44058 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753107Ab1KNPgT (ORCPT ); Mon, 14 Nov 2011 10:36:19 -0500 Content-Disposition: inline In-Reply-To: Sender: linux-ext4-owner@vger.kernel.org List-ID: On Mon 14-11-11 07:22:03, Manish Katiyar wrote: > On Mon, Nov 14, 2011 at 4:53 AM, Jan Kara 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 > > =A0Space saving isn't probably a good argument here since the numbe= r 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 i= t makes > > debugging of memory corruption of a transaction structure much easi= er (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 >=20 > 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 patc= h like this once or twice exactly to debug some stuff... That's why I thi= nk the patch has a value for ext4/jbd2 and I'll follow their lead with ext= 3. 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. > > > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0Honza > > > >> --- > >> =A0fs/jbd/checkpoint.c =A0| =A0 =A02 +- > >> =A0fs/jbd/journal.c =A0 =A0 | =A0 =A03 +++ > >> =A0fs/jbd/transaction.c | =A0 31 +++++++++++++++++++++++++++++-- > >> =A0include/linux/jbd.h =A0| =A0 =A05 +++++ > >> =A04 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 *jou= rnal, transaction_t *transaction) > >> > >> =A0 =A0 =A0 trace_jbd_drop_transaction(journal, transaction); > >> =A0 =A0 =A0 jbd_debug(1, "Dropping transaction %d, all done\n", tr= ansaction->t_tid); > >> - =A0 =A0 kfree(transaction); > >> + =A0 =A0 journal_free_transaction(transaction); > >> =A0} > >> 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) > >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D journal_init_journal_head_cach= e(); > >> =A0 =A0 =A0 if (ret =3D=3D 0) > >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D journal_init_handle_cache(); > >> + =A0 =A0 if (ret =3D=3D 0) > >> + =A0 =A0 =A0 =A0 =A0 =A0 ret =3D journal_init_transaction_cache()= ; > >> =A0 =A0 =A0 return ret; > >> =A0} > >> > >> @@ -2012,6 +2014,7 @@ static void journal_destroy_caches(void) > >> =A0 =A0 =A0 journal_destroy_revoke_caches(); > >> =A0 =A0 =A0 journal_destroy_journal_head_cache(); > >> =A0 =A0 =A0 journal_destroy_handle_cache(); > >> + =A0 =A0 journal_destroy_transaction_cache(); > >> =A0} > >> > >> =A0static 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 @@ > >> > >> =A0static void __journal_temp_unlink_buffer(struct journal_head *j= h); > >> > >> +static struct kmem_cache *transaction_cache; > >> +int __init journal_init_transaction_cache(void) > >> +{ > >> + =A0 =A0 J_ASSERT(!transaction_cache); > >> + =A0 =A0 transaction_cache =3D KMEM_CACHE(transaction_s, > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0SLAB_HWCACHE_ALIGN|SLAB_TEMPORARY); > >> + =A0 =A0 if (transaction_cache) > >> + =A0 =A0 =A0 =A0 =A0 =A0 return 0; > >> + =A0 =A0 return -ENOMEM; > >> +} > >> + > >> +void __init journal_destroy_transaction_cache(void) > >> +{ > >> + =A0 =A0 if (transaction_cache) { > >> + =A0 =A0 =A0 =A0 =A0 =A0 kmem_cache_destroy(transaction_cache); > >> + =A0 =A0 =A0 =A0 =A0 =A0 transaction_cache =3D NULL; > >> + =A0 =A0 } > >> +} > >> + > >> +void journal_free_transaction(transaction_t *transaction) > >> +{ > >> + =A0 =A0 if (unlikely(ZERO_OR_NULL_PTR(transaction))) > >> + =A0 =A0 =A0 =A0 =A0 =A0 return; > >> + =A0 =A0 kmem_cache_free(transaction_cache, transaction); > >> +} > >> + > >> =A0/* > >> =A0 * get_transaction: obtain a new transaction_t object. > >> =A0 * > >> @@ -100,11 +126,12 @@ static int start_this_handle(journal_t *jour= nal, handle_t *handle) > >> > >> =A0alloc_transaction: > >> =A0 =A0 =A0 if (!journal->j_running_transaction) { > >> - =A0 =A0 =A0 =A0 =A0 =A0 new_transaction =3D kzalloc(sizeof(*new_= transaction), GFP_NOFS); > >> + =A0 =A0 =A0 =A0 =A0 =A0 new_transaction =3D kmem_cache_alloc(tra= nsaction_cache, GFP_NOFS); > >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!new_transaction) { > >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 congestion_wait(BLK_RW= _ASYNC, HZ/50); > >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto alloc_transaction= ; > >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 } > >> + =A0 =A0 =A0 =A0 =A0 =A0 memset(new_transaction, 0, sizeof(*new_t= ransaction)); > >> =A0 =A0 =A0 } > >> > >> =A0 =A0 =A0 jbd_debug(3, "New handle %p going live.\n", handle); > >> @@ -233,7 +260,7 @@ repeat_locked: > >> =A0 =A0 =A0 lock_map_acquire(&handle->h_lockdep_map); > >> =A0out: > >> =A0 =A0 =A0 if (unlikely(new_transaction)) =A0 =A0 =A0 =A0 =A0/* I= t's usually NULL */ > >> - =A0 =A0 =A0 =A0 =A0 =A0 kfree(new_transaction); > >> + =A0 =A0 =A0 =A0 =A0 =A0 journal_free_transaction(new_transaction= ); > >> =A0 =A0 =A0 return ret; > >> =A0} > >> > >> 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 =A0= =A0 =A0 =A0*transaction, > >> =A0/* Transaction locking */ > >> =A0extern void =A0 =A0 =A0 =A0 =A0__wait_on_journal (journal_t *); > >> > >> +/* Transaction cache support */ > >> +extern void journal_destroy_transaction_cache(void); > >> +extern int =A0journal_init_transaction_cache(void); > >> +extern void journal_free_transaction(transaction_t *); > >> + > >> =A0/* > >> =A0 * Journal locking. > >> =A0 * > >> -- > >> 1.7.5.1 > >> > > -- > > Jan Kara > > SUSE Labs, CR > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-ext= 4" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at =A0http://vger.kernel.org/majordomo-info.htm= l > > >=20 >=20 >=20 > --=20 > Thanks - > Manish --=20 Jan Kara SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" i= n the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html