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

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

[Duane Griffin]

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 <duaneg@dghda.com>
---
 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));
-	}

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

[Duane Griffin]

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 <duaneg@dghda.com>
---
 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));
-	}
-
-	kfree(table->hash_table);
-	kmem_cache_free(jbd2_revoke_table_cache, table);
+	if (!journal->j_revoke_table[1])
+		return;
+	jbd2_journal_destroy_revoke_table(journal->j_revoke_table[1]);
 	journal->j_revoke = NULL;
 }
 
-- 
1.5.3.7

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

[Duane Griffin]

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 <duaneg@dghda.com>
---
 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

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

[Duane Griffin]

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 <duaneg@dghda.com>
---
 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

Re: [PATCH 2/3] jbd2: replace potentially false assertion with if block

[Andreas Dilger]

On Mar 07, 2008  01:31 +0000, Duane Griffin wrote:
> 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.

It would probably be prudent to verify that these caches are initialized
at journal_load() time and either re-try to create the cache, and/or report
an error in that case and refuse to continue mounting.

Also, I note that journal_init_journal_head_cache() is comparing pointers
to "0", a style no-no...

> Signed-off-by: Duane Griffin <duaneg@dghda.com>

Acked-by: Andreas Dilger <adilger@sun.com>
> ---
>  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
> 
> --
> 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

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Re: [PATCH 2/3] jbd2: replace potentially false assertion with if block

[Duane Griffin]

On Fri, Mar 07, 2008 at 02:23:36PM -0700, Andreas Dilger wrote:
> On Mar 07, 2008  01:31 +0000, Duane Griffin wrote:
> > 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.
> 
> It would probably be prudent to verify that these caches are initialized
> at journal_load() time and either re-try to create the cache, and/or report
> an error in that case and refuse to continue mounting.

Sounds like a plan. I'll see what I can whip up.

> Also, I note that journal_init_journal_head_cache() is comparing pointers
> to "0", a style no-no...

Yes, checkpatch noticed that, too. It was in the original, so I left it
alone. There are also a couple of places in revoke.c where it is done.
See the patch below. If you like it I'll send through one for jbd too.

Cheers,
Duane.

