| Message ID | 1307262510-16376-1-git-send-email-mkatiyar@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
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. Honza
On Wed, Jun 8, 2011 at 9:49 AM, Jan Kara <jack@suse.cz> 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? 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.
On Wed, Jun 8, 2011 at 11:35 PM, Manish Katiyar <mkatiyar@gmail.com> wrote: > On Wed, Jun 8, 2011 at 9:49 AM, Jan Kara <jack@suse.cz> 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? > > 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.
On Wed, Jun 8, 2011 at 9:49 AM, Jan Kara <jack@suse.cz> 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? 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 -- 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 Thu 09-06-11 23:21:17, Manish Katiyar wrote: > On Wed, Jun 8, 2011 at 9:49 AM, Jan Kara <jack@suse.cz> 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? > > 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
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) */
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(-)