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 <[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)
> {
(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.
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 <[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)
> > {
>
> (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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[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)
> > {
>
> (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));
- }
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