[PATCH 0 of 4] Check journal inode sanity and recreate journal

[PATCH 0 of 4] Check journal inode sanity and recreate journal

[Theodore Tso]

Hi Kalpak,

        I've broken up your patch and rewritten portions of them to
clean them up.  As I've mentioned before, smaller patches are easier to
review and to cherry pick as necessary.

The changes from your patches are:

1) A smaller set of functionality (just returning the recommended
   default size of the journal) was moved into the library.  This
   eliminated the need to create a number of new libext2fs error codes,
   and it minimized the changes needed to tune2fs and mke2fs.

2) Your changes to sanity check the journal blocks beyond the location
   of the indirect blocks used journal_bmap(); unfortunately
   journal_bmap doesn't verify the validity of the indirect/double
   indirect blocks.  So, I rewrote the checks to use
   ext2fs_block_iterate() instead, which was designed for use by e2fsck.
   It basically was a (very) simplified version of check_blocks() from
   pass1.c.   Your patch also didn't remove the original code which just
   verified the direct blocks, which was no longer necessary, and
   performed the check after the backup blocks and be written out to the
   journal, instead of before deciding to overwrite the journal inode
   with the backup information from the superblock.

Regards,

                                        - Ted

11 files changed, 144 insertions(+), 39 deletions(-)
e2fsck/journal.c              |   67 ++++++++++++++++++++++++++++++-----------
e2fsck/problem.c              |    5 +++
e2fsck/problem.h              |    7 ++++
e2fsck/unix.c                 |   40 ++++++++++++++++++++++++
lib/ext2fs/ext2fs.h           |    1 
lib/ext2fs/mkjournal.c        |   20 ++++++++++++
misc/util.c                   |   23 +++-----------
tests/f_badjourblks/expect.1  |    8 ++++
tests/f_badjourblks/expect.2  |    2 -
tests/f_miss_journal/expect.1 |    8 ++++
tests/f_miss_journal/expect.2 |    2 -

[PATCH 1 of 4] Add default journal size function

[Theodore Tso]

3 files changed, 26 insertions(+), 18 deletions(-)
lib/ext2fs/ext2fs.h    |    1 +
lib/ext2fs/mkjournal.c |   20 ++++++++++++++++++++
misc/util.c            |   23 +++++------------------


# HG changeset patch
# User tytso@mit.edu
# Date 1182441546 14400
# Node ID 5daddb24f35d514c4d21a7d79bd470439b74866e
# Parent  ee079e7cb509d2da74a65411e670efaa5e88850a
Add default journal size function

Factor out the code which sets the default journal size and move it
into libext2fs.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff -r ee079e7cb509 -r 5daddb24f35d lib/ext2fs/ext2fs.h
--- a/lib/ext2fs/ext2fs.h	Tue Jun 19 03:29:47 2007 -0400
+++ b/lib/ext2fs/ext2fs.h	Thu Jun 21 11:59:06 2007 -0400
@@ -881,6 +881,7 @@ extern errcode_t ext2fs_add_journal_devi
 					   ext2_filsys journal_dev);
 extern errcode_t ext2fs_add_journal_inode(ext2_filsys fs, blk_t size,
 					  int flags);
+extern int ext2fs_default_journal_size(__u64 blocks);
 
 /* openfs.c */
 extern errcode_t ext2fs_open(const char *name, int flags, int superblock,
diff -r ee079e7cb509 -r 5daddb24f35d lib/ext2fs/mkjournal.c
--- a/lib/ext2fs/mkjournal.c	Tue Jun 19 03:29:47 2007 -0400
+++ b/lib/ext2fs/mkjournal.c	Thu Jun 21 11:59:06 2007 -0400
@@ -248,6 +248,26 @@ errout:
 }
 
 /*
+ * Find a reasonable journal file size (in blocks) given the number of blocks
+ * in the filesystem.  For very small filesystems, it is not reasonable to
+ * have a journal that fills more than half of the filesystem.
+ */
+int ext2fs_default_journal_size(__u64 blocks)
+{
+	if (blocks < 2048)
+		return -1;
+	if (blocks < 32768)
+		return (1024);
+	if (blocks < 256*1024)
+		return (4096);
+	if (blocks < 512*1024)
+		return (8192);
+	if (blocks < 1024*1024)
+		return (16384);
+	return 32768;
+}
+
+/*
  * This function adds a journal device to a filesystem
  */
 errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
