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 -
1 file changed, 42 insertions(+), 16 deletions(-)
e2fsck/journal.c | 58 +++++++++++++++++++++++++++++++++++++++---------------
# HG changeset patch
# User [email protected]
# 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" <[email protected]>
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)
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 [email protected]
# 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" <[email protected]>
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;
}
1 file changed, 7 insertions(+)
e2fsck/journal.c | 7 +++++++
# HG changeset patch
# User [email protected]
# 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 <[email protected]>
Signed-off-by: Andreas Dilger <[email protected]>
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
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 [email protected]
# 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 <[email protected]>
Signed-off-by: Andreas Dilger <[email protected]>
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
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.