Re: ext3: bogus i_mode errors with 2.6.18.1

[Andre Noll <maan@systemlinux.org>]

[-- Attachment #1: Type: text/plain, Size: 6797 bytes --]

On 22:24, Andreas Dilger wrote:
> Well, it needs to also handle backup superblock, bitmaps, inode table:
> 
> 	if (ext3_bg_has_super())
> 		ext3_set_bit(0, gdp_bh->b_data);
> 	gdblocks = ext3_bg_num_gdb(sb, group);
> 	for (i = 0, bit = 1; i < gdblocks; i++, bit++)
> 		/* actually a bit more complex for INCOMPAT_META_BG fs */
> 		ext3_set_bit(i, gdp_bh->b_data);
> 	ext3_set_bit(gdp->bg_inode_bitmap % EXT3_BLOCKS_PER_GROUP(sb), ...);
> 	ext3_set_bit(gdp->bg_block_bitmap % EXT3_BLOCKS_PER_GROUP(sb), ...);
> 	for (i = 0, bit = gdp->bg_inode_table % EXT3_BLOCKS_PER_GROUP(sb);
> 	     i < EXT3_SB(sb)->s_itb_per_group; i++, bit++)
> 		ext3_set_bit(i, gdp_bh->b_data);
> 		
> (or something close to this).

OK. So here's a new version of the patch. I moved the check whether a
block needs fixing into an inline function, block_needs_fix(), and
introduced a new function fix_group() that sets the bits in
gdp_bh->b_data. Note that the patch does not address the
EXT3_FEATURE_INCOMPAT_META_BG case yet. I'll have to look at this in
more detail.

BTW: Currently e2fsck is running on the file system which suffered from
the bug this patch tries to fix. It looks like the file system is
completely busted as e2fsck is spitting out zillions of 

	"Too many illegal blocks" 
	"Inode xxxxx has compression flag set on filesystem without compression support."
	"Inode xxxxx has INDEX_FL flag set but is not a directory"

Thanks
Andre


diff --git a/fs/ext3/balloc.c b/fs/ext3/balloc.c
index 063d994..cc29e5b 100644
--- a/fs/ext3/balloc.c
+++ b/fs/ext3/balloc.c
@@ -359,17 +359,6 @@ do_more:
 	if (!desc)
 		goto error_return;
 
-	if (in_range (le32_to_cpu(desc->bg_block_bitmap), block, count) ||
-	    in_range (le32_to_cpu(desc->bg_inode_bitmap), block, count) ||
-	    in_range (block, le32_to_cpu(desc->bg_inode_table),
-		      sbi->s_itb_per_group) ||
-	    in_range (block + count - 1, le32_to_cpu(desc->bg_inode_table),
-		      sbi->s_itb_per_group))
-		ext3_error (sb, "ext3_free_blocks",
-			    "Freeing blocks in system zones - "
-			    "Block = "E3FSBLK", count = %lu",
-			    block, count);
-
 	/*
 	 * We are about to start releasing blocks in the bitmap,
 	 * so we need undo access.
@@ -392,7 +381,17 @@ do_more:
 
 	jbd_lock_bh_state(bitmap_bh);
 
-	for (i = 0, group_freed = 0; i < count; i++) {
+	for (i = 0, group_freed = 0; i < count; i++, block++) {
+		struct ext3_group_desc *gdp = ext3_get_group_desc(sb, i, NULL);
+		if (block == le32_to_cpu(gdp->bg_block_bitmap) ||
+			block == le32_to_cpu(gdp->bg_inode_bitmap) ||
+			in_range(block, le32_to_cpu(gdp->bg_inode_table),
+				EXT3_SB(sb)->s_itb_per_group)) {
+			ext3_error(sb, __FUNCTION__,
+				"Freeing block in system zone - block = %lu",
+				block);
+			continue;
+		}
 		/*
 		 * An HJ special.  This is expensive...
 		 */
@@ -400,7 +399,7 @@ do_more:
 		jbd_unlock_bh_state(bitmap_bh);
 		{
 			struct buffer_head *debug_bh;
-			debug_bh = sb_find_get_block(sb, block + i);
+			debug_bh = sb_find_get_block(sb, block);
 			if (debug_bh) {
 				BUFFER_TRACE(debug_bh, "Deleted!");
 				if (!bh2jh(bitmap_bh)->b_committed_data)
@@ -452,7 +451,7 @@ do_more:
 			jbd_unlock_bh_state(bitmap_bh);
 			ext3_error(sb, __FUNCTION__,
 				"bit already cleared for block "E3FSBLK,
-				 block + i);
+				block);
 			jbd_lock_bh_state(bitmap_bh);
 			BUFFER_TRACE(bitmap_bh, "bit already cleared");
 		} else {
@@ -479,7 +478,6 @@ do_more:
 	*pdquot_freed_blocks += group_freed;
 
 	if (overflow && !err) {
-		block += count;
 		count = overflow;
 		goto do_more;
 	}
@@ -1191,6 +1189,57 @@ int ext3_should_retry_alloc(struct super
 	return journal_force_commit_nested(EXT3_SB(sb)->s_journal);
 }
 
+static inline int block_needs_fix(ext3_fsblk_t block,
+		unsigned long num,
+		struct ext3_group_desc *gdp,
+		struct super_block *sb)
+{
+	if (in_range(le32_to_cpu(gdp->bg_block_bitmap), block, num))
+		return 1;
+	if (in_range(le32_to_cpu(gdp->bg_inode_bitmap), block, num))
+		return 1;
+	if (in_range(block, le32_to_cpu(gdp->bg_inode_table),
+			EXT3_SB(sb)->s_itb_per_group))
+		return 1;
+	if (in_range(block + num - 1, le32_to_cpu(gdp->bg_inode_table),
+			EXT3_SB(sb)->s_itb_per_group))
+		return 1;
+	return 0;
+}
+
+/*
+ *
+ * set the bits for all of the metadata blocks in the group
+ *
+ * Note: This will potentially use up some of the handle's buffer credits.
+ * Normally we have way too many credits, so that is OK. In _very_ rare cases it
+ * might not be OK.  We will trigger an assertion if we run out of credits, and we
+ * will have to do a full fsck of the filesystem - better than randomly corrupting
+ * filesystem metadata.
+ */
+static void fix_group(int group, struct super_block *sb)
+{
+	int i;
+	ext3_fsblk_t bit;
+	unsigned long gdblocks;
+	struct buffer_head *gdp_bh;
+	struct ext3_group_desc *gdp = ext3_get_group_desc(sb, group, &gdp_bh);
+
+	if (ext3_bg_has_super(sb, group))
+		ext3_set_bit(0, gdp_bh->b_data);
+	gdblocks = ext3_bg_num_gdb(sb, group);
+	for (i = 0, bit = 1; i < gdblocks; i++, bit++)
+		/* actually a bit more complex for INCOMPAT_META_BG fs */
+		ext3_set_bit(i, gdp_bh->b_data);
+	ext3_set_bit(gdp->bg_inode_bitmap % EXT3_BLOCKS_PER_GROUP(sb),
+		gdp_bh->b_data);
+	ext3_set_bit(gdp->bg_block_bitmap % EXT3_BLOCKS_PER_GROUP(sb),
+		gdp_bh->b_data);
+	for (i = 0, bit = gdp->bg_inode_table % EXT3_BLOCKS_PER_GROUP(sb);
+			i < EXT3_SB(sb)->s_itb_per_group; i++, bit++)
+		ext3_set_bit(i, gdp_bh->b_data);
+}
+
 /*
  * ext3_new_block uses a goal block to assist allocation.  If the goal is
  * free, or there is a free block within 32 blocks of the goal, that block
@@ -1260,7 +1309,7 @@ ext3_fsblk_t ext3_new_blocks(handle_t *h
 		*errp = -ENOSPC;
 		goto out;
 	}
-
+repeat:
 	/*
 	 * First, test whether the goal block is free.
 	 */
@@ -1367,17 +1416,10 @@ allocated:
 
 	ret_block = grp_alloc_blk + ext3_group_first_block_no(sb, group_no);
 
-	if (in_range(le32_to_cpu(gdp->bg_block_bitmap), ret_block, num) ||
-	    in_range(le32_to_cpu(gdp->bg_inode_bitmap), ret_block, num) ||
-	    in_range(ret_block, le32_to_cpu(gdp->bg_inode_table),
-		      EXT3_SB(sb)->s_itb_per_group) ||
-	    in_range(ret_block + num - 1, le32_to_cpu(gdp->bg_inode_table),
-		      EXT3_SB(sb)->s_itb_per_group))
-		ext3_error(sb, "ext3_new_block",
-			    "Allocating block in system zone - "
-			    "blocks from "E3FSBLK", length %lu",
-			     ret_block, num);
-
+	if (block_needs_fix(ret_block, num, gdp, sb)) {
+		fix_group(group_no, sb);
+		goto repeat;
+	}
 	performed_allocation = 1;
 
 #ifdef CONFIG_JBD_DEBUG
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]