| Message ID | 1321183772-6181-1-git-send-email-xiaoqiangnk@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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@gmail.com> 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 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 >
On Mon, Nov 14, 2011 at 4:53 AM, Jan Kara <jack@suse.cz> 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@gmail.com> > 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 > 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@suse.cz> > SUSE Labs, CR > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Mon 14-11-11 07:22:03, Manish Katiyar wrote: > On Mon, Nov 14, 2011 at 4:53 AM, Jan Kara <jack@suse.cz> 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@gmail.com> > > 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@suse.cz> > > SUSE Labs, CR > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > > -- > Thanks - > Manish
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. *
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@gmail.com> --- 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(-)