2008-03-07 01:31:28

by Duane Griffin

[permalink] [raw]
Subject: [PATCH 1/3] jbd2: eliminate duplicated code in revocation table init/destroy functions

The revocation table initialisation/destruction code is repeated for each of
the two revocation tables stored in the journal. Refactoring the duplicated
code into functions is tidier, simplifies the logic in initialisation in
particular, and slightly reduces the code size.

There should not be any functional change.

Signed-off-by: Duane Griffin <[email protected]>
---
fs/jbd2/revoke.c | 125 +++++++++++++++++++++++-------------------------------
1 files changed, 53 insertions(+), 72 deletions(-)

diff --git a/fs/jbd2/revoke.c b/fs/jbd2/revoke.c
index df36f42..1bf4c1f 100644
--- a/fs/jbd2/revoke.c
+++ b/fs/jbd2/revoke.c
@@ -196,108 +196,89 @@ void jbd2_journal_destroy_revoke_caches(void)
jbd2_revoke_table_cache = NULL;
}

-/* Initialise the revoke table for a given journal to a given size. */
-
-int jbd2_journal_init_revoke(journal_t *journal, int hash_size)
+static int jbd2_journal_init_revoke_table(struct jbd2_revoke_table_s *table,
+ int size)
{
- int shift, tmp;
+ int shift = 0;
+ int tmp = size;

- J_ASSERT (journal->j_revoke_table[0] == NULL);
-
- shift = 0;
- tmp = hash_size;
while((tmp >>= 1UL) != 0UL)
shift++;

- journal->j_revoke_table[0] = kmem_cache_alloc(jbd2_revoke_table_cache, GFP_KERNEL);
- if (!journal->j_revoke_table[0])
- return -ENOMEM;
- journal->j_revoke = journal->j_revoke_table[0];
-
- /* Check that the hash_size is a power of two */
- J_ASSERT(is_power_of_2(hash_size));
-
- journal->j_revoke->hash_size = hash_size;
-
- journal->j_revoke->hash_shift = shift;
-
- journal->j_revoke->hash_table =
- kmalloc(hash_size * sizeof(struct list_head), GFP_KERNEL);
- if (!journal->j_revoke->hash_table) {
- kmem_cache_free(jbd2_revoke_table_cache, journal->j_revoke_table[0]);
- journal->j_revoke = NULL;
+ table->hash_size = size;
+ table->hash_shift = shift;
+ table->hash_table = kmalloc(
+ size * sizeof(struct list_head), GFP_KERNEL);
+ if (!table->hash_table)
return -ENOMEM;
- }

- for (tmp = 0; tmp < hash_size; tmp++)
- INIT_LIST_HEAD(&journal->j_revoke->hash_table[tmp]);
+ for (tmp = 0; tmp < size; tmp++)
+ INIT_LIST_HEAD(&table->hash_table[tmp]);

- journal->j_revoke_table[1] = kmem_cache_alloc(jbd2_revoke_table_cache, GFP_KERNEL);
- if (!journal->j_revoke_table[1]) {
- kfree(journal->j_revoke_table[0]->hash_table);
- kmem_cache_free(jbd2_revoke_table_cache, journal->j_revoke_table[0]);
- return -ENOMEM;
- }
-
- journal->j_revoke = journal->j_revoke_table[1];
+ return 0;
+}

- /* Check that the hash_size is a power of two */
+/* Initialise the revoke table for a given journal to a given size. */
+int jbd2_journal_init_revoke(journal_t *journal, int hash_size)
+{
+ J_ASSERT(journal->j_revoke_table[0] == NULL);
J_ASSERT(is_power_of_2(hash_size));

- journal->j_revoke->hash_size = hash_size;
+ journal->j_revoke_table[0] = kmem_cache_alloc(
+ jbd2_revoke_table_cache, GFP_KERNEL);
+ if (!journal->j_revoke_table[0])
+ goto failed_alloc1;
+ if (jbd2_journal_init_revoke_table(journal->j_revoke_table[0], hash_size))
+ goto failed_init1;

- journal->j_revoke->hash_shift = shift;
+ journal->j_revoke_table[1] = kmem_cache_alloc(
+ jbd2_revoke_table_cache, GFP_KERNEL);
+ if (!journal->j_revoke_table[1])
+ goto failed_alloc2;
+ if (jbd2_journal_init_revoke_table(journal->j_revoke_table[1], hash_size))
+ goto failed_init2;

- journal->j_revoke->hash_table =
- kmalloc(hash_size * sizeof(struct list_head), GFP_KERNEL);
- if (!journal->j_revoke->hash_table) {
- kfree(journal->j_revoke_table[0]->hash_table);
- kmem_cache_free(jbd2_revoke_table_cache, journal->j_revoke_table[0]);
- kmem_cache_free(jbd2_revoke_table_cache, journal->j_revoke_table[1]);
- journal->j_revoke = NULL;
- return -ENOMEM;
- }
-
- for (tmp = 0; tmp < hash_size; tmp++)
- INIT_LIST_HEAD(&journal->j_revoke->hash_table[tmp]);
+ journal->j_revoke = journal->j_revoke_table[1];

spin_lock_init(&journal->j_revoke_lock);

return 0;
-}

-/* Destoy a journal's revoke table. The table must already be empty! */
+failed_init2:
+ kmem_cache_free(jbd2_revoke_table_cache, journal->j_revoke_table[1]);
+failed_alloc2:
+ kfree(journal->j_revoke_table[0]->hash_table);
+failed_init1:
+ kmem_cache_free(jbd2_revoke_table_cache, journal->j_revoke_table[0]);
+failed_alloc1:
+ return -ENOMEM;
+}

