| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1204934726.14884.63.camel@localhost.localdomain>
Date: Fri, 07 Mar 2008 16:05:26 -0800
From: Mingming Cao <cmm@...ibm.com>
To: Andreas Dilger <adilger@....com>
Cc: Duane Griffin <duaneg@...da.com>, linux-ext4@...r.kernel.org,
linux-kernel@...r.kernel.org, Theodore Tso <tytso@....edu>,
sct@...hat.com, akpm@...ux-foundation.org, adilger@...sterfs.com
Subject: Re: [PATCH 1/3] jbd2: eliminate duplicated code in
revocation table init/destroy functions
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@...da.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@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists