From: Manish Katiyar <mkatiyar@gmail.com>
To: Jan Kara <jack@suse.cz>
Cc: tytso@mit.edu, linux-ext4@vger.kernel.org, mkatiyar@gmail.com
Subject: Re: [PATCH] jbd2:Make journal transaction allocations come from its own cache.
Date: Thu, 09 Jun 2011 00:41:04 -0700 [thread overview]
Message-ID: <4DF07910.1060209@gmail.com> (raw)
In-Reply-To: <20110608164903.GG5361@quack.suse.cz>
On 06/08/2011 09:49 AM, Jan Kara 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@gmail.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?
>
>> + kmem_cache_destroy(jbd2_transaction_cache);
>> +}
>> +
>> /*
>> * Module startup and shutdown
>> */
>> @@ -2384,6 +2406,8 @@ static int __init journal_init_caches(void)
>> ret = journal_init_jbd2_journal_head_cache();
>> if (ret == 0)
>> ret = journal_init_handle_cache();
>> + if (ret == 0)
>> + ret = journal_init_transaction_cache();
>> return ret;
>> }
>>
>> @@ -2392,6 +2416,7 @@ static void jbd2_journal_destroy_caches(void)
>> jbd2_journal_destroy_revoke_caches();
>> jbd2_journal_destroy_jbd2_journal_head_cache();
>> jbd2_journal_destroy_handle_cache();
>> + jbd2_journal_destroy_transaction_cache();
>> jbd2_journal_destroy_slabs();
>> }
>>
>> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
>> index 3eec82d..58fbec7 100644
>> --- a/fs/jbd2/transaction.c
>> +++ b/fs/jbd2/transaction.c
>> @@ -131,7 +131,7 @@ 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_mask);
>> + new_transaction = jbd2_alloc_transaction(gfp_mask);
>> if (!new_transaction) {
>> /*
>> * If __GFP_FS is not present, then we may be
>> @@ -160,7 +160,7 @@ repeat:
>> if (is_journal_aborted(journal) ||
>> (journal->j_errno != 0 && !(journal->j_flags & JBD2_ACK_ERR))) {
>> read_unlock(&journal->j_state_lock);
>> - kfree(new_transaction);
>> + jbd2_free_transaction(new_transaction);
>> return -EROFS;
>> }
>>
>> @@ -282,7 +282,8 @@ repeat:
>> read_unlock(&journal->j_state_lock);
>>
>> lock_map_acquire(&handle->h_lockdep_map);
>> - kfree(new_transaction);
>> + jbd2_free_transaction(new_transaction);
>> +
>> return 0;
>> }
>>
>> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
>> index 4ecb7b1..df7f101 100644
>> --- a/include/linux/jbd2.h
>> +++ b/include/linux/jbd2.h
>> @@ -1184,6 +1184,27 @@ static inline void jbd2_free_handle(handle_t *handle)
>> }
>>
>> /*
>> + * transaction management
>> + */
>> +extern struct kmem_cache *jbd2_transaction_cache;
>> +
>> +static inline transaction_t *jbd2_alloc_transaction(gfp_t gfp_flags)
>> +{
>> + return kmem_cache_zalloc(jbd2_transaction_cache, gfp_flags);
>> +}
>> +
>> +static inline void jbd2_free_transaction(transaction_t *transaction)
>> +{
>> + if (transaction) {
>> +#ifdef CONFIG_JBD2_DEBUG
>> + memset(transaction, JBD2_POISON_FREE, sizeof(*transaction));
>> +#endif
> No need for this - CONFIG_DEBUG_SLAB does this for you...
>
>> + kmem_cache_free(jbd2_transaction_cache, transaction);
>> + }
>> +}
>> +
>> +
>> +/*
>> * jbd2_inode management (optional, for those file systems that want to use
>> * dynamically allocated jbd2_inode structures)
>> */
> Otherwise the patch looks OK to me.
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@gmail.com>
---
v1->v2 :
*) Drop SLAB_TEMPORARY flag for cache allocation.
*) Remove filling with pattern during frees.
fs/jbd2/checkpoint.c | 2 +-
fs/jbd2/commit.c | 2 +-
fs/jbd2/journal.c | 24 ++++++++++++++++++++++++
fs/jbd2/transaction.c | 7 ++++---
include/linux/jbd2.h | 17 +++++++++++++++++
5 files changed, 47 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..2d9795b 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,26 @@ 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, 0, NULL);
+ 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)
+{
+ kmem_cache_destroy(jbd2_transaction_cache);
+}
+
/*
* Module startup and shutdown
*/
@@ -2384,6 +2405,8 @@ static int __init journal_init_caches(void)
ret = journal_init_jbd2_journal_head_cache();
if (ret == 0)
ret = journal_init_handle_cache();
+ if (ret == 0)
+ ret = journal_init_transaction_cache();
return ret;
}
@@ -2392,6 +2415,7 @@ static void jbd2_journal_destroy_caches(void)
jbd2_journal_destroy_revoke_caches();
jbd2_journal_destroy_jbd2_journal_head_cache();
jbd2_journal_destroy_handle_cache();
+ jbd2_journal_destroy_transaction_cache();
jbd2_journal_destroy_slabs();
}
diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 3eec82d..58fbec7 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -131,7 +131,7 @@ 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_mask);
+ new_transaction = jbd2_alloc_transaction(gfp_mask);
if (!new_transaction) {
/*
* If __GFP_FS is not present, then we may be
@@ -160,7 +160,7 @@ repeat:
if (is_journal_aborted(journal) ||
(journal->j_errno != 0 && !(journal->j_flags & JBD2_ACK_ERR))) {
read_unlock(&journal->j_state_lock);
- kfree(new_transaction);
+ jbd2_free_transaction(new_transaction);
return -EROFS;
}
@@ -282,7 +282,8 @@ repeat:
read_unlock(&journal->j_state_lock);
lock_map_acquire(&handle->h_lockdep_map);
- kfree(new_transaction);
+ jbd2_free_transaction(new_transaction);
+
return 0;
}
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 4ecb7b1..803b5c4 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1184,6 +1184,23 @@ static inline void jbd2_free_handle(handle_t *handle)
}
/*
+ * transaction management
+ */
+extern struct kmem_cache *jbd2_transaction_cache;
+
+static inline transaction_t *jbd2_alloc_transaction(gfp_t gfp_flags)
+{
+ return kmem_cache_zalloc(jbd2_transaction_cache, gfp_flags);
+}
+
+static inline void jbd2_free_transaction(transaction_t *transaction)
+{
+ if (transaction)
+ kmem_cache_free(jbd2_transaction_cache, transaction);
+}
+
+
+/*
* jbd2_inode management (optional, for those file systems that want to use
* dynamically allocated jbd2_inode structures)
*/
--
1.7.4.1
next prev parent reply other threads:[~2011-06-09 7:41 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-06-05 8:28 [PATCH] jbd2:Make journal transaction allocations come from its own cache Manish Katiyar
2011-06-08 16:49 ` Jan Kara
2011-06-09 6:35 ` Manish Katiyar
2011-06-09 7:20 ` Manish Katiyar
2011-06-09 7:41 ` Manish Katiyar [this message]
2011-06-09 10:15 ` Jan Kara
2011-06-10 6:21 ` Manish Katiyar
2011-06-13 13:31 ` Jan Kara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4DF07910.1060209@gmail.com \
--to=mkatiyar@gmail.com \
--cc=jack@suse.cz \
--cc=linux-ext4@vger.kernel.org \
--cc=tytso@mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.