fix ext3 mounts at 16T

PATCH In review Eric Sandeen 6 participants 8 messages Aug 11, 2006 – Aug 18, 2006 ext2 file system

lore

Share & tools

lore

Eric Sandeen

author

2006-08-11 22:13 UTC #0 lore raw

PATCH 1/1 fix ext3 mounts at 16T

I figure before we get too fired up about 48 bits and ext4 let's fix 32 bits on ext3 :)

I need to do some actual IO testing now, but this gets things mounting for a 16T ext3 filesystem. (patched up e2fsprogs is needed too, I'll send that off the kernel list)

This patch fixes these issues in the kernel:

o sbi->s_groups_count overflows in ext3_fill_super()

	sbi->s_groups_count = (le32_to_cpu(es->s_blocks_count) -
			       le32_to_cpu(es->s_first_data_block) +
			       EXT3_BLOCKS_PER_GROUP(sb) - 1) /
			      EXT3_BLOCKS_PER_GROUP(sb);

at 16T, s_blocks_count is already maxed out; adding EXT3_BLOCKS_PER_GROUP(sb) overflows it and groups_count comes out to 0. Not really what we want, and causes a failed mount.

Feel free to check my math (actually, please do!), but changing it this way should work & avoid the overflow:

(A + B - 1)/B changed to: ((A - 1)/B) + 1

o ext3_check_descriptors() overflows range checks

ext3_check_descriptors() iterates over all block groups making sure that various bits are within the right block ranges... on the last pass through, it is checking the error case

[item] >= block + EXT3_BLOCKS_PER_GROUP(sb)

where "block" is the first block in the last block group. The last block in this group (and the last one that will fit in 32 bits) is block + EXT3_BLOCKS_PER_GROUP(sb)- 1. block + EXT3_BLOCKS_PER_GROUP(sb) wraps back around to 0.

so, make things clearer with "first_block" and "last_block" where those are first and last, inclusive, and use <, > rather than <, >=.

Finally, the last block group may be smaller than the rest, so account for this on the last pass through: last_block = sb->s_blocks_count - 1;

(a similar patch could be done for ext2; does anyone in their right mind use ext2 at 16T? I'll send an ext2 patch doing the same thing if that's warranted)

Thanks,

-Eric

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

fs/ext3/super.c +0 -0

Index: linux-2.6-xfs/fs/ext3/super.c
===================================================================
@@ -1132,7 +1132,8 @@ static int ext3_setup_super(struct super

static int ext3_check_descriptors (struct super_block * sb)

{
	struct ext3_sb_info *sbi = EXT3_SB(sb);
-	ext3_fsblk_t block = le32_to_cpu(sbi->s_es->s_first_data_block);
+	ext3_fsblk_t first_block = le32_to_cpu(sbi->s_es->s_first_data_block);
+	ext3_fsblk_t last_block;
	struct ext3_group_desc * gdp = NULL;
	int desc_block = 0;
	int i;

@@ -1141,12 +1142,16 @@ static int ext3_check_descriptors (struc

	for (i = 0; i < sbi->s_groups_count; i++)
	{

+ if (i == sbi->s_groups_count - 1)
+ last_block = sb->s_blocks_count - 1;
+ else
+ last = first + (EXT3_BLOCKS_PER_GROUP(sb) - 1);
+

		if ((i % EXT3_DESC_PER_BLOCK(sb)) == 0)
			gdp = (struct ext3_group_desc *)
					sbi->s_group_desc[desc_block++]->b_data;

- if (le32_to_cpu(gdp->bg_block_bitmap) < block ||
- le32_to_cpu(gdp->bg_block_bitmap) >=
- block + EXT3_BLOCKS_PER_GROUP(sb))
+ if (le32_to_cpu(gdp->bg_block_bitmap) < first_block ||
+ le32_to_cpu(gdp->bg_block_bitmap) > last_block)
{
ext3_error (sb, "ext3_check_descriptors",
"Block bitmap for group %d"
@@ -1155,9 +1160,8 @@ static int ext3_check_descriptors (struc

					le32_to_cpu(gdp->bg_block_bitmap));
			return 0;
		}

- if (le32_to_cpu(gdp->bg_inode_bitmap) < block ||
- le32_to_cpu(gdp->bg_inode_bitmap) >=
- block + EXT3_BLOCKS_PER_GROUP(sb))
+ if (le32_to_cpu(gdp->bg_inode_bitmap) < first_block ||
+ le32_to_cpu(gdp->bg_inode_bitmap) > last_block)
{
ext3_error (sb, "ext3_check_descriptors",
"Inode bitmap for group %d"
@@ -1166,9 +1170,9 @@ static int ext3_check_descriptors (struc

					le32_to_cpu(gdp->bg_inode_bitmap));
			return 0;
		}

- if (le32_to_cpu(gdp->bg_inode_table) < block ||
- le32_to_cpu(gdp->bg_inode_table) + sbi->s_itb_per_group >=
- block + EXT3_BLOCKS_PER_GROUP(sb))
+ if (le32_to_cpu(gdp->bg_inode_table) < first_block ||
+ le32_to_cpu(gdp->bg_inode_table) + sbi->s_itb_per_group >
+ last_block)
{
ext3_error (sb, "ext3_check_descriptors",
"Inode table for group %d"
@@ -1177,7 +1181,7 @@ static int ext3_check_descriptors (struc

					le32_to_cpu(gdp->bg_inode_table));
			return 0;
		}
-		block += EXT3_BLOCKS_PER_GROUP(sb);
+		first_block += EXT3_BLOCKS_PER_GROUP(sb);
		gdp++;
	}

@@ -1580,10 +1584,9 @@ static int ext3_fill_super (struct super

	if (EXT3_BLOCKS_PER_GROUP(sb) == 0)
		goto cantfind_ext3;

- sbi->s_groups_count = (le32_to_cpu(es->s_blocks_count) -
- le32_to_cpu(es->s_first_data_block) +
- EXT3_BLOCKS_PER_GROUP(sb) - 1) /
- EXT3_BLOCKS_PER_GROUP(sb);
+ sbi->s_groups_count = ((le32_to_cpu(es->s_blocks_count) -
+ le32_to_cpu(es->s_first_data_block) - 1)

+				       / EXT3_BLOCKS_PER_GROUP(sb)) + 1;
	db_count = (sbi->s_groups_count + EXT3_DESC_PER_BLOCK(sb) - 1) /
		   EXT3_DESC_PER_BLOCK(sb);
	sbi->s_group_desc = kmalloc(db_count * sizeof (struct buffer_head *),

Mingming Cao

2006-08-11 22:30 UTC #1 lore raw

Quoted 17 lines · Eric Sandeen

I figure before we get too fired up about 48 bits and ext4 let's fix 32 bits on ext3 :)
I need to do some actual IO testing now, but this gets things mounting for a 16T ext3 filesystem. (patched up e2fsprogs is needed too, I'll send that off the kernel list)
This patch fixes these issues in the kernel:
o sbi->s_groups_count overflows in ext3_fill_super()
	sbi->s_groups_count = (le32_to_cpu(es->s_blocks_count) -
			       le32_to_cpu(es->s_first_data_block) +
			       EXT3_BLOCKS_PER_GROUP(sb) - 1) /
			      EXT3_BLOCKS_PER_GROUP(sb);
at 16T, s_blocks_count is already maxed out; adding EXT3_BLOCKS_PER_GROUP(sb) overflows it and groups_count comes out to 0. Not really what we want, and causes a failed mount.

Quoted 6 lines

Feel free to check my math (actually, please do!), but changing it this way should work & avoid the overflow:
(A + B - 1)/B changed to: ((A - 1)/B) + 1
o ext3_check_descriptors() overflows range checks

Looks correct to me.:)

Mingming

Eric Sandeen

2006-08-14 19:54 UTC #3 lore raw

In reply to Eric Sandeen

Quoted 2 lines · Eric Sandeen

I figure before we get too fired up about 48 bits and ext4 let's fix 32 bits on ext3 :)

Urk... must remember to "quilt refresh..." - this version actually compiles :/ sorry!

(1st part of 2nd hunk was wrong before)

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

fs/ext3/super.c +0 -0

Index: linux-2.6.17/fs/ext3/super.c
===================================================================
@@ -1132,7 +1132,8 @@ static int ext3_setup_super(struct super

static int ext3_check_descriptors (struct super_block * sb)

{
	struct ext3_sb_info *sbi = EXT3_SB(sb);
-	ext3_fsblk_t block = le32_to_cpu(sbi->s_es->s_first_data_block);
+	ext3_fsblk_t first_block = le32_to_cpu(sbi->s_es->s_first_data_block);
+	ext3_fsblk_t last_block;
	struct ext3_group_desc * gdp = NULL;
	int desc_block = 0;
	int i;

@@ -1141,12 +1142,17 @@ static int ext3_check_descriptors (struc

	for (i = 0; i < sbi->s_groups_count; i++)
	{

+ if (i == sbi->s_groups_count - 1)
+ last_block = sbi->s_es->s_blocks_count - 1;
+ else
+ last_block = first_block +
+ (EXT3_BLOCKS_PER_GROUP(sb) - 1);
+

		if ((i % EXT3_DESC_PER_BLOCK(sb)) == 0)
			gdp = (struct ext3_group_desc *)
					sbi->s_group_desc[desc_block++]->b_data;

					le32_to_cpu(gdp->bg_block_bitmap));
			return 0;
		}

					le32_to_cpu(gdp->bg_inode_bitmap));
			return 0;
		}

					le32_to_cpu(gdp->bg_inode_table));
			return 0;
		}
-		block += EXT3_BLOCKS_PER_GROUP(sb);
+		first_block += EXT3_BLOCKS_PER_GROUP(sb);
		gdp++;
	}

@@ -1580,10 +1585,9 @@ static int ext3_fill_super (struct super

	if (EXT3_BLOCKS_PER_GROUP(sb) == 0)
		goto cantfind_ext3;

+				       / EXT3_BLOCKS_PER_GROUP(sb)) + 1;
	db_count = (sbi->s_groups_count + EXT3_DESC_PER_BLOCK(sb) - 1) /
		   EXT3_DESC_PER_BLOCK(sb);
	sbi->s_group_desc = kmalloc(db_count * sizeof (struct buffer_head *),

Andrew Morton

2006-08-14 22:58 UTC #4 lore raw

In reply to Eric Sandeen

Quoted 1 line · On Fri, 11 Aug 2006 17:13:14 -0500 Eric Sandeen <[email protected]> wrote:

(a similar patch could be done for ext2; does anyone in their right mind use ext2 at 16T?

well, a bug's a bug. People might want to ue 16TB ext2 for comparative performance testing, or because they get their jollies from running fsck or something.

Quoted 1 line

I'll send an ext2 patch doing the same thing if that's warranted)

please, when you have nothing better to do ;)

Eric Sandeen

author

2006-08-14 23:02 UTC #5 lore raw

Quoted 8 lines · Andrew Morton

Quoted 1 line · On Fri, 11 Aug 2006 17:13:14 -0500 Eric Sandeen <[email protected]> wrote:
(a similar patch could be done for ext2; does anyone in their right mind use ext2 at 16T?
well, a bug's a bug. People might want to ue 16TB ext2 for comparative performance testing, or because they get their jollies from running fsck or something.

ext2 and ext3 have seemingly already diverged a bit, but I suppose no reason to let it go further.

Quoted 3 lines

Quoted 1 line
I'll send an ext2 patch doing the same thing if that's warranted)
please, when you have nothing better to do ;)

Will do :)

-Eric

Eric Sandeen

author

2006-08-15 17:28 UTC #6 lore raw

PATCH 1/1 fix ext2 mounts at 16T

In reply to Andrew Morton

Quoted 2 lines · Andrew Morton

Quoted 1 line
(I'll send an ext2 patch doing the same thing if that's warranted)
please, when you have nothing better to do ;)

Ok, here's the ext2 version. note however that ext2 never got the ext3_fsblk_t treatment, so there are probably signed containers lurking in ext2 still. I'll leave that for another day when I have nothing better to do ;-) But at least this part is less divergent now.

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

fs/ext2/super.c +0 -0

@@ -505,17 +505,24 @@ static int ext2_check_descriptors (struc

	int i;
	int desc_block = 0;
	struct ext2_sb_info *sbi = EXT2_SB(sb);
-	unsigned long block = le32_to_cpu(sbi->s_es->s_first_data_block);
+	unsigned long first_block = le32_to_cpu(sbi->s_es->s_first_data_block);
+	unsigned long last_block;
	struct ext2_group_desc * gdp = NULL;

	ext2_debug ("Checking group descriptors");

	for (i = 0; i < sbi->s_groups_count; i++)
	{

+ if (i == sbi->s_groups_count - 1)
+ last_block = sbi->s_es->s_blocks_count - 1;
+ else
+ last_block = first_block +
+ (EXT2_BLOCKS_PER_GROUP(sb) - 1);
+

		if ((i % EXT2_DESC_PER_BLOCK(sb)) == 0)
			gdp = (struct ext2_group_desc *) sbi->s_group_desc[desc_block++]->b_data;

- if (le32_to_cpu(gdp->bg_block_bitmap) < block ||
- le32_to_cpu(gdp->bg_block_bitmap) >= block + EXT2_BLOCKS_PER_GROUP(sb))
+ if (le32_to_cpu(gdp->bg_block_bitmap) < first_block ||
+ le32_to_cpu(gdp->bg_block_bitmap) > last_block)
{
ext2_error (sb, "ext2_check_descriptors",
"Block bitmap for group %d"
@@ -523,8 +530,8 @@ static int ext2_check_descriptors (struc

				    i, (unsigned long) le32_to_cpu(gdp->bg_block_bitmap));
			return 0;
		}

- if (le32_to_cpu(gdp->bg_inode_bitmap) < block ||
- le32_to_cpu(gdp->bg_inode_bitmap) >= block + EXT2_BLOCKS_PER_GROUP(sb))
+ if (le32_to_cpu(gdp->bg_inode_bitmap) < first_block ||
+ le32_to_cpu(gdp->bg_inode_bitmap) > last_block)
{
ext2_error (sb, "ext2_check_descriptors",
"Inode bitmap for group %d"
@@ -532,9 +539,9 @@ static int ext2_check_descriptors (struc

				    i, (unsigned long) le32_to_cpu(gdp->bg_inode_bitmap));
			return 0;
		}

- if (le32_to_cpu(gdp->bg_inode_table) < block ||
- le32_to_cpu(gdp->bg_inode_table) + sbi->s_itb_per_group >=
- block + EXT2_BLOCKS_PER_GROUP(sb))
+ if (le32_to_cpu(gdp->bg_inode_table) < first_block ||
+ le32_to_cpu(gdp->bg_inode_table) + sbi->s_itb_per_group >
+ last_block)
{
ext2_error (sb, "ext2_check_descriptors",
"Inode table for group %d"
@@ -542,7 +549,7 @@ static int ext2_check_descriptors (struc

				    i, (unsigned long) le32_to_cpu(gdp->bg_inode_table));
			return 0;
		}
-		block += EXT2_BLOCKS_PER_GROUP(sb);
+		first_block += EXT2_BLOCKS_PER_GROUP(sb);
		gdp++;
	}
	return 1;

@@ -822,10 +829,9 @@ static int ext2_fill_super(struct super_

	if (EXT2_BLOCKS_PER_GROUP(sb) == 0)
		goto cantfind_ext2;

- sbi->s_groups_count = (le32_to_cpu(es->s_blocks_count) -
- le32_to_cpu(es->s_first_data_block) +
- EXT2_BLOCKS_PER_GROUP(sb) - 1) /
- EXT2_BLOCKS_PER_GROUP(sb);
+ sbi->s_groups_count = ((le32_to_cpu(es->s_blocks_count) -
+ le32_to_cpu(es->s_first_data_block) - 1)

+ 					/ EXT2_BLOCKS_PER_GROUP(sb)) + 1;
	db_count = (sbi->s_groups_count + EXT2_DESC_PER_BLOCK(sb) - 1) /
		   EXT2_DESC_PER_BLOCK(sb);
	sbi->s_group_desc = kmalloc (db_count * sizeof (struct buffer_head *), GFP_KERNEL);

Johann Lombardi

2006-08-16 11:45 UTC #7 lore raw

In reply to Eric Sandeen

Quoted 10 lines

static int ext3_check_descriptors (struct super_block * sb) {

(quoted diff) +2 -1

 	struct ext3_sb_info *sbi = EXT3_SB(sb);
-	ext3_fsblk_t block = le32_to_cpu(sbi->s_es->s_first_data_block);
+	ext3_fsblk_t first_block = le32_to_cpu(sbi->s_es->s_first_data_block);
+	ext3_fsblk_t last_block;
 	struct ext3_group_desc * gdp = NULL;
 	int desc_block = 0;
 	int i;

Review static int ext3_check_descriptors (struc · line 1148

Quoted 7 lines

(quoted diff) +4 -0

@@ -1141,12 +1142,16 @@ static int ext3_check_descriptors (struc
 
 	for (i = 0; i < sbi->s_groups_count; i++)
 	{
+		if (i == sbi->s_groups_count - 1)
+			last_block = sb->s_blocks_count - 1;
+		else
+			last = first + (EXT3_BLOCKS_PER_GROUP(sb) - 1);

I think it should be:
last_block = first_block + ...

Johann

Andreas Dilger

2006-08-18 08:50 UTC #2 lore raw

In reply to Mingming Cao

Quoted 18 lines · Mingming Cao

Quoted 17 lines · Eric Sandeen
I figure before we get too fired up about 48 bits and ext4 let's fix 32 bits on ext3 :)
I need to do some actual IO testing now, but this gets things mounting for a 16T ext3 filesystem. (patched up e2fsprogs is needed too, I'll send that off the kernel list)
This patch fixes these issues in the kernel:
o sbi->s_groups_count overflows in ext3_fill_super()
	sbi->s_groups_count = (le32_to_cpu(es->s_blocks_count) -
			       le32_to_cpu(es->s_first_data_block) +
			       EXT3_BLOCKS_PER_GROUP(sb) - 1) /
			      EXT3_BLOCKS_PER_GROUP(sb);
at 16T, s_blocks_count is already maxed out; adding EXT3_BLOCKS_PER_GROUP(sb) overflows it and groups_count comes out to 0. Not really what we want, and causes a failed mount.

Quoted 6 lines

Quoted 6 lines
Feel free to check my math (actually, please do!), but changing it this way should work & avoid the overflow:
(A + B - 1)/B changed to: ((A - 1)/B) + 1
o ext3_check_descriptors() overflows range checks

This is generally not safe because of underflow, but it also isn't possible to have a 0-block filesystem so it should be OK.

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.