ext4: Enable mount ext4 with AGGRESSIVE_TEST

PATCH In review Akira Fujita 2 participants 3 messages Aug 27, 2009 – Sep 1, 2009 ext4 file system

Share & tools

Akira Fujita

author

2009-08-27 04:55 UTC #0 lore raw

PATCH 1/1 ext4: Enable mount ext4 with AGGRESSIVE_TEST

When the AGGRESSIVE_TEST is enabled,
the maximum extent count of inode becomes '3' in ext4_ext_space_root(), but __ext4_ext_check() which is called via ext4_fill_super()
checks eh_max is larger '4', therefore we always get -EIO and can not mount ext4. The patch fix this issue.

Aug 26 15:43:50 bsd086 kernel: [   96.070277] EXT4-fs error (device sda8): ext4_ext_check_inode: bad header/extent in inode #8: too large eh_max - magic f30a, entries 1, max 4(3), depth 0(0)
Aug 26 15:43:50 bsd086 kernel: [   96.070526] EXT4-fs (sda8): no journal found

Signed-off-by Akira Fujita <[email protected]>

---
 extents.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/ext4/extents.c 2009-08-26 15:45:31.000000000 +0900 renamed from fs/ext4/extents.c 2009-08-26 14:59:47.000000000 +0900 +2 -2

@@ -263,8 +263,8 @@ static int ext4_ext_space_root(struct in
        size -= sizeof(struct ext4_extent_header);
        size /= sizeof(struct ext4_extent);
 #ifdef AGGRESSIVE_TEST
-       if (size > 3)
-               size = 3;
+       if (size > 4)
+               size = 4;
 #endif
        return size;
 }

Theodore Tso

maintainer

2009-08-28 14:40 UTC #1 lore raw

Quoted 5 lines · Akira Fujita

When the AGGRESSIVE_TEST is enabled,
the maximum extent count of inode becomes '3' in ext4_ext_space_root(), but __ext4_ext_check() which is called via ext4_fill_super()
checks eh_max is larger '4', therefore we always get -EIO and can not mount ext4. The patch fix this issue.

This isn't really the right fix for the problem.

What's actually going on here is that the patch which added which sanity checks for extents, which added (among other functions) __ext4_ext_check() and ext4_ext_max_entries() doesn't quite work right in the presence of AGGRESSIVE_TEST.

The goal of AGGRESSIVE_TEST is to stress test the extent tree insert and deletion code by forcing the size of eh_max to be a smaller value than it otherwise would be. It did this by dropping in limits to the values returned by ext4_ext_space_root(), ext4_ext_space_idx(), ext4_ext_space_block(), and ext4_ext_block_idx(). This worked all very well and coded when these functions were only used to initialize the eh_max field.

The problem is that the extent checking code also used these functions to check whether or not the extent tree was sane, and if AGGRESSIVE_TEST was enabled, it caused the journal to be considered invalid.

Your patch patches over the problem by changing ext4_ext_space_root to fix the one case where a problem arises, which patches over the problem for a freshly created filesystem, but it doesn't fix problems that will arise for a populated filesystem. It also decreases the effectiveness of AGGRESSIVE_TEST.

The following patch is a better way of fixing the problem.

- Ted

commit e07fa129e97b33b3f7772d98c184813ea7604a35

Author: Theodore Ts'o <[email protected]>
Date:   Fri Aug 28 10:40:33 2009 -0400

ext4: fix extent sanity checking code with AGGRESSIVE_TEST

The extents sanity-checking code depends on the ext4_ext_space_*() functions returning the maximum alloable size for eh_max; however, when the debugging #ifdef AGGRESSIVE_TEST is enabled to test the extent tree handling code, this prevents a normally created ext4 filesystem from being mounted with the errors:

Aug 26 15:43:50 bsd086 kernel: [ 96.070277] EXT4-fs error (device sda8): ext4_ext_check_inode: bad header/extent in inode #8: too large eh_max - magic f30a, entries 1, max 4(3), depth 0(0)
Aug 26 15:43:50 bsd086 kernel: [ 96.070526] EXT4-fs (sda8): no journal found

Bug reported by Akira Fujita.

Signed-off-by "Theodore Ts'o" <[email protected]>

fs/ext4/extents.c +34 ~~-26~~

index 94f40b1..f7bdd55 100644
@@ -229,57 +229,65 @@ ext4_ext_new_meta_block(handle_t *handle, struct inode *inode,
 	return newblock;
 }
 
