Patchwork jbd: Make jbd transactions come from its own cache.

login
register
mail settings
Submitter Manish Katiyar
Date June 11, 2011, 9:36 a.m.
Message ID <1307784961-1485-1-git-send-email-mkatiyar@gmail.com>
Download mbox | patch
Permalink /patch/99995/
State Not Applicable
Headers show

Comments

Manish Katiyar - June 11, 2011, 9:36 a.m.
Allocate a slab cache for jbd transaction allocations. It may help to test
various memory failure scenarios.

Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
---
 fs/jbd/checkpoint.c  |    2 +-
 fs/jbd/journal.c     |   25 +++++++++++++++++++++++++
 fs/jbd/transaction.c |    6 ++----
 include/linux/jbd.h  |   16 ++++++++++++++++
 4 files changed, 44 insertions(+), 5 deletions(-)
Andrew Morton - June 13, 2011, 11:41 p.m.
On Sat, 11 Jun 2011 02:36:01 -0700
Manish Katiyar <mkatiyar@gmail.com> wrote:

> Allocate a slab cache for jbd transaction allocations. It may help to test
> various memory failure scenarios.

This doesn't seem terribly useful to me.  That's truly ancient code and
the change rate is really low.  If there were problem in there then
we'd already know about them.  And the allocation frequency for these
objects is very low.

If you had some deeper reason for making this change, please describe
it.  For example, were you chasing some bug?

> +static int journal_init_transaction_cache(void)
> +{
> +	J_ASSERT(jbd_transaction_cache == NULL);
> +	jbd_transaction_cache = kmem_cache_create("jbd_transaction",
> +			sizeof(transaction_t),
> +			0, 0, NULL);

We have a KMEM_CACHE helper macro for this.  It proved useful at the
time, when we were changing the kmem_cache_create() arguments fairly
often.  It's less useful now.

--
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
Manish Katiyar - June 14, 2011, 12:02 a.m.
On Mon, Jun 13, 2011 at 4:41 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Sat, 11 Jun 2011 02:36:01 -0700
> Manish Katiyar <mkatiyar@gmail.com> wrote:
>
>> Allocate a slab cache for jbd transaction allocations. It may help to test
>> various memory failure scenarios.
>
> This doesn't seem terribly useful to me.  That's truly ancient code and
> the change rate is really low.  If there were problem in there then
> we'd already know about them.  And the allocation frequency for these
> objects is very low.
>
> If you had some deeper reason for making this change, please describe
> it.  For example, were you chasing some bug?

Hi Andrew,

   I was doing similar changes for jbd2, hence did it for jbd too. The
changes for jbd2 were done so that we can use the
fault injection framework to introduce/test ENOMEM situations from the
journal transaction allocation layer and test
how well the callers (ext4 code in particular) deal with such
situations and fix them if required.

Patch

diff --git a/fs/jbd/checkpoint.c b/fs/jbd/checkpoint.c
index e4b87bc..94c428c 100644
--- a/fs/jbd/checkpoint.c
+++ b/fs/jbd/checkpoint.c
@@ -753,5 +753,5 @@  void __journal_drop_transaction(journal_t *journal, transaction_t *transaction)
 	J_ASSERT(journal->j_running_transaction != transaction);
 
 	jbd_debug(1, "Dropping transaction %d, all done\n", transaction->t_tid);
-	kfree(transaction);
+	jbd_free_transaction(transaction);
 }
diff --git a/fs/jbd/journal.c b/fs/jbd/journal.c
index e2d4285..9e0ddb8 100644
--- a/fs/jbd/journal.c
+++ b/fs/jbd/journal.c
@@ -82,6 +82,7 @@  EXPORT_SYMBOL(journal_blocks_per_page);
 EXPORT_SYMBOL(journal_invalidatepage);
 EXPORT_SYMBOL(journal_try_to_free_buffers);
 EXPORT_SYMBOL(journal_force_commit);
+EXPORT_SYMBOL(jbd_transaction_cache);
 
 static int journal_convert_superblock_v1(journal_t *, journal_superblock_t *);
 static void __journal_abort_soft (journal_t *journal, int errno);
@@ -1752,6 +1753,27 @@  static void journal_destroy_journal_head_cache(void)
 	}
 }
 
+struct kmem_cache *jbd_transaction_cache;
+
+static int journal_init_transaction_cache(void)
+{
+	J_ASSERT(jbd_transaction_cache == NULL);
+	jbd_transaction_cache = kmem_cache_create("jbd_transaction",
+			sizeof(transaction_t),
+			0, 0, NULL);
+	if (jbd_transaction_cache == NULL) {
+		printk(KERN_EMERG "JBD: failed to create transaction cache\n");
+		return -ENOMEM;
+	}
+	return 0;
+}
+
+static void journal_destroy_transaction_cache(void)
+{
+	if (jbd_transaction_cache)
+		kmem_cache_destroy(jbd_transaction_cache);
+}
+
 /*
  * journal_head splicing and dicing
  */
@@ -2033,6 +2055,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;
 }
 
@@ -2041,6 +2065,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 f7ee81a..64069dd 100644
--- a/fs/jbd/transaction.c
+++ b/fs/jbd/transaction.c
@@ -99,8 +99,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_NOFS|__GFP_NOFAIL);
+		new_transaction = jbd_alloc_transaction(GFP_NOFS|__GFP_NOFAIL);
 		if (!new_transaction) {
 			ret = -ENOMEM;
 			goto out;
@@ -232,8 +231,7 @@  repeat_locked:
 
 	lock_map_acquire(&handle->h_lockdep_map);
 out:
-	if (unlikely(new_transaction))		/* It's usually NULL */
-		kfree(new_transaction);
+	jbd_free_transaction(new_transaction);
 	return ret;
 }
 
diff --git a/include/linux/jbd.h b/include/linux/jbd.h
index e069650..9d7c35e 100644
--- a/include/linux/jbd.h
+++ b/include/linux/jbd.h
@@ -958,6 +958,22 @@  static inline void jbd_free_handle(handle_t *handle)
 	kmem_cache_free(jbd_handle_cache, handle);
 }
 
+/*
+ * transaction management
+ */
+extern struct kmem_cache *jbd_transaction_cache;
+
+static inline transaction_t *jbd_alloc_transaction(gfp_t gfp_flags)
+{
+	return kmem_cache_zalloc(jbd_transaction_cache, gfp_flags);
+}
+
+static inline void jbd_free_transaction(transaction_t *transaction)
+{
+	if (transaction)
+		kmem_cache_free(jbd_transaction_cache, transaction);
+}
+
 /* Primary revoke support */
 #define JOURNAL_REVOKE_DEFAULT_HASH 256
 extern int	   journal_init_revoke(journal_t *, int);