Skip to contentSkip to conversation

e2fsprogs-uninit-fixes.patch

PATCH In review Kalpak Shah 2 participants 3 messages Oct 6, 2008 – Oct 11, 2008

lore
Share & tools
lore
#0 lore raw

PATCH 5/15 e2fsprogs-uninit-fixes.patch

Bug fixes for uninit_bg feature:
- display correct inode number during BG_INO_UNINIT and INOREF_IN_USED errors
- restart e2fsck only once in case of above errors
- initialize bitmaps properly considering whether block group is uninit or not (needed for e2freefrag)

Bug fixes for uninit_bg feature:
- display correct inode number during BG_INO_UNINIT and INOREF_IN_USED errors
- restart e2fsck only once in case of above errors
- initialize bitmaps properly considering uninit or not (needed for e2freefrag)

debugfs/set_fields.c +1 -0
Index: e2fsprogs-1.41.1/debugfs/set_fields.c
===================================================================
@@ -566,6 +566,7 @@ void do_set_block_group_descriptor(int a
 	}
 
 	set_gd = current_fs->group_desc[set_bg];
+	set_sb = *current_fs->super;
 
 	if (ss->func(ss, argv[3]) == 0) {
 		current_fs->group_desc[set_bg] = set_gd;
e2fsck/problem.c +2 -2
Index: e2fsprogs-1.41.1/e2fsck/problem.c
===================================================================
@@ -1301,12 +1301,12 @@ static struct e2fsck_problem problem_tab
 
 	/* Inode found in group where _INODE_UNINIT is set */
 	{ PR_2_INOREF_BG_INO_UNINIT,
-	  N_("@i %i found in @g %g where _INODE_UNINIT is set.  "),
+	  N_("@i %N found in @g %g where _INODE_UNINIT is set.  "),
 	  PROMPT_FIX, PR_PREEN_OK },
 
 	/* Inode found in group unused inodes area */
 	{ PR_2_INOREF_IN_UNUSED,
-	  N_("@i %i found in @g %g unused inodes area.  "),
+	  N_("@i %N found in @g %g unused inodes area.  "),
 	  PROMPT_FIX, PR_PREEN_OK },
 
 	/* Pass 3 errors */
lib/ext2fs/rw_bitmaps.c +48 -4

70 lines not on this page · raw message

misc/mke2fs.c +4 -0
Index: e2fsprogs-1.41.1/misc/mke2fs.c
===================================================================
@@ -274,6 +274,8 @@ _("Warning: the backup superblock/group 
 				ext2fs_group_desc_csum_set(fs, group);
 				fs->super->s_free_blocks_count++;
 			}
+			/* The kernel doesn't need to zero the itable blocks */
+			fs->group_desc[i].bg_flags |= EXT2_BG_INODE_ZEROED;
 		}
 		group_block += fs->super->s_blocks_per_group;
 	}
@@ -2066,6 +2068,8 @@ int main (int argc, char *argv[])
 	}
 no_journal:
 
+	if (!super_only)
+		ext2fs_set_gdt_csum(fs);
 	if (!quiet)
 		printf(_("Writing superblocks and "
 		       "filesystem accounting information: "));
e2fsck/pass2.c +15 -10
Index: e2fsprogs-1.41.1/e2fsck/pass2.c
===================================================================
@@ -153,6 +153,12 @@ void e2fsck_pass2(e2fsck_t ctx)
 						&cd);
 	if (ctx->flags & E2F_FLAG_SIGNAL_MASK || ctx->flags & E2F_FLAG_RESTART)
 		return;