-static int ext4_ext_space_block(struct inode *inode)
+static inline int ext4_ext_space_block(struct inode *inode, int check)
 {
 	int size;
 
 	size = (inode->i_sb->s_blocksize - sizeof(struct ext4_extent_header))
 			/ sizeof(struct ext4_extent);
+	if (!check) {
 #ifdef AGGRESSIVE_TEST
-	if (size > 6)
-		size = 6;
+		if (size > 6)
+			size = 6;
 #endif
+	}
 	return size;
 }
 
-static int ext4_ext_space_block_idx(struct inode *inode)
+static inline int ext4_ext_space_block_idx(struct inode *inode, int check)
 {
 	int size;
 
 	size = (inode->i_sb->s_blocksize - sizeof(struct ext4_extent_header))
 			/ sizeof(struct ext4_extent_idx);
+	if (!check) {
 #ifdef AGGRESSIVE_TEST
-	if (size > 5)
-		size = 5;
+		if (size > 5)
+			size = 5;
 #endif
+	}
 	return size;
 }
 
-static int ext4_ext_space_root(struct inode *inode)
+static inline int ext4_ext_space_root(struct inode *inode, int check)
 {
 	int size;
 
 	size = sizeof(EXT4_I(inode)->i_data);
 	size -= sizeof(struct ext4_extent_header);
 	size /= sizeof(struct ext4_extent);
+	if (!check) {
 #ifdef AGGRESSIVE_TEST
-	if (size > 3)
-		size = 3;
+		if (size > 3)
+			size = 3;
 #endif
+	}
 	return size;
 }
 
-static int ext4_ext_space_root_idx(struct inode *inode)
+static inline int ext4_ext_space_root_idx(struct inode *inode, int check)
 {
 	int size;
 
 	size = sizeof(EXT4_I(inode)->i_data);
 	size -= sizeof(struct ext4_extent_header);
 	size /= sizeof(struct ext4_extent_idx);
+	if (!check) {
 #ifdef AGGRESSIVE_TEST
-	if (size > 4)
-		size = 4;
+		if (size > 4)
+			size = 4;
 #endif
+	}
 	return size;
 }

@@ -293,9 +301,9 @@ int ext4_ext_calc_metadata_amount(struct inode *inode, int blocks)
 	int lcap, icap, rcap, leafs, idxs, num;
 	int newextents = blocks;
 
-	rcap = ext4_ext_space_root_idx(inode);
-	lcap = ext4_ext_space_block(inode);
-	icap = ext4_ext_space_block_idx(inode);
+	rcap = ext4_ext_space_root_idx(inode, 0);
+	lcap = ext4_ext_space_block(inode, 0);
+	icap = ext4_ext_space_block_idx(inode, 0);
 
 	/* number of new leaf blocks needed */
 	num = leafs = (newextents + lcap - 1) / lcap;

@@ -320,14 +328,14 @@ ext4_ext_max_entries(struct inode *inode, int depth)
 
 	if (depth == ext_depth(inode)) {
 		if (depth == 0)
-			max = ext4_ext_space_root(inode);
+			max = ext4_ext_space_root(inode, 1);
 		else
-			max = ext4_ext_space_root_idx(inode);
+			max = ext4_ext_space_root_idx(inode, 1);
 	} else {
 		if (depth == 0)
-			max = ext4_ext_space_block(inode);
+			max = ext4_ext_space_block(inode, 1);
 		else
-			max = ext4_ext_space_block_idx(inode);
+			max = ext4_ext_space_block_idx(inode, 1);
 	}
 
 	return max;

@@ -626,7 +634,7 @@ int ext4_ext_tree_init(handle_t *handle, struct inode *inode)
 	eh->eh_depth = 0;
 	eh->eh_entries = 0;
 	eh->eh_magic = EXT4_EXT_MAGIC;
-	eh->eh_max = cpu_to_le16(ext4_ext_space_root(inode));
+	eh->eh_max = cpu_to_le16(ext4_ext_space_root(inode, 0));
 	ext4_mark_inode_dirty(handle, inode);
 	ext4_ext_invalidate_cache(inode);
 	return 0;

