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 <[email protected]>
Acked-by: Andreas Dilger <[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
>
> --
> 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
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
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;
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.
On 08/03/2008, Christoph Hellwig <[email protected]> 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
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.
On 08/03/2008, Christoph Hellwig <[email protected]> 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
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.