Re: [PATCH 1/2] jbd: allocate transacion from special cache

On Mon 14-11-11 07:22:03, Manish Katiyar wrote:

On Mon, Nov 14, 2011 at 4:53 AM, Jan Kara [off-list ref] wrote:

quoted

On Sun 13-11-11 19:29:31, Yongqiang Yang wrote:

quoted

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 <redacted>

 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

quoted

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

quoted

---
 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 [off-list ref]
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

-- 
Jan Kara [off-list ref]
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