s_first_meta_bg treatment incompatibility between kernel and e2fsprogs

Hello,

I have taken a look at META_BG feature and think there is some incoherency between kernel and e2fsprogs about s_first_meta_bg handling.

When considering initialisation of unitialised block bitmaps for groups before first meta group one:
- kernel considers that descriptors blocks occupy EXT4_SB(sb)->s_gdb_count blocks (see ext4_bg_num_gdb_nometa at balloc.c:764 that is indirectly called from ext4_init_block_bitmap), s_gdb_count being the number of blocks to store all descriptors (computed from super.c).
- e2fsprogs considers that descriptors blocks occupy s_first_meta_bg
(see ext2fs_reserve_super_and_bgd at alloc_sb.c:55-68).

The difference of behaviour is wrong as e2fsck will certainly complain there is a bitmap marked unused when it should be if the bitmap was initialised by kernel and number of descriptors blocks is lower than s_first_meta_bg or reverse if number of descriptors blocks is higher than s_first_meta_bg.

So, the kernel behaviour seems to be wrong in the META_BG case when s_first_meta_bg is not 0: ext4_bg_num_gdb returns either 1 (META_BG feature present and group being one of the meta group) or s_gdb_count (META_BG feature not present or group being one before first meta group). As s_gdb_count is number of blocks for all groups, I think it should returns either 1 (META_BG present and group being one of the meta group) or s_first_meta_bg (META_BG present and group being one before first meta group) or s_gdb_count (META_BG not set).

I see also that the resize2fs does not handle the s_first_meta_bg flag in case a filesystem is shrunk such as number of descriptor blocks goes below s_first_meta_bg. To what I looked, it does not seem to be a problem (apart from the problem in kernel described above), but I did not perform a complete check about that. At least there is some blocks still allocated when there is no more need for that (but e2fsck does not complain as it uses also the s_first_meta_bg value). I do not know if it is desired behaviour. In case the s_first_meta_bg is lowered and blocks freed, it will certainly be better to add a check into e2fsck to check that s_first_meta_bg is coherent with number of descriptor blocks (s_first_meta_bg <= fs->desc_blocks).

If you want to perform some tests on that, I modified tune2fs to allow setting the META_BG flag on a filesystem that does not have it with setting s_first_meta_bg to the current number of blocks for descriptors (it is how I understand META_BG/s_first_meta_group should be used).

Regards,

Damien

*** misc/tune2fs.old 2009-11-11 12:20:33.698192912 +0100
--- misc/tune2fs.c 2009-11-11 11:38:20.265333248 +0100
***************
*** 121,126 ****
--- 121,127 ----

                EXT2_FEATURE_COMPAT_DIR_INDEX,
        /* Incompat */
        EXT2_FEATURE_INCOMPAT_FILETYPE |
+         EXT2_FEATURE_INCOMPAT_META_BG |
                EXT3_FEATURE_INCOMPAT_EXTENTS |
                EXT4_FEATURE_INCOMPAT_FLEX_BG,
        /* R/O compat */
***************
*** 418,423 ****
--- 419,440 ----
                }
        }