@@ -851,7 +859,7 @@ static int ext4_ext_split(handle_t *handle, struct inode *inode,
 
 	neh = ext_block_hdr(bh);
 	neh->eh_entries = 0;
-	neh->eh_max = cpu_to_le16(ext4_ext_space_block(inode));
+	neh->eh_max = cpu_to_le16(ext4_ext_space_block(inode, 0));
 	neh->eh_magic = EXT4_EXT_MAGIC;
 	neh->eh_depth = 0;
 	ex = EXT_FIRST_EXTENT(neh);

@@ -927,7 +935,7 @@ static int ext4_ext_split(handle_t *handle, struct inode *inode,
 		neh = ext_block_hdr(bh);
 		neh->eh_entries = cpu_to_le16(1);
 		neh->eh_magic = EXT4_EXT_MAGIC;
-		neh->eh_max = cpu_to_le16(ext4_ext_space_block_idx(inode));
+		neh->eh_max = cpu_to_le16(ext4_ext_space_block_idx(inode, 0));
 		neh->eh_depth = cpu_to_le16(depth - i);
 		fidx = EXT_FIRST_INDEX(neh);
 		fidx->ei_block = border;

@@ -1052,9 +1060,9 @@ static int ext4_ext_grow_indepth(handle_t *handle, struct inode *inode,
 	/* old root could have indexes or leaves
 	 * so calculate e_max right way */
 	if (ext_depth(inode))
-	  neh->eh_max = cpu_to_le16(ext4_ext_space_block_idx(inode));
+		neh->eh_max = cpu_to_le16(ext4_ext_space_block_idx(inode, 0));
 	else
-	  neh->eh_max = cpu_to_le16(ext4_ext_space_block(inode));
+		neh->eh_max = cpu_to_le16(ext4_ext_space_block(inode, 0));
 	neh->eh_magic = EXT4_EXT_MAGIC;
 	set_buffer_uptodate(bh);
 	unlock_buffer(bh);

@@ -1069,7 +1077,7 @@ static int ext4_ext_grow_indepth(handle_t *handle, struct inode *inode,
 		goto out;
 
 	curp->p_hdr->eh_magic = EXT4_EXT_MAGIC;
-	curp->p_hdr->eh_max = cpu_to_le16(ext4_ext_space_root_idx(inode));
+	curp->p_hdr->eh_max = cpu_to_le16(ext4_ext_space_root_idx(inode, 0));
 	curp->p_hdr->eh_entries = cpu_to_le16(1);
 	curp->p_idx = EXT_FIRST_INDEX(curp->p_hdr);

@@ -2348,7 +2356,7 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
 		if (err == 0) {
 			ext_inode_hdr(inode)->eh_depth = 0;
 			ext_inode_hdr(inode)->eh_max =
-				cpu_to_le16(ext4_ext_space_root(inode));
+				cpu_to_le16(ext4_ext_space_root(inode, 0));
 			err = ext4_ext_dirty(handle, inode, path);
 		}
 	}

Akira Fujita

author Tested-by

2009-09-01 01:35 UTC #2 lore raw

Hi,

Quoted 27 lines · Theodore Tso

Quoted 5 lines · Akira Fujita
When the AGGRESSIVE_TEST is enabled,
the maximum extent count of inode becomes '3' in ext4_ext_space_root(), but __ext4_ext_check() which is called via ext4_fill_super()
checks eh_max is larger '4', therefore we always get -EIO and can not mount ext4. The patch fix this issue.
This isn't really the right fix for the problem.
What's actually going on here is that the patch which added which sanity checks for extents, which added (among other functions) __ext4_ext_check() and ext4_ext_max_entries() doesn't quite work right in the presence of AGGRESSIVE_TEST.
The goal of AGGRESSIVE_TEST is to stress test the extent tree insert and deletion code by forcing the size of eh_max to be a smaller value than it otherwise would be. It did this by dropping in limits to the values returned by ext4_ext_space_root(), ext4_ext_space_idx(), ext4_ext_space_block(), and ext4_ext_block_idx(). This worked all very well and coded when these functions were only used to initialize the eh_max field.
The problem is that the extent checking code also used these functions to check whether or not the extent tree was sane, and if AGGRESSIVE_TEST was enabled, it caused the journal to be considered invalid.

Quoted 6 lines

Your patch patches over the problem by changing ext4_ext_space_root to fix the one case where a problem arises, which patches over the problem for a freshly created filesystem, but it doesn't fix problems that will arise for a populated filesystem. It also decreases the effectiveness of AGGRESSIVE_TEST.

Thank you for careful explanation. :-)
I confirmed that the problem was fixed with your patch.

Tested-by Akira Fujita <[email protected]>

Regards,
Akira Fujita