diff -r ee079e7cb509 -r 5daddb24f35d misc/util.c
--- a/misc/util.c	Tue Jun 19 03:29:47 2007 -0400
+++ b/misc/util.c	Thu Jun 21 11:59:06 2007 -0400
@@ -251,9 +251,10 @@ void parse_journal_opts(const char *opts
  */
 int figure_journal_size(int size, ext2_filsys fs)
 {
-	blk_t j_blocks;
-
-	if (fs->super->s_blocks_count < 2048) {
+	int j_blocks;
+
+	j_blocks = ext2fs_default_journal_size(fs->super->s_blocks_count);
+	if (j_blocks < 0) {
 		fputs(_("\nFilesystem too small for a journal\n"), stderr);
 		return 0;
 	}
@@ -273,21 +274,7 @@ int figure_journal_size(int size, ext2_f
 			      stderr);
 			exit(1);
 		}
-		return j_blocks;
-	}
-
-	if (fs->super->s_blocks_count < 32768)
-		j_blocks = 1400;
-	else if (fs->super->s_blocks_count < 256*1024)
-		j_blocks = 4096;
-	else if (fs->super->s_blocks_count < 512*1024)
-		j_blocks = 8192;
-	else if (fs->super->s_blocks_count < 1024*1024)
-		j_blocks = 16384;
-	else
-		j_blocks = 32768;
-
-
+	}
 	return j_blocks;
 }
 

[PATCH 2 of 4] Recreate journal that had been removed previously due to corruption

[Theodore Tso]

8 files changed, 69 insertions(+), 5 deletions(-)
e2fsck/journal.c              |    2 +-
e2fsck/problem.c              |    5 +++++
e2fsck/problem.h              |    7 +++++++
e2fsck/unix.c                 |   40 ++++++++++++++++++++++++++++++++++++++++
tests/f_badjourblks/expect.1  |    8 +++++++-
tests/f_badjourblks/expect.2  |    2 +-
tests/f_miss_journal/expect.1 |    8 +++++++-
tests/f_miss_journal/expect.2 |    2 +-


# HG changeset patch
# User tytso@mit.edu
# Date 1182441546 14400
# Node ID 8c5c3cc3fa06ab7791d1a55c3191a4e63a32c8a5
# Parent  5daddb24f35d514c4d21a7d79bd470439b74866e
Recreate journal that had been removed previously due to corruption

If the journal had been removed because it was corrupt, the
E2F_FLAG_JOURNAL_INODE flag will be set.  If this flag is set, then
recreate the filesystem after checking the filesystem.

Signed-off-by: Kalpak Shah <kalpak@clusterfs.com>
Signed-off-by: Andreas Dilger <adilger@clusterfs.com>

diff -r 5daddb24f35d -r 8c5c3cc3fa06 e2fsck/journal.c
--- a/e2fsck/journal.c	Thu Jun 21 11:59:06 2007 -0400
+++ b/e2fsck/journal.c	Thu Jun 21 11:59:06 2007 -0400
@@ -420,7 +420,7 @@ static errcode_t e2fsck_journal_fix_bad_
 				       "filesystem is now ext2 only ***\n\n");
 			sb->s_feature_compat &= ~EXT3_FEATURE_COMPAT_HAS_JOURNAL;
 			sb->s_journal_inum = 0;
-			ctx->flags |= E2F_FLAG_JOURNAL_INODE; /* FIXME: todo */
+			ctx->flags |= E2F_FLAG_JOURNAL_INODE;
 			e2fsck_clear_recover(ctx, 1);
 			return 0;
 		}
diff -r 5daddb24f35d -r 8c5c3cc3fa06 e2fsck/problem.c
--- a/e2fsck/problem.c	Thu Jun 21 11:59:06 2007 -0400
+++ b/e2fsck/problem.c	Thu Jun 21 11:59:06 2007 -0400
@@ -1493,6 +1493,11 @@ static struct e2fsck_problem problem_tab
 	{ PR_5_INODE_RANGE_USED,
 	  " +(%i--%j)",
 	  PROMPT_NONE, PR_LATCH_IBITMAP | PR_PREEN_OK | PR_PREEN_NOMSG },
+
+	/* Recreate journal if E2F_FLAG_JOURNAL_INODE flag is set */
+	{ PR_6_RECREATE_JOURNAL,
+	  N_("Recreate journal to make the filesystem ext3 again?\n"),
+	  PROMPT_FIX, PR_PREEN_OK | PR_NO_OK },
 
 	{ 0 }
 };