-void jbd2_journal_destroy_revoke(journal_t *journal)
+static void jbd2_journal_destroy_revoke_table(struct jbd2_revoke_table_s *table)
{
- struct jbd2_revoke_table_s *table;
- struct list_head *hash_list;
int i;
+ struct list_head *hash_list;

- table = journal->j_revoke_table[0];
- if (!table)
- return;
-
- for (i=0; i<table->hash_size; i++) {
+ for (i = 0; i < table->hash_size; i++) {
hash_list = &table->hash_table[i];
- J_ASSERT (list_empty(hash_list));
+ J_ASSERT(list_empty(hash_list));
}

kfree(table->hash_table);
kmem_cache_free(jbd2_revoke_table_cache, table);
- journal->j_revoke = NULL;
+}

- table = journal->j_revoke_table[1];
- if (!table)
+/* Destroy a journal's revoke table. The table must already be empty! */
+void jbd2_journal_destroy_revoke(journal_t *journal)
+{
+ if (!journal->j_revoke_table[0])
return;
+ jbd2_journal_destroy_revoke_table(journal->j_revoke_table[0]);
+ journal->j_revoke = NULL;

- for (i=0; i<table->hash_size; i++) {
- hash_list = &table->hash_table[i];
- J_ASSERT (list_empty(hash_list));
- }


2008-03-07 01:31:30

by Duane Griffin

[permalink] [raw]
Subject: [PATCH 2/3] jbd2: replace potentially false assertion with if block

If an error occurs during jbd2 cache initialisation it is possible for the
journal_head_cache to be NULL when jbd2_journal_destroy_journal_head_cache is
called. Replace the J_ASSERT with an if block to handle the situation
correctly.

Note that even with this fix things will break badly if jbd2 is statically
compiled in and cache initialisation fails.

Signed-off-by: Duane Griffin <[email protected]>
---
fs/jbd2/journal.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 96ba846..0d8a595 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1985,9 +1985,10 @@ static int journal_init_jbd2_journal_head_cache(void)

static void jbd2_journal_destroy_jbd2_journal_head_cache(void)
{
- J_ASSERT(jbd2_journal_head_cache != NULL);
- kmem_cache_destroy(jbd2_journal_head_cache);
- jbd2_journal_head_cache = NULL;
+ if (jbd2_journal_head_cache) {
+ kmem_cache_destroy(jbd2_journal_head_cache);
+ jbd2_journal_head_cache = NULL;
+ }
}

/*
--
1.5.3.7


2008-03-07 01:31:32

by Duane Griffin

[permalink] [raw]
Subject: [PATCH 3/3] jbd2: only create debugfs and stats entries if cache initialisation is successful

jbd2 debugfs and stats entries should only be created if cache initialisation
is successful. At the moment they are being created unconditionally which will
leave them dangling if cache (and hence module) initialisation fails.

Signed-off-by: Duane Griffin <[email protected]>
---
fs/jbd2/journal.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 0d8a595..9d48419 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -2315,10 +2315,12 @@ static int __init journal_init(void)
BUILD_BUG_ON(sizeof(struct journal_superblock_s) != 1024);

ret = journal_init_caches();
- if (ret != 0)
+ if (ret == 0) {
+ jbd2_create_debugfs_entry();
+ jbd2_create_jbd_stats_proc_entry();
+ } else {
jbd2_journal_destroy_caches();
- jbd2_create_debugfs_entry();
- jbd2_create_jbd_stats_proc_entry();
+ }
return ret;
}

--
1.5.3.7


2008-03-07 01:31:23

by Duane Griffin

[permalink] [raw]
Subject: [PATCH 2/3] jbd2: replace potentially false assertion with if block

If an error occurs during jbd2 cache initialisation it is possible for the
journal_head_cache to be NULL when jbd2_journal_destroy_journal_head_cache is
called. Replace the J_ASSERT with an if block to handle the situation
correctly.

Note that even with this fix things will break badly if jbd2 is statically
compiled in and cache initialisation fails.

Signed-off-by: Duane Griffin <[email protected]>
---
fs/jbd2/journal.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 96ba846..0d8a595 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1985,9 +1985,10 @@ static int journal_init_jbd2_journal_head_cache(void)

static void jbd2_journal_destroy_jbd2_journal_head_cache(void)
{
- J_ASSERT(jbd2_journal_head_cache != NULL);
- kmem_cache_destroy(jbd2_journal_head_cache);
- jbd2_journal_head_cache = NULL;
+ if (jbd2_journal_head_cache) {
+ kmem_cache_destroy(jbd2_journal_head_cache);
+ jbd2_journal_head_cache = NULL;
+ }
}

/*
--
1.5.3.7

2008-03-07 01:31:24

by Duane Griffin

[permalink] [raw]
Subject: [PATCH 3/3] jbd2: only create debugfs and stats entries if cache initialisation is successful

jbd2 debugfs and stats entries should only be created if cache initialisation
is successful. At the moment they are being created unconditionally which will
leave them dangling if cache (and hence module) initialisation fails.

Signed-off-by: Duane Griffin <[email protected]>
---
fs/jbd2/journal.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 0d8a595..9d48419 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -2315,10 +2315,12 @@ static int __init journal_init(void)
BUILD_BUG_ON(sizeof(struct journal_superblock_s) != 1024);

ret = journal_init_caches();
- if (ret != 0)
+ if (ret == 0) {
+ jbd2_create_debugfs_entry();
+ jbd2_create_jbd_stats_proc_entry();
+ } else {
jbd2_journal_destroy_caches();
- jbd2_create_debugfs_entry();
- jbd2_create_jbd_stats_proc_entry();
+ }
return ret;
}

--
1.5.3.7