+
+	if (ctx->flags & E2F_FLAG_RESTART_LATER) {
+		ctx->flags |= E2F_FLAG_RESTART;
+		return;
+	}
+
 	if (cd.pctx.errcode) {
 		fix_problem(ctx, PR_2_DBLIST_ITERATE, &cd.pctx);
 		ctx->flags |= E2F_FLAG_ABORT;
@@ -981,32 +987,32 @@ out_htree:
 
 		/*
 		 * Check if the inode was missed out because _INODE_UNINIT
-		 * flag was set or bg_itable_unused was incorrect.
-		 * If that is the case restart e2fsck.
-		 * XXX Optimisations TODO:
-		 * 1. only restart e2fsck once
-		 * 2. only exposed inodes are checked again.
+		 * flag was set or bg_itable_unused was incorrect. If that is
+		 * the case restart e2fsck, but this is done only once after
+		 * checking all directory blocks.
+		 * XXX Optimizations TODO:
+		 * 1. only exposed inodes are checked again.
 		 */
 		if (fs->group_desc[group].bg_flags & EXT2_BG_INODE_UNINIT) {
+			pctx.num = dirent->inode;
 			if (fix_problem(ctx, PR_2_INOREF_BG_INO_UNINIT,
 					&cd->pctx)){
 				fs->group_desc[group].bg_flags &=
 					~EXT2_BG_INODE_UNINIT;
-				ctx->flags |= E2F_FLAG_RESTART |
-					E2F_FLAG_SIGNAL_MASK;
+				ctx->flags |= E2F_FLAG_RESTART_LATER;
 			} else {
 				ext2fs_unmark_valid(fs);
 				if (problem == PR_2_BAD_INO)
 					goto next;
 			}
 		} else if (dirent->inode >= first_unused_inode) {
+			pctx.num = dirent->inode;
 			if (fix_problem(ctx, PR_2_INOREF_IN_UNUSED, &cd->pctx)){
 				fs->group_desc[group].bg_itable_unused = 0;
 				fs->group_desc[group].bg_flags &=
 					~EXT2_BG_INODE_UNINIT;
 				ext2fs_mark_super_dirty(fs);
-				ctx->flags |= E2F_FLAG_RESTART;
-				goto restart_fsck;
+				ctx->flags |= E2F_FLAG_RESTART_LATER;
 			} else {
 				ext2fs_unmark_valid(fs);
 				if (problem == PR_2_BAD_INO)
@@ -1147,7 +1153,6 @@ out_htree:
 	return 0;
 abort_free_dict:
 	ctx->flags |= E2F_FLAG_ABORT;
-restart_fsck:
 	dict_free_nodes(&de_dict);
 	return DIRENT_ABORT;
 }
e2fsck/e2fsck.h +1 -0
Index: e2fsprogs-1.41.1/e2fsck/e2fsck.h
===================================================================
@@ -163,6 +163,7 @@ struct resource_track {
 #define E2F_FLAG_CANCEL		0x0002 /* Cancel signaled */
 #define E2F_FLAG_SIGNAL_MASK	0x0003
 #define E2F_FLAG_RESTART	0x0004 /* Restart signaled */
+#define E2F_FLAG_RESTART_LATER	0x0008 /* Restart after all iterations done */
 
 #define E2F_FLAG_SETJMP_OK	0x0010 /* Setjmp valid for abort */
 
lib/ext2fs/closefs.c +80 -0

86 lines not on this page · raw message

tests/m_uninit/expect.1 +24 -24

116 lines not on this page · raw message

#1 lore raw
Quoted 19 lines · Kalpak Shah

Bug fixes for uninit_bg feature:
- display correct inode number during BG_INO_UNINIT and INOREF_IN_USED errors
- restart e2fsck only once in case of above errors
- initialize bitmaps properly considering whether block group is uninit or not (needed for e2freefrag)

Bug fixes for uninit_bg feature:
- display correct inode number during BG_INO_UNINIT and INOREF_IN_USED errors
- restart e2fsck only once in case of above errors
- initialize bitmaps properly considering uninit or not (needed for e2freefrag)

Index: e2fsprogs-1.41.1/debugfs/set_fields.c
===================================================================

debugfs/set_fields.c void do_set_block_group_descriptor(int a · line 572

Quoted 7 lines
debugfs/set_fields.c +1 -0
@@ -566,6 +566,7 @@ void do_set_block_group_descriptor(int a
 	}
 
 	set_gd = current_fs->group_desc[set_bg];
+	set_sb = *current_fs->super;
 
 	if (ss->func(ss, argv[3]) == 0) {
 		current_fs->group_desc[set_bg] = set_gd;

Why is this needed? None of the table entries used by set_block_group_descriptor requires set_sb to be set?

Quoted 8 lines

Index: e2fsprogs-1.41.1/e2fsck/problem.c
===================================================================

e2fsck/problem.c +0 -0
@@ -1301,12 +1301,12 @@ static struct e2fsck_problem problem_tab
 
 	/* Inode found in group where _INODE_UNINIT is set */
 	{ PR_2_INOREF_BG_INO_UNINIT,

e2fsck/problem.c static struct e2fsck_problem problem_tab · line 1311

Quoted 10 lines
e2fsck/problem.c +2 -2
-	  N_("@i %i found in @g %g where _INODE_UNINIT is set.  "),
+	  N_("@i %N found in @g %g where _INODE_UNINIT is set.  "),
 	  PROMPT_FIX, PR_PREEN_OK },
 
 	/* Inode found in group unused inodes area */
 	{ PR_2_INOREF_IN_UNUSED,
-	  N_("@i %i found in @g %g unused inodes area.  "),
+	  N_("@i %N found in @g %g unused inodes area.  "),
 	  PROMPT_FIX, PR_PREEN_OK },
 

This doesn't look right. %N prints pctx.num, which is set to the directory entry's offset. It looks like %Di is the correct expansion, and maybe something like this:

@E references @i %Di in @g %g where _INODE_UNINIT is set.\n

and

@E references @i %Di in @g %g unused inods area.\n

- Ted

#2 lore raw
In reply to Kalpak Shah
Quoted 7 lines · Kalpak Shah

Bug fixes for uninit_bg feature:
- display correct inode number during BG_INO_UNINIT and INOREF_IN_USED errors
- restart e2fsck only once in case of above errors
- initialize bitmaps properly considering whether block group is uninit or not (needed for e2freefrag)

Quoted 7 lines

Bug fixes for uninit_bg feature:
- display correct inode number during BG_INO_UNINIT and INOREF_IN_USED errors
- restart e2fsck only once in case of above errors
- initialize bitmaps properly considering uninit or not (needed for e2freefrag)

Note: While I really do very much appreciate your diffs against the luster e2fsprogs tree, in the future, it would be *really* helpful if you could separate out patches so that each patch does only one thing. It makes it much easier to audit patches, and then if I end up taking some parts of a patch, but not all of it, it becomes easier on your end to match up what I've taken from what I haven't taken. There are some hunks of this patch I which I will probably never take, such as the debugfs/set_fields.c patch, which as far as I can tell serves no purpose, and the addition of ext2fs_super_and_bgd_loc2() in lib/ext2fs/closefs.c (which as near as I can tell was taken from an earlier version of jose's 64-bit patches):

Quoted 4 lines

+ * XXX: Remove this when we upgrade our e2fsprogs patches. Also blk64_t has been
+ * changed to blk_t here since this version does not have 64-bit support.
+ */
+errcode_t ext2fs_super_and_bgd_loc2(ext2_filsys fs,

If this had been separated into a second patch, it becomes easier to drop that one patch hunk from the patch.

Anyway, just a suggestion....

I will be trying to separate out a few of the bug fixes for the maint branch for an e2fsprogs 1.41.3 release, but most of this is going to be reserved for later in the master branch, after quite a bit of refinement.

- Ted