libext2: clear BG_BLOCK_UNINIT in any group containing metadata

Running a test like the m_uninit test runs:

# mke2fs -F -o Linux -O uninit_bg testfs 131072

results in groups which have blocks allocated for metadata, but which still have the BLOCK_UNINIT flag set:

Group 2: (Blocks 16385-24576) [INODE_UNINIT, BLOCK_UNINIT, ITABLE_ZEROED]
Checksum 0x17c2, unused inodes 2048
Block bitmap at 16385 (+0), Inode bitmap at 16386 (+1) Inode table at 16387-16642 (+2)

however, e2fsck doesn't find this error - which seems like another bug.

Doing another test like this:

mkfs.ext4 -I 256 -b 4096 -N 256 -G 1 -g 256 /dev/loop0 1024 mount /dev/loop0 /mnt/test
mkdir /mnt/test/dir
umount /mnt/test
fsck.ext4 -fn /dev/loop0

does find the error on fsck, I think because BLOCK_UNINIT gets cleared, and the group isn't ignored at fsck time:

Free blocks count wrong for group #2 (253, counted=249).

In the first case, flex_bg is not set; in the 2nd it is set, but s_log_groups_per_flex == 0 (due to -G 1), so in neither case is flexbg_size set in ext2fs_allocate_group_table().

This leads to skipping the code which clears BLOCK_UNINIT after the metadata is allocated.

I think the fix is to unconditionally clear the BLOCK_UNINIT flag in whatever block group gets metadata allocated to it, as follows.

the m_uninit test needs to be fixed up too, as does e2fsck it seems, to catch this error in the first place?

Sorry for the long commit message, if the patch looks right I can try to do better ;)

Signed-off-by Eric Sandeen <[email protected]>

---

lib/ext2fs/alloc_tables.c +10 -11

index 9f3d4e0..3462e85 100644
@@ -88,6 +88,7 @@ errcode_t ext2fs_allocate_group_table(ext2_filsys fs, dgrp_t group,
 	blk64_t		group_blk, start_blk, last_blk, new_blk, blk;
 	dgrp_t		last_grp = 0;
 	int		rem_grps = 0, flexbg_size = 0;
+	dgrp_t		gr;
 
 	group_blk = ext2fs_group_first_block2(fs, group);
 	last_blk = ext2fs_group_last_block2(fs, group);

@@ -141,13 +142,13 @@ errcode_t ext2fs_allocate_group_table(ext2_filsys fs, dgrp_t group,
 			return retval;
 		ext2fs_mark_block_bitmap2(bmap, new_blk);
 		ext2fs_block_bitmap_loc_set(fs, group, new_blk);
+		gr = ext2fs_group_of_blk2(fs, new_blk);
+		ext2fs_bg_flags_clear(fs, gr, EXT2_BG_BLOCK_UNINIT);
 		if (flexbg_size) {
-			dgrp_t gr = ext2fs_group_of_blk2(fs, new_blk);
 			ext2fs_bg_free_blocks_count_set(fs, gr, ext2fs_bg_free_blocks_count(fs, gr) - 1);
 			ext2fs_free_blocks_count_add(fs->super, -1);
-			ext2fs_bg_flags_clear(fs, gr, EXT2_BG_BLOCK_UNINIT);
-			ext2fs_group_desc_csum_set(fs, gr);
 		}
+		ext2fs_group_desc_csum_set(fs, gr);
 	}
 
 	if (flexbg_size) {

@@ -172,13 +173,13 @@ errcode_t ext2fs_allocate_group_table(ext2_filsys fs, dgrp_t group,
 			return retval;
 		ext2fs_mark_block_bitmap2(bmap, new_blk);
 		ext2fs_inode_bitmap_loc_set(fs, group, new_blk);
+		gr = ext2fs_group_of_blk2(fs, new_blk);
+		ext2fs_bg_flags_clear(fs, gr, EXT2_BG_BLOCK_UNINIT);
 		if (flexbg_size) {
-			dgrp_t gr = ext2fs_group_of_blk2(fs, new_blk);
 			ext2fs_bg_free_blocks_count_set(fs, gr, ext2fs_bg_free_blocks_count(fs, gr) - 1);
 			ext2fs_free_blocks_count_add(fs->super, -1);
-			ext2fs_bg_flags_clear(fs, gr, EXT2_BG_BLOCK_UNINIT);
-			ext2fs_group_desc_csum_set(fs, gr);
 		}
+		ext2fs_group_desc_csum_set(fs, gr);
 	}
 
 	/*

@@ -209,14 +210,13 @@ errcode_t ext2fs_allocate_group_table(ext2_filsys fs, dgrp_t group,
 		     j < fs->inode_blocks_per_group;
 		     j++, blk++) {
 			ext2fs_mark_block_bitmap2(bmap, blk);
+			gr = ext2fs_group_of_blk2(fs, blk);
+			ext2fs_bg_flags_clear(fs, gr, EXT2_BG_BLOCK_UNINIT);
 			if (flexbg_size) {
-				dgrp_t gr = ext2fs_group_of_blk2(fs, blk);
 				ext2fs_bg_free_blocks_count_set(fs, gr, ext2fs_bg_free_blocks_count(fs, gr) - 1);
 				ext2fs_free_blocks_count_add(fs->super, -1);
-				ext2fs_bg_flags_clear(fs, gr,
-						     EXT2_BG_BLOCK_UNINIT);
-				ext2fs_group_desc_csum_set(fs, gr);
 			}
+			ext2fs_group_desc_csum_set(fs, gr);
 		}
 		ext2fs_inode_table_loc_set(fs, group, new_blk);
 	}
 
--

To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Running a test like the m_uninit test runs:

# mke2fs -F -o Linux -O uninit_bg testfs 131072

results in groups which have blocks allocated for metadata, but which still have the BLOCK_UNINIT flag set:

Group 2: (Blocks 16385-24576) [INODE_UNINIT, BLOCK_UNINIT, ITABLE_ZEROED]
Checksum 0x17c2, unused inodes 2048
Block bitmap at 16385 (+0), Inode bitmap at 16386 (+1) Inode table at 16387-16642 (+2)

Quoted 11 lines · Eric Sandeen

Running a test like the m_uninit test runs:

# mke2fs -F -o Linux -O uninit_bg testfs 131072

results in groups which have blocks allocated for metadata, but which still have the BLOCK_UNINIT flag set:

Group 2: (Blocks 16385-24576) [INODE_UNINIT, BLOCK_UNINIT, ITABLE_ZEROED]
Checksum 0x17c2, unused inodes 2048
Block bitmap at 16385 (+0), Inode bitmap at 16386 (+1) Inode table at 16387-16642 (+2)

Sorry for the delay in getting back to you. The general rule here is what triggers BLOCK_UNINIT getting cleared is when we allocate metadata blocks for *another* block group in that block group. When we initialize an uninitialized block group, it's easy enough to set the bits for the block group's allocation bitmaps and inode tables.

However, we *don't* want to have to scan all of the block groups to see if they have metadata blocks appearing in a particular block group, so that's the reason for this rule.

So in the above dumpe2fs output, since we aren't using flex_bg, the only metadata blocks which are in a bg is the bg's own metadata blocks, and so that's why it's OK for BLOCK_UNINIT to be set. This isn't a bug.

- Ted