lkml.org 
[lkml]   [2001]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: Bug in ext3
    Hi,

    On Thu, Nov 15, 2001 at 04:21:49PM -0700, Andreas Dilger wrote:

    > I don't disagree that something corrupted it, but it is hard to tell from
    > here what it could be. Looking at ext3_read_super(), it is pretty much
    > a read-only path, except journal recovery. If, for some reason, you had
    > an old, unrecovered ext3 journal in the fs, it is possible that recovering
    > from it would corrupt your fs by writing old data into the fs.

    Except that can only happen once we have found a journal to recover,
    and the error that ext3 spat out indicated that it couldn't find a
    journal at all and was giving up.

    > Looking at the ext3_read_super() path, we call ext3_load_journal(),
    > which calls journal_wipe() and journal_load(), which both call
    > journal_load()->load_superblock()->journal_get_superblock(), which
    > gives us the two "JBD: no valid journal superblock found" messages,
    > and return errors before doing anything else. Then we get the message
    > "EXT3-fs: error loading journal." and return without mounting the fs.

    Indeed.

    > Hmm, there is a possibility that journal_destroy() calling
    > journal_update_superblock() scribbling data into the first block of
    > the old "journal". Stephen, Andrew, we need to exit from the
    > journal_get_superblock() with j_sb_buffer = NULL, and then check for
    > this in journal_destroy() so we don't call journal_update_superblock().
    > How does the below patch look?

    Looks OK. I've done a slightly better version which catches a couple
    of extra cases but it's basically the same solution. I've also added
    a tiny patch to prevent a failed journal_wipe() from being followed by
    a journal_load() attempt, so we don't get the same error twice.

    Patch below.

    Cheers,
    Stephen
    Index: fs/ext3/super.c
    ===================================================================
    RCS file: /cvsroot/gkernel/ext3/fs/ext3/super.c,v
    retrieving revision 1.34.2.11
    diff -u -r1.34.2.11 super.c
    --- fs/ext3/super.c 2001/10/27 03:34:03 1.34.2.11
    +++ fs/ext3/super.c 2001/11/16 18:30:39
    @@ -1350,7 +1350,7 @@
    journal_t *journal;
    int journal_inum = le32_to_cpu(es->s_journal_inum);
    int journal_dev = le32_to_cpu(es->s_journal_dev);
    - int err;
    + int err = 0;
    int really_read_only;

    really_read_only = is_read_only(sb->s_dev);
    @@ -1400,9 +1400,10 @@
    }

    if (!EXT3_HAS_INCOMPAT_FEATURE(sb, EXT3_FEATURE_INCOMPAT_RECOVER))
    - journal_wipe(journal, !really_read_only);
    + err = journal_wipe(journal, !really_read_only);
    + if (!err)
    + err = journal_load(journal);

    - err = journal_load(journal);
    if (err) {
    printk(KERN_ERR "EXT3-fs: error loading journal.\n");
    journal_destroy(journal);
    Index: fs/jbd/journal.c
    ===================================================================
    RCS file: /cvsroot/gkernel/ext3/fs/jbd/journal.c,v
    retrieving revision 1.49.2.6
    diff -u -r1.49.2.6 journal.c
    --- fs/jbd/journal.c 2001/11/16 14:45:21 1.49.2.6
    +++ fs/jbd/journal.c 2001/11/16 18:30:42
    @@ -773,6 +773,18 @@
    return journal;
    }

    +/*
    + * If the journal init or create aborts, we need to mark the journal
    + * superblock as being NULL to prevent the journal destroy from writing
    + * back a bogus superblock.
    + */
    +static void journal_fail_superblock (journal_t *journal)
    +{
    + struct buffer_head *bh = journal->j_sb_buffer;
    + brelse(bh);
    + journal->j_sb_buffer = NULL;
    +}
    +
    /*
    * Given a journal_t structure, initialise the various fields for
    * startup of a new journaling session. We use this both when creating
    @@ -826,6 +838,7 @@
    if (journal->j_maxlen < JFS_MIN_JOURNAL_BLOCKS) {
    printk (KERN_ERR "Journal length (%d blocks) too short.\n",
    journal->j_maxlen);
    + journal_fail_superblock(journal);
    return -EINVAL;
    }

    @@ -916,7 +929,8 @@
    {
    struct buffer_head *bh;
    journal_superblock_t *sb;
    -
    + int err = -EIO;
    +
    bh = journal->j_sb_buffer;

    J_ASSERT(bh != NULL);
    @@ -926,16 +940,18 @@
    if (!buffer_uptodate(bh)) {
    printk (KERN_ERR
    "JBD: IO error reading journal superblock\n");
    - return -EIO;
    + goto out;
    }
    }

    sb = journal->j_superblock;

    + err = -EINVAL;
    +
    if (sb->s_header.h_magic != htonl(JFS_MAGIC_NUMBER) ||
    sb->s_blocksize != htonl(journal->j_blocksize)) {
    printk(KERN_WARNING "JBD: no valid journal superblock found\n");
    - return -EINVAL;
    + goto out;
    }

    switch(ntohl(sb->s_header.h_blocktype)) {
    @@ -947,17 +963,21 @@
    break;
    default:
    printk(KERN_WARNING "JBD: unrecognised superblock format ID\n");
    - return -EINVAL;
    + goto out;
    }

    if (ntohl(sb->s_maxlen) < journal->j_maxlen)
    journal->j_maxlen = ntohl(sb->s_maxlen);
    else if (ntohl(sb->s_maxlen) > journal->j_maxlen) {
    printk (KERN_WARNING "JBD: journal file too short\n");
    - return -EINVAL;
    + goto out;
    }

    return 0;
    +
    +out:
    + journal_fail_superblock(journal);
    + return err;
    }

    /*
    @@ -1062,7 +1082,10 @@
    /* We can now mark the journal as empty. */
    journal->j_tail = 0;
    journal->j_tail_sequence = ++journal->j_transaction_sequence;
    - journal_update_superblock(journal, 1);
    + if (journal->j_sb_buffer) {
    + journal_update_superblock(journal, 1);
    + brelse(journal->j_sb_buffer);
    + }

    if (journal->j_inode)
    iput(journal->j_inode);
    @@ -1070,7 +1093,6 @@
    journal_destroy_revoke(journal);

    unlock_journal(journal);
    - brelse(journal->j_sb_buffer);
    kfree(journal);
    MOD_DEC_USE_COUNT;
    }
    \
     
     \ /
      Last update: 2005-03-22 13:13    [W:0.031 / U:30.684 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site