diff -r 5daddb24f35d -r 8c5c3cc3fa06 e2fsck/problem.h
--- a/e2fsck/problem.h	Thu Jun 21 11:59:06 2007 -0400
+++ b/e2fsck/problem.h	Thu Jun 21 11:59:06 2007 -0400
@@ -901,6 +901,13 @@ struct problem_context {
 #define PR_5_INODE_RANGE_USED		0x050017
 
 /*
+ * Post-Pass 5 errors
+ */
+
+/* Recreate the journal if E2F_FLAG_JOURNAL_INODE flag is set */
+#define PR_6_RECREATE_JOURNAL		0x060001
+
+/*
  * Function declarations
  */
 int fix_problem(e2fsck_t ctx, problem_t code, struct problem_context *pctx);
diff -r 5daddb24f35d -r 8c5c3cc3fa06 e2fsck/unix.c
--- a/e2fsck/unix.c	Thu Jun 21 11:59:06 2007 -0400
+++ b/e2fsck/unix.c	Thu Jun 21 11:59:06 2007 -0400
@@ -851,6 +851,7 @@ int main (int argc, char *argv[])
 	e2fsck_t	ctx;
 	struct problem_context pctx;
 	int flags, run_result;
+	int journal_size;
 	
 	clear_problem_context(&pctx);
 #ifdef MTRACE
@@ -1184,8 +1185,47 @@ restart:
 			 " but we'll try to go on...\n"));
 	}
 
+	/*
+	 * Save the journal size in megabytes.
+	 * Try and use the journal size from the backup else let e2fsck
+	 * find the default journal size.
+	 */
+	if (sb->s_jnl_backup_type == EXT3_JNL_BACKUP_BLOCKS)
+		journal_size = sb->s_jnl_blocks[16] >> 20;
+	else
+		journal_size = -1;
+
 	run_result = e2fsck_run(ctx);
 	e2fsck_clear_progbar(ctx);