+ if (FEATURE_ON(E2P_FEATURE_INCOMPAT, EXT2_FEATURE_INCOMPAT_META_BG)) {
+ if (mount_flags & EXT2_MF_MOUNTED) {
+ fputs(_("The meta_bg feature may only be "
+ "set when the filesystem is\n"
+ "unmounted.\n"), stderr);
+ exit(1);
+ }
+ if (sb->s_feature_compat & EXT2_FEATURE_COMPAT_RESIZE_INODE) {
+ fputs(_("The meta_bg feature cannot be "
+ "set when the resize_inode is\n"
+ "set.\n"), stderr);
+ exit(1);
+ }
+ sb->s_first_meta_bg = fs->desc_blocks;
+ }
+

        if (FEATURE_OFF(E2P_FEATURE_RO_INCOMPAT,
                            EXT4_FEATURE_RO_COMPAT_HUGE_FILE)) {
                if ((mount_flags & EXT2_MF_MOUNTED) &&

Yup, you're right. Ouch. That is a kernel bug, and it means that if we resize a filesystem to the point where we need to use meta_bg (because we've run out of blocks to reserve), if there are uninitialized block bitmaps, kernels that don't have a fix will misbehave by reserving too many file system metadata blocks. This will waste bit of disk space, which fsck will fix.

(s_first_meta_bg by definition is always less than or equal to s_gdb_count.)

I think this patch should fix things up.

- Ted

commit b33c339814f97fc48a843f45f6068f84bc735141

Author: Theodore Ts'o <[email protected]>
Date:   Sat Nov 14 23:20:30 2009 -0500

ext4: Fix uninit block bitmap initialization when s_meta_first_bg is non-zero

The number of old-style block group descriptor blocks is s_meta_first_bg when the meta_bg feature flag is set.

index 1d04189..f3032c9 100644
@@ -761,7 +761,13 @@ static unsigned long ext4_bg_num_gdb_meta(struct super_block *sb,
 static unsigned long ext4_bg_num_gdb_nometa(struct super_block *sb,
 					ext4_group_t group)
 {
-	return ext4_bg_has_super(sb, group) ? EXT4_SB(sb)->s_gdb_count : 0;
+	if (!ext4_bg_has_super(sb, group))
+		return 0;
+
+	if (EXT4_HAS_INCOMPAT_FEATURE(sb,EXT4_FEATURE_INCOMPAT_META_BG))
+		return le32_to_cpu(EXT4_SB(sb)->s_es->s_first_meta_bg);
+	else
+		return EXT4_SB(sb)->s_gdb_count;
 }
 
 /**

For future references, patches are less likely to slip through the cracks if they are sent to the linux-ext4 mailing list as opposed to having a BZ bug opened. (Yeah, I know, that's unusual). The reason for that is that patches are tracked via patchwork, here:

http://patchwork.ozlabs.org/project/linux-ext4

Basically, anything that looks like a patch which is sent to linux-ext4 gets snagged by patchwork, and it's a good place to look for stuff that hasn't yet been merged. In some cases there are good reasons why a patch has been kept out, and in other cases patches have been merged or definitely rejected and I don't get to getting that status updated in patchwork, but overall I've found it to work very well.

As far as the matter of taste issue is concerned, I think we already have too many static functions with a single caller, and it actually makes the code harder to understand. So adding yet another simple static function I think is a bad thing, not a good thing.

I've looked through the other areas, and the one place where I see a problem is in the block validity checks in ext4_iget() for the extended attribute block and in block_validity.c. The former can and should be fixed to use the latter.

Here's the fix that I plan to be using. Comments, anyone?

- Ted

ext4: fix block validity checks so they work correctly with meta_bg

The block validity checks used by ext4_data_block_valid() wasn't correctly written to check file systems with the meta_bg feature. Fix this.

---
 fs/ext4/block_validity.c |    2 +-
 fs/ext4/inode.c          |    5 +----
 2 files changed, 2 insertions(+), 5 deletions(-)

index 50784ef..dc79b75 100644
@@ -160,7 +160,7 @@ int ext4_setup_system_zone(struct super_block *sb)
 		if (ext4_bg_has_super(sb, i) &&
 		    ((i < 5) || ((i % flex_size) == 0)))
 			add_system_zone(sbi, ext4_group_first_block_no(sb, i),
-					sbi->s_gdb_count + 1);
+					ext4_bg_num_gdb(sb, i) + 1);
 		gdp = ext4_get_group_desc(sb, i, NULL);
 		ret = add_system_zone(sbi, ext4_block_bitmap(sb, gdp), 1);
 		if (ret)

index b5cdb88..c62ca93 100644
@@ -4886,10 +4886,7 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
 
 	ret = 0;
 	if (ei->i_file_acl &&
-	    ((ei->i_file_acl <
-	      (le32_to_cpu(EXT4_SB(sb)->s_es->s_first_data_block) +
-	       EXT4_SB(sb)->s_gdb_count)) ||
-	     (ei->i_file_acl >= ext4_blocks_count(EXT4_SB(sb)->s_es)))) {
+	    !ext4_data_block_valid(EXT4_SB(sb), ei->i_file_acl, 1)) {
 		ext4_error(sb, __func__,
 			   "bad extended attribute block %llu in inode #%lu",
 			   ei->i_file_acl, inode->i_ino);

Hello,

[...]

It was just to mimic the existing function, but I agree with you. The other difference is that it shall be applied on ext3 also.

For the first one (on block_validity.c), as far as I understand, presence of superblock and descriptors blocks in a group are no more related in case of meta_bg group, so shouldn't be the code divided into 2 distincts part: one to treat super block, second to treat descriptor blocks (I do not understand the ((i < 5) || ((i % flex_size) == 0) part into the test, so add it if it is trully needed), something as:

		ext4_fsblk_t firstSystemBlock = ext4_group_first_block_no(sb, i);
		unsigned long nbDescBlocks;
                if (ext4_bg_has_super(sb, i)) {
                        add_system_zone(sbi, firstSystemBlock,
                                        1);
			firstSystemBlock++;
		}
		nbDescBlocks = ext4_bg_num_gdb(sb, i);
		if (nbDescBlocks != 0)
			add_system_zone(sbi, firstSystemBlock,
                                        nbDescBlocks);

Regards,

Damien