-- 
"I never could learn to drink that blood and call it wine" - Bob Dylan

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 9d48419..75349b1 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1969,14 +1969,14 @@ static int journal_init_jbd2_journal_head_cache(void)
 {
 	int retval;
 
-	J_ASSERT(jbd2_journal_head_cache == 0);
+	J_ASSERT(!jbd2_journal_head_cache);
 	jbd2_journal_head_cache = kmem_cache_create("jbd2_journal_head",
 				sizeof(struct journal_head),
 				0,		/* offset */
 				SLAB_TEMPORARY,	/* flags */
 				NULL);		/* ctor */
 	retval = 0;
-	if (jbd2_journal_head_cache == 0) {
+	if (!jbd2_journal_head_cache) {
 		retval = -ENOMEM;
 		printk(KERN_EMERG "JBD: no memory for journal_head cache\n");
 	}
diff --git a/fs/jbd2/revoke.c b/fs/jbd2/revoke.c
index 1bf4c1f..cae1172 100644
--- a/fs/jbd2/revoke.c
+++ b/fs/jbd2/revoke.c
@@ -174,13 +174,13 @@ int __init jbd2_journal_init_revoke_caches(void)
 					   0,
 					   SLAB_HWCACHE_ALIGN|SLAB_TEMPORARY,
 					   NULL);
-	if (jbd2_revoke_record_cache == 0)
+	if (!jbd2_revoke_record_cache)
 		return -ENOMEM;
 
 	jbd2_revoke_table_cache = kmem_cache_create("jbd2_revoke_table",
 					   sizeof(struct jbd2_revoke_table_s),
 					   0, SLAB_TEMPORARY, NULL);
-	if (jbd2_revoke_table_cache == 0) {
+	if (!jbd2_revoke_table_cache) {
 		kmem_cache_destroy(jbd2_revoke_record_cache);
 		jbd2_revoke_record_cache = NULL;
 		return -ENOMEM;

Re: [PATCH 2/3] jbd2: replace potentially false assertion with if block

[Christoph Hellwig]

On Fri, Mar 07, 2008 at 02:23:36PM -0700, Andreas Dilger wrote:
> It would probably be prudent to verify that these caches are initialized
> at journal_load() time and either re-try to create the cache, and/or report
> an error in that case and refuse to continue mounting.

That doesn't make any sense.  They're initialized in module_init and
destoryed in module_exit.  They can never be zero at journal_load time
unless you get random memory corruption overwriting exactly that pointer
with zero.

Re: [PATCH 2/3] jbd2: replace potentially false assertion with if block

[Duane Griffin]

On 08/03/2008, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Mar 07, 2008 at 02:23:36PM -0700, Andreas Dilger wrote:
>
> > It would probably be prudent to verify that these caches are initialized
>  > at journal_load() time and either re-try to create the cache, and/or report
>  > an error in that case and refuse to continue mounting.
>
> That doesn't make any sense.  They're initialized in module_init and
>  destoryed in module_exit.  They can never be zero at journal_load time
>  unless you get random memory corruption overwriting exactly that pointer
>  with zero.

If jbd is a module, sure, but not if it is statically linked in. I
have the stacktraces to prove it :)

Cheers,
Duane.

-- 
"I never could learn to drink that blood and call it wine" - Bob Dylan

Re: [PATCH 2/3] jbd2: replace potentially false assertion with if block

[Christoph Hellwig]

On Sat, Mar 08, 2008 at 04:40:46PM +0000, Duane Griffin wrote:
> > That doesn't make any sense.  They're initialized in module_init and
> >  destoryed in module_exit.  They can never be zero at journal_load time
> >  unless you get random memory corruption overwriting exactly that pointer
> >  with zero.
> 
> If jbd is a module, sure, but not if it is statically linked in. I
> have the stacktraces to prove it :)

That text above was in reply to Andreas comment about checking it in
journal_load.  Your fix obviously does make sense althrough doing it
differently as in my reply to your first series would be even better.

Re: [PATCH 2/3] jbd2: replace potentially false assertion with if block

[Duane Griffin]

On 08/03/2008, Christoph Hellwig <hch@infradead.org> wrote:
>
> That text above was in reply to Andreas comment about checking it in
>  journal_load.  Your fix obviously does make sense althrough doing it
>  differently as in my reply to your first series would be even better.

Sorry if I'm missing something here, but I think the caches do need to
be checked. If jbd/ext3 are not modular then even if initialisation
fails the journal code may still be called later. I noticed this when
testing the failure modes after making my original fix.

I have some patches ready to go to address this, which I'll send after
this. It turns out journal_load is actually too late to check, though:
journal_init_common is called prior to that and will also blow up if
the caches are uninitialised. I've taken Andreas' suggestion and
attempted to initialise the caches again at that point before failing.

I've modified my changes to match the way you suggested doing things
in your earlier reply (and thanks for the review, BTW). If you would
prefer I'll rework my changes as a separate patch on top. Just let me
know.

Cheers,
Duane.

-- 
"I never could learn to drink that blood and call it wine" - Bob Dylan

Re: [PATCH 2/3] jbd2: replace potentially false assertion with if block

[Christoph Hellwig]

On Sat, Mar 08, 2008 at 06:37:08PM +0000, Duane Griffin wrote:
> Sorry if I'm missing something here, but I think the caches do need to
> be checked. If jbd/ext3 are not modular then even if initialisation
> fails the journal code may still be called later. I noticed this when
> testing the failure modes after making my original fix.

Doh, I think you're right.  Just because jbd initialization fails
ext3 could succeed later on.   We probably want to make ext3
initialization fail early in that case through an exported flag.

> I've modified my changes to match the way you suggested doing things
> in your earlier reply (and thanks for the review, BTW). If you would
> prefer I'll rework my changes as a separate patch on top. Just let me
> know.

I don't really care how it's delivered.  Just consider my patches a 
scetch and do whatever you think is best with them.

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

[Duane Griffin]

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 <duaneg@dghda.com>
---
 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

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

[Duane Griffin]

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 <duaneg@dghda.com>
---
 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

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

[Andreas Dilger]

On Mar 07, 2008  01:31 +0000, Duane Griffin wrote:
> 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 <duaneg@dghda.com>

Acked-by: Andreas Dilger <adilger@sun.com>

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

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

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

[Andreas Dilger]

On Mar 07, 2008  01:31 +0000, Duane Griffin wrote:
> 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.

Duane, thanks for doing the cleanup.  Comments inline.

> Signed-off-by: Duane Griffin <duaneg@dghda.com>
> ---
>  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)
>  {

(minor) calling this "hash_size" would be a bit clearer, and more consistent
with the old code.  Not a reason in itself to redo the patch though.

> +	int shift = 0;
> +	int tmp = size;
>  
>  	while((tmp >>= 1UL) != 0UL)
>  		shift++;
>  
> +	table->hash_size = size;
> +	table->hash_shift = shift;
> +	table->hash_table = kmalloc(
> +		size * sizeof(struct list_head), GFP_KERNEL);

(style) could fit on a single line by removing one space somewhere, or follow
code style and move only "GFP_KERNEL" to the second line...

> +	if (!table->hash_table)
>  		return -ENOMEM;
>  
> +	for (tmp = 0; tmp < size; tmp++)
> +		INIT_LIST_HEAD(&table->hash_table[tmp]);
>  
> +	return 0;
> +}
>  
> +/* 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_table[0] = kmem_cache_alloc(
> +		jbd2_revoke_table_cache, GFP_KERNEL);

(style) it is preferred to indent continuation lines to the previous '(' like:

	journal->j_revoke_table[0] = kmem_cache_alloc(jbd2_revoke_table_cache,
						      GFP_KERNEL);

or alternately:

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

(style) wrap at 80 columns.

> +		goto failed_init1;
>  
> +	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;

(minor) It appears we could reduce some more code duplication by doing
the allocation of j_revoke_table[0] and j_revoke_table[1] inside
journal_init_revoke_table(), passing back the table pointer or NULL on
failure (-ENOMEM is really the only possible error return code here)?

> +	journal->j_revoke = journal->j_revoke_table[1];
>  
>  	spin_lock_init(&journal->j_revoke_lock);
>  
>  	return 0;
>  
> +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;

Doing the table allocation inside journal_init_revoke_table() also
simplifies cleanup, because we don't need to handle "init" and "alloc"
failures separately here.

> +static void jbd2_journal_destroy_revoke_table(struct jbd2_revoke_table_s *table)
>  {
>  	int i;
> +	struct list_head *hash_list;
>  
> +	for (i = 0; i < table->hash_size; i++) {
>  		hash_list = &table->hash_table[i];
> +		J_ASSERT(list_empty(hash_list));
>  	}
>  
>  	kfree(table->hash_table);
>  	kmem_cache_free(jbd2_revoke_table_cache, table);
> +}

(minor) This should be moved above journal_init_revoke_table() and be used
to free the first table if allocation/init of the second table fails.
That is proper encapsulation of functionality, and by moving the table
allocation inside journal_init_revoke_table() as previously suggested,
we never have to handle partially-initialized tables (i.e. alloc, but
list_heads not init.

Sure, it is a bit more overhead than just freeing the arrays, but
performance isn't critical if the mount just failed due to ENOMEM,
and isn't expected to happen very often at all.

> +/* 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;

(style) empty line here.

> +	jbd2_journal_destroy_revoke_table(journal->j_revoke_table[0]);
> +	journal->j_revoke = NULL;
>  
> +	if (!journal->j_revoke_table[1])
> +		return;
> +	jbd2_journal_destroy_revoke_table(journal->j_revoke_table[1]);
>  	journal->j_revoke = NULL;

(style) I'd probably write this as below, to keep the logic simpler:

	journal->j_revoke = NULL;

	if (journal->j_revoke_table[0])
		jbd2_journal_destroy_revoke_table(journal->j_revoke_table[0]);
	if (journal->j_revoke_table[1])
		jbd2_journal_destroy_revoke_table(journal->j_revoke_table[1]);

Also, we don't really need to set journal->j_revoke = NULL twice.

Same of course applies to both versions of the patch.  Hopefully once ext4
has had some chance to bake in the kernel (when people start using it after
the "dev" moniker is removed) and Fedora we can revert back to a single jbd
code base.  There are no incompatible format changes in jbd2 that would be
forced upon ext3 by consolidating the code base, it was just split during
development to avoid destabilizing ext3.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

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

[Mingming Cao]

On Fri, 2008-03-07 at 14:52 -0700, Andreas Dilger wrote:
> On Mar 07, 2008  01:31 +0000, Duane Griffin wrote:
> > 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.
> 
> Duane, thanks for doing the cleanup.  Comments inline.
> 
> > Signed-off-by: Duane Griffin <duaneg@dghda.com>
> > ---
> >  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)
> >  {
> 
> (minor) calling this "hash_size" would be a bit clearer, and more consistent
> with the old code.  Not a reason in itself to redo the patch though.
> 
> > +	int shift = 0;
> > +	int tmp = size;
> >  
> >  	while((tmp >>= 1UL) != 0UL)
> >  		shift++;
> >  
> > +	table->hash_size = size;
> > +	table->hash_shift = shift;
> > +	table->hash_table = kmalloc(
> > +		size * sizeof(struct list_head), GFP_KERNEL);
> 
> (style) could fit on a single line by removing one space somewhere, or follow
> code style and move only "GFP_KERNEL" to the second line...
> 
> > +	if (!table->hash_table)
> >  		return -ENOMEM;
> >  
> > +	for (tmp = 0; tmp < size; tmp++)
> > +		INIT_LIST_HEAD(&table->hash_table[tmp]);
> >  
> > +	return 0;
> > +}
> >  
> > +/* 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_table[0] = kmem_cache_alloc(
> > +		jbd2_revoke_table_cache, GFP_KERNEL);
> 
> (style) it is preferred to indent continuation lines to the previous '(' like:
> 
> 	journal->j_revoke_table[0] = kmem_cache_alloc(jbd2_revoke_table_cache,
> 						      GFP_KERNEL);
> 
> or alternately:
> 
> 	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))
> 
> (style) wrap at 80 columns.
> 

checkpatch.pl catched this...fyi.

> > +		goto failed_init1;
> >  
> > +	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;
> 
> (minor) It appears we could reduce some more code duplication by doing
> the allocation of j_revoke_table[0] and j_revoke_table[1] inside
> journal_init_revoke_table(), passing back the table pointer or NULL on
> failure (-ENOMEM is really the only possible error return code here)?
> 
> > +	journal->j_revoke = journal->j_revoke_table[1];
> >  
> >  	spin_lock_init(&journal->j_revoke_lock);
> >  
> >  	return 0;
> >  
> > +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;
> 
> Doing the table allocation inside journal_init_revoke_table() also
> simplifies cleanup, because we don't need to handle "init" and "alloc"
> failures separately here.
> 
> > +static void jbd2_journal_destroy_revoke_table(struct jbd2_revoke_table_s *table)
> >  {
> >  	int i;
> > +	struct list_head *hash_list;
> >  
> > +	for (i = 0; i < table->hash_size; i++) {
> >  		hash_list = &table->hash_table[i];
> > +		J_ASSERT(list_empty(hash_list));
> >  	}
> >  
> >  	kfree(table->hash_table);
> >  	kmem_cache_free(jbd2_revoke_table_cache, table);
> > +}
> 
> (minor) This should be moved above journal_init_revoke_table() and be used
> to free the first table if allocation/init of the second table fails.
> That is proper encapsulation of functionality, and by moving the table
> allocation inside journal_init_revoke_table() as previously suggested,
> we never have to handle partially-initialized tables (i.e. alloc, but
> list_heads not init.
> 
> Sure, it is a bit more overhead than just freeing the arrays, but
> performance isn't critical if the mount just failed due to ENOMEM,
> and isn't expected to happen very often at all.
> 
> > +/* 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;
> 
> (style) empty line here.
> 
> > +	jbd2_journal_destroy_revoke_table(journal->j_revoke_table[0]);
> > +	journal->j_revoke = NULL;
> >  
> > +	if (!journal->j_revoke_table[1])
> > +		return;
> > +	jbd2_journal_destroy_revoke_table(journal->j_revoke_table[1]);
> >  	journal->j_revoke = NULL;
> 
> (style) I'd probably write this as below, to keep the logic simpler:
> 
> 	journal->j_revoke = NULL;
> 
> 	if (journal->j_revoke_table[0])
> 		jbd2_journal_destroy_revoke_table(journal->j_revoke_table[0]);
> 	if (journal->j_revoke_table[1])
> 		jbd2_journal_destroy_revoke_table(journal->j_revoke_table[1]);
> 
> Also, we don't really need to set journal->j_revoke = NULL twice.
> 
> Same of course applies to both versions of the patch.  Hopefully once ext4
> has had some chance to bake in the kernel (when people start using it after
> the "dev" moniker is removed) and Fedora we can revert back to a single jbd
> code base.  There are no incompatible format changes in jbd2 that would be
> forced upon ext3 by consolidating the code base, it was just split during
> development to avoid destabilizing ext3.
> 

Thanks for reviewing this Andreas.

The patch is already in ext4 candidate patch queue.  FYI.

Mingming
> Cheers, Andreas
> --
> Andreas Dilger
> Sr. Staff Engineer, Lustre Group
> Sun Microsystems of Canada, Inc.
> 
> --
> 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

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

[Duane Griffin]

On Fri, Mar 07, 2008 at 04:05:26PM -0800, Mingming Cao wrote:
> > > +	if (!journal->j_revoke_table[0])
> > > +		goto failed_alloc1;
> > > +	if (jbd2_journal_init_revoke_table(journal->j_revoke_table[0], hash_size))
> > 
> > (style) wrap at 80 columns.
> > 
> 
> checkpatch.pl catched this...fyi.

It did :)

After following recent LKML threads on this I figured it would probably
be more readable to leave it than fix it. I should have mentioned that,
sorry. Anyway, it is no longer an issue after addressing Andreas'
comments.

> Thanks for reviewing this Andreas.

My thanks, too. I really appreciate the quick and thorough review.

> The patch is already in ext4 candidate patch queue.  FYI.

If I can do anything to simplify updating it with revised versions then
let me know.

> Mingming

Cheers,
Duane.

-- 
"I never could learn to drink that blood and call it wine" - Bob Dylan

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

[Duane Griffin]

On Fri, Mar 07, 2008 at 02:52:38PM -0700, Andreas Dilger wrote:
> Duane, thanks for doing the cleanup.  Comments inline.

My pleasure! See my responses below and a revised patch at the end.

> > Signed-off-by: Duane Griffin <duaneg@dghda.com>
> > ---
> >  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)
> >  {
> 
> (minor) calling this "hash_size" would be a bit clearer, and more consistent
> with the old code.  Not a reason in itself to redo the patch though.

No problem, I'll be redoing it to address your other comments anyway.

> > +	int shift = 0;
> > +	int tmp = size;
> >  
> >  	while((tmp >>= 1UL) != 0UL)
> >  		shift++;
> >  
> > +	table->hash_size = size;
> > +	table->hash_shift = shift;
> > +	table->hash_table = kmalloc(
> > +		size * sizeof(struct list_head), GFP_KERNEL);
> 
> (style) could fit on a single line by removing one space somewhere, or follow
> code style and move only "GFP_KERNEL" to the second line...

Unfortunately one space won't do it with size changed to hash_size, so
I'll go for the second option.

> > +	if (!table->hash_table)
> >  		return -ENOMEM;
> >  
> > +	for (tmp = 0; tmp < size; tmp++)
> > +		INIT_LIST_HEAD(&table->hash_table[tmp]);
> >  
> > +	return 0;
> > +}
> >  
> > +/* 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_table[0] = kmem_cache_alloc(
> > +		jbd2_revoke_table_cache, GFP_KERNEL);
> 
> (style) it is preferred to indent continuation lines to the previous '(' like:
> 
> 	journal->j_revoke_table[0] = kmem_cache_alloc(jbd2_revoke_table_cache,
> 						      GFP_KERNEL);
> 
> or alternately:
> 
> 	journal->j_revoke_table[0] =
> 		kmem_cache_alloc(jbd2_revoke_table_cache, GFP_KERNEL);

OK, I'll go for the second option so as not to split the args over two
lines.

> > +	if (!journal->j_revoke_table[0])
> > +		goto failed_alloc1;
> > +	if (jbd2_journal_init_revoke_table(journal->j_revoke_table[0], hash_size))
> 
> (style) wrap at 80 columns.

OK.

> > +		goto failed_init1;
> >  
> > +	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;
> 
> (minor) It appears we could reduce some more code duplication by doing
> the allocation of j_revoke_table[0] and j_revoke_table[1] inside
> journal_init_revoke_table(), passing back the table pointer or NULL on
> failure (-ENOMEM is really the only possible error return code here)?

Indeed, much nicer! And yes, -ENOMEM is the only possible error, unless
I've missed something.

> > +	journal->j_revoke = journal->j_revoke_table[1];
> >  
> >  	spin_lock_init(&journal->j_revoke_lock);
> >  
> >  	return 0;
> >  
> > +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;
> 
> Doing the table allocation inside journal_init_revoke_table() also
> simplifies cleanup, because we don't need to handle "init" and "alloc"
> failures separately here.

Yep.

> > +static void jbd2_journal_destroy_revoke_table(struct jbd2_revoke_table_s *table)
> >  {
> >  	int i;
> > +	struct list_head *hash_list;
> >  
> > +	for (i = 0; i < table->hash_size; i++) {
> >  		hash_list = &table->hash_table[i];
> > +		J_ASSERT(list_empty(hash_list));
> >  	}
> >  
> >  	kfree(table->hash_table);
> >  	kmem_cache_free(jbd2_revoke_table_cache, table);
> > +}
> 
> (minor) This should be moved above journal_init_revoke_table() and be used
> to free the first table if allocation/init of the second table fails.
> That is proper encapsulation of functionality, and by moving the table
> allocation inside journal_init_revoke_table() as previously suggested,
> we never have to handle partially-initialized tables (i.e. alloc, but
> list_heads not init.

Yes, that looks much nicer.

> Sure, it is a bit more overhead than just freeing the arrays, but
> performance isn't critical if the mount just failed due to ENOMEM,
> and isn't expected to happen very often at all.

Certainly not.

> > +/* 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;
> 
> (style) empty line here.

OK.

> > +	jbd2_journal_destroy_revoke_table(journal->j_revoke_table[0]);
> > +	journal->j_revoke = NULL;
> >  
> > +	if (!journal->j_revoke_table[1])
> > +		return;
> > +	jbd2_journal_destroy_revoke_table(journal->j_revoke_table[1]);
> >  	journal->j_revoke = NULL;
> 
> (style) I'd probably write this as below, to keep the logic simpler:
> 
> 	journal->j_revoke = NULL;
> 
> 	if (journal->j_revoke_table[0])
> 		jbd2_journal_destroy_revoke_table(journal->j_revoke_table[0]);
> 	if (journal->j_revoke_table[1])
> 		jbd2_journal_destroy_revoke_table(journal->j_revoke_table[1]);
> 
> Also, we don't really need to set journal->j_revoke = NULL twice.

Yeah. I was being ultra-paranoid about not changing the behaviour, even
in strange corner cases that shouldn't have happened, like
j_revoke_table[1] being initialised when j_revoke_table[0] wasn't.

Which was a bit silly, in retrospect.

> Same of course applies to both versions of the patch.  Hopefully once ext4
> has had some chance to bake in the kernel (when people start using it after
> the "dev" moniker is removed) and Fedora we can revert back to a single jbd
> code base.  There are no incompatible format changes in jbd2 that would be
> forced upon ext3 by consolidating the code base, it was just split during
> development to avoid destabilizing ext3.

I'll send the jbd version out shortly. Would you like this version sent
out again with S-O-B and changelog, or is it OK like this?

Cheers,
Duane.

-- 
"I never could learn to drink that blood and call it wine" - Bob Dylan

diff --git a/fs/jbd2/revoke.c b/fs/jbd2/revoke.c
index df36f42..07e4703 100644
--- a/fs/jbd2/revoke.c
+++ b/fs/jbd2/revoke.c
@@ -196,109 +196,84 @@ 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 struct jbd2_revoke_table_s *jbd2_journal_init_revoke_table(int hash_size)
 {
-	int shift, tmp;
+	int shift = 0;
+	int tmp = hash_size;
+	struct jbd2_revoke_table_s *table;
 
-	J_ASSERT (journal->j_revoke_table[0] == NULL);
+	table = kmem_cache_alloc(jbd2_revoke_table_cache, GFP_KERNEL);
+	if (!table)
+		goto out;
 
-	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 =
+	table->hash_size = hash_size;
+	table->hash_shift = shift;
+	table->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;
-		return -ENOMEM;
+	if (!table->hash_table) {
+		kmem_cache_free(jbd2_revoke_table_cache, table);
+		table = NULL;
+		goto out;
 	}
 
 	for (tmp = 0; tmp < hash_size; tmp++)
-		INIT_LIST_HEAD(&journal->j_revoke->hash_table[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;
+out:
+	return table;
+}
+
+static void jbd2_journal_destroy_revoke_table(struct jbd2_revoke_table_s *table)
+{
+	int i;
+	struct list_head *hash_list;
+
+	for (i = 0; i < table->hash_size; i++) {
+		hash_list = &table->hash_table[i];
+		J_ASSERT(list_empty(hash_list));
 	}
 
-	journal->j_revoke = journal->j_revoke_table[1];
+	kfree(table->hash_table);
+	kmem_cache_free(jbd2_revoke_table_cache, table);
+}
 
-	/* 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] = jbd2_journal_init_revoke_table(hash_size);
+	if (!journal->j_revoke_table[0])
+		goto fail0;
 
-	journal->j_revoke->hash_shift = shift;
+	journal->j_revoke_table[1] = jbd2_journal_init_revoke_table(hash_size);
+	if (!journal->j_revoke_table[1])
+		goto fail1;
 
-	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! */
+fail1:
+	jbd2_journal_destroy_revoke_table(journal->j_revoke_table[0]);
+fail0:
+	return -ENOMEM;
+}
 
+/* Destroy a journal's revoke table.  The table must already be empty! */
 void jbd2_journal_destroy_revoke(journal_t *journal)
 {
-	struct jbd2_revoke_table_s *table;
-	struct list_head *hash_list;
-	int i;
-
-	table = journal->j_revoke_table[0];
-	if (!table)
-		return;
-
-	for (i=0; i<table->hash_size; i++) {
-		hash_list = &table->hash_table[i];
-		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)
-		return;
-
-	for (i=0; i<table->hash_size; i++) {
-		hash_list = &table->hash_table[i];
-		J_ASSERT (list_empty(hash_list));
-	}