+
+	if (ctx->flags & E2F_FLAG_JOURNAL_INODE) {
+		if (fix_problem(ctx, PR_6_RECREATE_JOURNAL, &pctx)) {
+			if (journal_size < 1024)
+				journal_size = ext2fs_default_journal_size(fs->super->s_blocks_count);
+			if (journal_size < 0) {
+				fs->super->s_feature_compat &=
+					~EXT3_FEATURE_COMPAT_HAS_JOURNAL;
+				com_err(ctx->program_name, 0, 
+					_("Couldn't determine journal size"));
+				goto no_journal;
+			}
+			printf(_("Creating journal (%d blocks): "),
+			       journal_size);
+			fflush(stdout);
+			retval = ext2fs_add_journal_inode(fs,
+							  journal_size, 0);
+			if (retval) {
+				com_err("Error ", retval,
+					_("\n\twhile trying to create journal"));
+				goto no_journal;
+			}
+			printf(_(" Done.\n"));
+			printf(_("\n*** journal has been re-created - "
+				       "filesystem is now ext3 again ***\n"));
+		}
+	}
+no_journal:
+
 	if (run_result == E2F_FLAG_RESTART) {
 		printf(_("Restarting e2fsck from the beginning...\n"));
 		retval = e2fsck_reset_context(ctx);
diff -r 5daddb24f35d -r 8c5c3cc3fa06 tests/f_badjourblks/expect.1
--- a/tests/f_badjourblks/expect.1	Thu Jun 21 11:59:06 2007 -0400
+++ b/tests/f_badjourblks/expect.1	Thu Jun 21 11:59:06 2007 -0400
@@ -19,7 +19,13 @@ Free blocks count wrong (7112, counted=8
 Free blocks count wrong (7112, counted=8142).
 Fix? yes
 
+Recreate journal to make the filesystem ext3 again?
+Fix? yes
+
+Creating journal (1024 blocks):  Done.
+
+*** journal has been re-created - filesystem is now ext3 again ***
 
 test_filesys: ***** FILE SYSTEM WAS MODIFIED *****
-test_filesys: 11/256 files (0.0% non-contiguous), 50/8192 blocks
+test_filesys: 11/256 files (0.0% non-contiguous), 1080/8192 blocks
 Exit status is 1
diff -r 5daddb24f35d -r 8c5c3cc3fa06 tests/f_badjourblks/expect.2
--- a/tests/f_badjourblks/expect.2	Thu Jun 21 11:59:06 2007 -0400
+++ b/tests/f_badjourblks/expect.2	Thu Jun 21 11:59:06 2007 -0400
@@ -3,5 +3,5 @@ Pass 3: Checking directory connectivity
 Pass 3: Checking directory connectivity
 Pass 4: Checking reference counts
 Pass 5: Checking group summary information
-test_filesys: 11/256 files (0.0% non-contiguous), 50/8192 blocks
+test_filesys: 11/256 files (0.0% non-contiguous), 1080/8192 blocks
 Exit status is 0
diff -r 5daddb24f35d -r 8c5c3cc3fa06 tests/f_miss_journal/expect.1
--- a/tests/f_miss_journal/expect.1	Thu Jun 21 11:59:06 2007 -0400
+++ b/tests/f_miss_journal/expect.1	Thu Jun 21 11:59:06 2007 -0400
@@ -17,7 +17,13 @@ Free blocks count wrong (968, counted=19
 Free blocks count wrong (968, counted=1998).
 Fix? yes
 
+Recreate journal to make the filesystem ext3 again?
+Fix? yes
+
+Creating journal (1024 blocks):  Done.
+
+*** journal has been re-created - filesystem is now ext3 again ***
 
 test_filesys: ***** FILE SYSTEM WAS MODIFIED *****
-test_filesys: 11/256 files (0.0% non-contiguous), 50/2048 blocks
+test_filesys: 11/256 files (0.0% non-contiguous), 1080/2048 blocks
 Exit status is 1
diff -r 5daddb24f35d -r 8c5c3cc3fa06 tests/f_miss_journal/expect.2
--- a/tests/f_miss_journal/expect.2	Thu Jun 21 11:59:06 2007 -0400
+++ b/tests/f_miss_journal/expect.2	Thu Jun 21 11:59:06 2007 -0400
@@ -3,5 +3,5 @@ Pass 3: Checking directory connectivity
 Pass 3: Checking directory connectivity
 Pass 4: Checking reference counts
 Pass 5: Checking group summary information
-test_filesys: 11/256 files (0.0% non-contiguous), 50/2048 blocks
+test_filesys: 11/256 files (0.0% non-contiguous), 1080/2048 blocks
 Exit status is 0

[PATCH 3 of 4] Write the updated journal inode if s_jnl_blocks was successfully used

[Theodore Tso]

1 file changed, 7 insertions(+)
e2fsck/journal.c |    7 +++++++


# HG changeset patch
# User tytso@mit.edu
# Date 1182441546 14400
# Node ID 7a1859d26e58056688629ee2faf2f2ee619a1bf1
# Parent  8c5c3cc3fa06ab7791d1a55c3191a4e63a32c8a5
Write the updated journal inode if s_jnl_blocks was successfully used

If the journal inode was corrected from s_jnl_blocks, write the fixed
journal inode back to disk.

Signed-off-by: Kalpak Shah <kalpak@clusterfs.com>
Signed-off-by: Andreas Dilger <adilger@clusterfs.com>

diff -r 8c5c3cc3fa06 -r 7a1859d26e58 e2fsck/journal.c
--- a/e2fsck/journal.c	Thu Jun 21 11:59:06 2007 -0400
+++ b/e2fsck/journal.c	Thu Jun 21 11:59:06 2007 -0400
@@ -285,6 +285,13 @@ static errcode_t e2fsck_get_journal(e2fs
 				goto try_backup_journal;
 			}
 		}
+		if (tried_backup_jnl && !(ctx->options & E2F_OPT_READONLY)) {
+			retval = ext2fs_write_inode(ctx->fs, sb->s_journal_inum,
+						    &j_inode->i_ext2);
+			if (retval)
+				goto errout;
+		}
+
 		journal->j_maxlen = j_inode->i_ext2.i_size / journal->j_blocksize;
 
 #ifdef USE_INODE_IO

[PATCH 4 of 4] e2fsck: Check the all of journal blocks for validity

[Theodore Tso]

1 file changed, 42 insertions(+), 16 deletions(-)
e2fsck/journal.c |   58 +++++++++++++++++++++++++++++++++++++++---------------


# HG changeset patch
# User tytso@mit.edu
# Date 1182441546 14400
# Node ID f0da9dbdd0751be62a0e3b3afeec85ab27a13d20
# Parent  7a1859d26e58056688629ee2faf2f2ee619a1bf1
e2fsck: Check the all of journal blocks for validity

The original code only checked the direct blocks to make sure the
journal inode was sane.  Unfortunately, if some or all of the indirect
or doubly indirect blocks were corrupted, this would not be caught.
Thanks to Andreas Dilger and Kalpak Shah for noticing this problem.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff -r 7a1859d26e58 -r f0da9dbdd075 e2fsck/journal.c
--- a/e2fsck/journal.c	Thu Jun 21 11:59:06 2007 -0400
+++ b/e2fsck/journal.c	Thu Jun 21 11:59:06 2007 -0400
@@ -189,8 +189,37 @@ static void e2fsck_clear_recover(e2fsck_
 	ext2fs_mark_super_dirty(ctx->fs);
 }
 
+/*
+ * This is a helper function to check the validity of the journal.
+ */
+struct process_block_struct {
+	e2_blkcnt_t	last_block;
+};
+
+static int process_journal_block(ext2_filsys fs,
+				 blk_t	*block_nr,
+				 e2_blkcnt_t blockcnt,
+				 blk_t ref_block EXT2FS_ATTR((unused)),
+				 int ref_offset EXT2FS_ATTR((unused)),
+				 void *priv_data)
+{
+	struct process_block_struct *p;
+	blk_t	blk = *block_nr;
+
+	p = (struct process_block_struct *) priv_data;
+
+	if (blk < fs->super->s_first_data_block ||
+	    blk >= fs->super->s_blocks_count)
+		return BLOCK_ABORT;
+
+	if (blockcnt >= 0)
+		p->last_block = blockcnt;
+	return 0;
+}
+
 static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
 {
+	struct process_block_struct pb;
 	struct ext2_super_block *sb = ctx->fs->super;
 	struct ext2_super_block jsuper;
 	struct problem_context	pctx;
@@ -202,10 +231,8 @@ static errcode_t e2fsck_get_journal(e2fs
 	errcode_t		retval = 0;
 	io_manager		io_ptr = 0;
 	unsigned long		start = 0;
-	blk_t			blk;
 	int			ext_journal = 0;
 	int			tried_backup_jnl = 0;
-	int			i;
 
 	clear_problem_context(&pctx);
 
@@ -258,6 +285,9 @@ static errcode_t e2fsck_get_journal(e2fs
 			j_inode->i_ext2.i_size = sb->s_jnl_blocks[16];
 			j_inode->i_ext2.i_links_count = 1;
 			j_inode->i_ext2.i_mode = LINUX_S_IFREG | 0600;
+			e2fsck_use_inode_shortcuts(ctx, 1);
+			ctx->stashed_ino = j_inode->i_ino;
+			ctx->stashed_inode = &j_inode->i_ext2;
 			tried_backup_jnl++;
 		}
 		if (!j_inode->i_ext2.i_links_count ||
@@ -270,20 +300,14 @@ static errcode_t e2fsck_get_journal(e2fs
 			retval = EXT2_ET_JOURNAL_TOO_SMALL;
 			goto try_backup_journal;
 		}
-		for (i=0; i < EXT2_N_BLOCKS; i++) {
-			blk = j_inode->i_ext2.i_block[i];
-			if (!blk) {
-				if (i < EXT2_NDIR_BLOCKS) {
-					retval = EXT2_ET_JOURNAL_TOO_SMALL;
-					goto try_backup_journal;
-				}
-				continue;
-			}
-			if (blk < sb->s_first_data_block ||
-			    blk >= sb->s_blocks_count) {
-				retval = EXT2_ET_BAD_BLOCK_NUM;
-				goto try_backup_journal;
-			}
+		pb.last_block = -1;
+		retval = ext2fs_block_iterate2(ctx->fs, j_inode->i_ino,
+					       BLOCK_FLAG_HOLE, 0, 
+					       process_journal_block, &pb);
+		if ((pb.last_block+1) * ctx->fs->blocksize < 
+		    j_inode->i_ext2.i_size) {
+			retval = EXT2_ET_JOURNAL_TOO_SMALL;
+			goto try_backup_journal;
 		}
 		if (tried_backup_jnl && !(ctx->options & E2F_OPT_READONLY)) {
 			retval = ext2fs_write_inode(ctx->fs, sb->s_journal_inum,
@@ -397,9 +421,11 @@ static errcode_t e2fsck_get_journal(e2fs
 #endif
 
 	*ret_journal = journal;
+	e2fsck_use_inode_shortcuts(ctx, 0);
 	return 0;
 
 errout:
+	e2fsck_use_inode_shortcuts(ctx, 0);
 	if (dev_fs)
 		ext2fs_free_mem(&dev_fs);
 	if (j_inode)

Re: [PATCH 0 of 4] Check journal inode sanity and recreate journal

[Kalpak Shah]

On Thu, 2007-06-21 at 13:00 -0400, Theodore Tso wrote:
> Hi Kalpak,
> 
>         I've broken up your patch and rewritten portions of them to
> clean them up.  As I've mentioned before, smaller patches are easier to
> review and to cherry pick as necessary.

Thanks. Will certainly split the patches whenever possible while
submitting them henceforth.

Thanks,
Kalpak.