[PATCH] fix ext3 mounts at 16T

[PATCH] fix ext3 mounts at 16T

[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.

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 <esandeen@redhat.com>

Index: linux-2.6-xfs/fs/ext3/super.c
===================================================================
--- linux-2.6-xfs.orig/fs/ext3/super.c
+++ 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 *),

Re: [Ext2-devel] [PATCH] fix ext3 mounts at 16T

[Mingming Cao]

On Fri, 2006-08-11 at 17:13 -0500, Eric Sandeen wrote:
> 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

Looks correct to me.:)

Mingming

Re: [UPDATED PATCH] fix ext3 mounts at 16T

[Eric Sandeen]

Eric Sandeen wrote:

> 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 <esandeen@redhat.com>

Index: linux-2.6.17/fs/ext3/super.c
===================================================================
--- linux-2.6.17.orig/fs/ext3/super.c
+++ 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;
-		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 +1161,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 +1171,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 +1182,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 +1585,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 *),

Re: [PATCH] fix ext3 mounts at 16T

[Andrew Morton]

On Fri, 11 Aug 2006 17:13:14 -0500
Eric Sandeen <esandeen@redhat.com> 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.

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

please, when you have nothing better to do ;)

Re: [Ext2-devel] [PATCH] fix ext3 mounts at 16T

[Eric Sandeen]

Andrew Morton wrote:
> On Fri, 11 Aug 2006 17:13:14 -0500
> Eric Sandeen <esandeen@redhat.com> 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.

>> 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

[PATCH] fix ext2 mounts at 16T

[Eric Sandeen]

Andrew Morton wrote:

>> (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 <esandeen@redhat.com>

--- linux.orig/fs/ext2/super.c
+++ linux/fs/ext2/super.c
@@ -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);

Re: [Ext2-devel] [PATCH] fix ext3 mounts at 16T

[Johann Lombardi]

>  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);

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

Johann

Re: [Ext2-devel] [PATCH] fix ext3 mounts at 16T

[Andreas Dilger]

On Aug 11, 2006  15:29 -0700, Mingming Cao wrote:
> On Fri, 2006-08-11 at 17:13 -0500, Eric Sandeen wrote:
> > 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

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.