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 <[email protected]>
---
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);
+ 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)
+ 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
+ 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
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 <[email protected]>
> ---
> 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.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed, Jun 8, 2011 at 9:49 AM, Jan Kara <[email protected]> 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 <[email protected]>
>> ---
>> ?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,
In start_this_handle() we can pass a NULL value here. Since we had
kfree() earlier which could silently handle NULL, either I can check
before calling or handle it here. To verify I removed the if condition
and it immediately panic'd on boot.
(gdb) do
#20 start_this_handle (journal=0x17cd7c00, handle=0x1790e000,
gfp_mask=80) at fs/jbd2/transaction.c:285
285 jbd2_free_transaction(new_transaction);
(gdb) l
280 atomic_read(&transaction->t_outstanding_credits),
281 __jbd2_log_space_left(journal));
282 read_unlock(&journal->j_state_lock);
283
284 lock_map_acquire(&handle->h_lockdep_map);
285 jbd2_free_transaction(new_transaction);
286
287 return 0;
288 }
289
(gdb) p new_transaction
$1 = (transaction_t *) 0x0
I'll send a patch shortly with other comments incorporated.
--
Thanks -
Manish
On Wed, Jun 8, 2011 at 11:35 PM, Manish Katiyar <[email protected]> wrote:
> On Wed, Jun 8, 2011 at 9:49 AM, Jan Kara <[email protected]> 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 <[email protected]>
>>> ---
>>> ?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,
>
> In start_this_handle() we can pass a NULL value here. Since we had
> kfree() earlier which could silently handle NULL, either I can check
> before calling or handle it here. To verify I removed the if condition
> and it immediately panic'd on boot.
>
> (gdb) do
> #20 start_this_handle (journal=0x17cd7c00, handle=0x1790e000,
> gfp_mask=80) at fs/jbd2/transaction.c:285
> 285 ? ? ? ? ? ? jbd2_free_transaction(new_transaction);
> (gdb) l
> 280 ? ? ? ? ? ? ? ? ? ? ? atomic_read(&transaction->t_outstanding_credits),
> 281 ? ? ? ? ? ? ? ? ? ? ? __jbd2_log_space_left(journal));
> 282 ? ? ? ? ? ? read_unlock(&journal->j_state_lock);
> 283
> 284 ? ? ? ? ? ? lock_map_acquire(&handle->h_lockdep_map);
> 285 ? ? ? ? ? ? jbd2_free_transaction(new_transaction);
> 286
> 287 ? ? ? ? ? ? return 0;
> 288 ? ? }
> 289
> (gdb) p new_transaction
> $1 = (transaction_t *) 0x0
>
>
> I'll send a patch shortly with other comments incorporated.
Sorry, I misread your comment. You are right...this should never happen.
--
Thanks -
Manish
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 <[email protected]>
>> ---
>> 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 <[email protected]>
---
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
On Thu 09-06-11 00:41:04, 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 <[email protected]>
Now the patch looks OK. You can add:
Acked-by: Jan Kara <[email protected]>
Honza
PS: Thanks for formatting the patch submission properly now! Just when you
have the final version, send it separately (i.e. not as a reply to my
comments) so that Ted can easily see it and pick it up.
> ---
> 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
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed, Jun 8, 2011 at 9:49 AM, Jan Kara <[email protected]> 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 <[email protected]>
>> ---
>> ?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.
Thanks -
Manish
On Thu 09-06-11 23:21:17, Manish Katiyar wrote:
> On Wed, Jun 8, 2011 at 9:49 AM, Jan Kara <[email protected]> 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 <[email protected]>
> >> ---
> >> ?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 <[email protected]>
SUSE Labs, CR