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

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

[sho]

Hi,

>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

I had done the same work for both ext3 and ext2,
and sent the patches to ext2-devel three months ago.
If you need, you can get them from the following URL.

The introduction for my patch set:
http://marc.theaimsgroup.com/?l=ext2-devel&m=114856080926308&w=2

The patch which fixes overflow problem on ext3:
http://marc.theaimsgroup.com/?l=ext2-devel&m=114856129220863&w=2

The patch which fixes overflow problem on ext2:
http://marc.theaimsgroup.com/?l=ext2-devel&m=114856135120693&w=2

I have reviewed your patch and found other place which might
cause overflow as below.  If group_first_block is the first block of
the last group, overflow will occur.  This has already been fixed
in my patch.

o ext3_try_to_allocate_with_rsv() in fs/ext3/balloc.c
	if ((my_rsv->rsv_start >= group_first_block + EXT3_BLOCKS_PER_GROUP(sb))
		    || (my_rsv->rsv_end < group_first_block))
			BUG();

Cheers, Takashi

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

[Eric Sandeen]

sho@tnes.nec.co.jp wrote:
> Hi,
> 
> I had done the same work for both ext3 and ext2,
> and sent the patches to ext2-devel three months ago.
> If you need, you can get them from the following URL.

Thank you, I did not know that anyone else had done this work (I guess I should 
have searched...!)

Thanks for the pointer, I will compare my patches to yours.  I guess it can be 
considered an independent review. :)

> I have reviewed your patch and found other place which might
> cause overflow as below.  If group_first_block is the first block of
> the last group, overflow will occur.  This has already been fixed
> in my patch.

I had that patch locally as well, but had not yet sent it out.

Thanks,
-Eric

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

[Mingming Cao]

sho@tnes.nec.co.jp wrote:

> I have reviewed your patch and found other place which might
> cause overflow as below.  If group_first_block is the first block of
> the last group, overflow will occur.  This has already been fixed
> in my patch.
> 
> o ext3_try_to_allocate_with_rsv() in fs/ext3/balloc.c
> 	if ((my_rsv->rsv_start >= group_first_block + EXT3_BLOCKS_PER_GROUP(sb))
> 		    || (my_rsv->rsv_end < group_first_block))
> 			BUG();
> 

Yes, this isn't being addressed in the current 2.6.18-rc4 kernel. I 
think this is better than casting to unsigned long long:

- 	if ((my_rsv->rsv_start >= group_first_block + EXT3_BLOCKS_PER_GROUP(sb))
+ 	if ((my_rsv->rsv_start > group_first_block - 1 + 
EXT3_BLOCKS_PER_GROUP(sb))


Thanks,

Mingming

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

[Eric Sandeen]

Mingming Cao wrote:

> Yes, this isn't being addressed in the current 2.6.18-rc4 kernel. I 
> think this is better than casting to unsigned long long:
> 
> -     if ((my_rsv->rsv_start >= group_first_block + EXT3_BLOCKS_PER_GROUP(sb))
> +     if ((my_rsv->rsv_start > group_first_block - 1 + EXT3_BLOCKS_PER_GROUP(sb))

And there are a few more of these.  The patch I currently have in my stack follows.
(personally I think
	last = first + (count - 1)
is clearer than
	last = first - 1 + count
but that's just my opinion...)

Thanks,

-Eric

Signed-off-by: Eric Sandeen <esandeen@redhat.com>

Index: linux-2.6.17/fs/ext3/balloc.c
===================================================================
--- linux-2.6.17.orig/fs/ext3/balloc.c
+++ linux-2.6.17/fs/ext3/balloc.c
@@ -168,7 +168,7 @@ goal_in_my_reservation(struct ext3_reser
 	ext3_fsblk_t group_first_block, group_last_block;
 
 	group_first_block = ext3_group_first_block_no(sb, group);
-	group_last_block = group_first_block + EXT3_BLOCKS_PER_GROUP(sb) - 1;
+	group_last_block = group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1);
 
 	if ((rsv->_rsv_start > group_last_block) ||
 	    (rsv->_rsv_end < group_first_block))
@@ -897,7 +897,7 @@ static int alloc_new_reservation(struct 
 	spinlock_t *rsv_lock = &EXT3_SB(sb)->s_rsv_window_lock;
 
 	group_first_block = ext3_group_first_block_no(sb, group);
-	group_end_block = group_first_block + EXT3_BLOCKS_PER_GROUP(sb) - 1;
+	group_end_block = group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1);
 
 	if (grp_goal < 0)
 		start_block = group_first_block;
@@ -1132,7 +1132,7 @@ ext3_try_to_allocate_with_rsv(struct sup
 			try_to_extend_reservation(my_rsv, sb,
 					*count-my_rsv->rsv_end + grp_goal - 1);
 
-		if ((my_rsv->rsv_start >= group_first_block + EXT3_BLOCKS_PER_GROUP(sb))
+		if ((my_rsv->rsv_start > group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1))
 		    || (my_rsv->rsv_end < group_first_block))
 			BUG();
 		ret = ext3_try_to_allocate(sb, handle, group, bitmap_bh, grp_goal,

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

[Mingming Cao]

On Fri, 2006-08-18 at 12:39 -0500, Eric Sandeen wrote:
> Mingming Cao wrote:
> 
> > Yes, this isn't being addressed in the current 2.6.18-rc4 kernel. I 
> > think this is better than casting to unsigned long long:
> > 
> > -     if ((my_rsv->rsv_start >= group_first_block + EXT3_BLOCKS_PER_GROUP(sb))
> > +     if ((my_rsv->rsv_start > group_first_block - 1 + EXT3_BLOCKS_PER_GROUP(sb))
> 
> And there are a few more of these.  The patch I currently have in my stack follows.
> (personally I think
> 	last = first + (count - 1)
> is clearer than
> 	last = first - 1 + count
> but that's just my opinion...)
> 
> Thanks,
> 

ACK.
> -Eric
> 
> Signed-off-by: Eric Sandeen <esandeen@redhat.com>
> 
> Index: linux-2.6.17/fs/ext3/balloc.c
> ===================================================================
> --- linux-2.6.17.orig/fs/ext3/balloc.c
> +++ linux-2.6.17/fs/ext3/balloc.c
> @@ -168,7 +168,7 @@ goal_in_my_reservation(struct ext3_reser
>  	ext3_fsblk_t group_first_block, group_last_block;
>  
>  	group_first_block = ext3_group_first_block_no(sb, group);
> -	group_last_block = group_first_block + EXT3_BLOCKS_PER_GROUP(sb) - 1;
> +	group_last_block = group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1);
>  
>  	if ((rsv->_rsv_start > group_last_block) ||
>  	    (rsv->_rsv_end < group_first_block))
> @@ -897,7 +897,7 @@ static int alloc_new_reservation(struct 
>  	spinlock_t *rsv_lock = &EXT3_SB(sb)->s_rsv_window_lock;
>  
>  	group_first_block = ext3_group_first_block_no(sb, group);
> -	group_end_block = group_first_block + EXT3_BLOCKS_PER_GROUP(sb) - 1;
> +	group_end_block = group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1);
>  
>  	if (grp_goal < 0)
>  		start_block = group_first_block;
> @@ -1132,7 +1132,7 @@ ext3_try_to_allocate_with_rsv(struct sup
>  			try_to_extend_reservation(my_rsv, sb,
>  					*count-my_rsv->rsv_end + grp_goal - 1);
>  
> -		if ((my_rsv->rsv_start >= group_first_block + EXT3_BLOCKS_PER_GROUP(sb))
> +		if ((my_rsv->rsv_start > group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1))
>  		    || (my_rsv->rsv_end < group_first_block))
>  			BUG();
>  		ret = ext3_try_to_allocate(sb, handle, group, bitmap_bh, grp_goal,
> 
> 
> 
> -------------------------------------------------------------------------
> Using Tomcat but need to do more? Need to support web services, security?
> Get stuff done quickly with pre-integrated technology to make your job easier
> Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
> http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
> _______________________________________________
> Ext2-devel mailing list
> Ext2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/ext2-devel

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

[Andreas Dilger]

On Aug 18, 2006  12:39 -0500, Eric Sandeen wrote:
> @@ -168,7 +168,7 @@ goal_in_my_reservation(struct ext3_reser
>  	ext3_fsblk_t group_first_block, group_last_block;
>  
>  	group_first_block = ext3_group_first_block_no(sb, group);
> -	group_last_block = group_first_block + EXT3_BLOCKS_PER_GROUP(sb) - 1;
> +	group_last_block = group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1);
>  
>  	if ((rsv->_rsv_start > group_last_block) ||
>  	    (rsv->_rsv_end < group_first_block))
> @@ -897,7 +897,7 @@ static int alloc_new_reservation(struct 
>  	spinlock_t *rsv_lock = &EXT3_SB(sb)->s_rsv_window_lock;
>  
>  	group_first_block = ext3_group_first_block_no(sb, group);
> -	group_end_block = group_first_block + EXT3_BLOCKS_PER_GROUP(sb) - 1;
> +	group_end_block = group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1);
>  
>  	if (grp_goal < 0)
>  		start_block = group_first_block;

I don't see how these can make a difference?  Surely, if the intermediate
sum overflows it will then underflow when "- 1" is done?  Not that I mind,
per-se, just curious why you think this fixes anything.

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

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

[Eric Sandeen]

Andreas Dilger wrote:
> On Aug 18, 2006  12:39 -0500, Eric Sandeen wrote:
>> @@ -168,7 +168,7 @@ goal_in_my_reservation(struct ext3_reser
>>  	ext3_fsblk_t group_first_block, group_last_block;
>>  
>>  	group_first_block = ext3_group_first_block_no(sb, group);
>> -	group_last_block = group_first_block + EXT3_BLOCKS_PER_GROUP(sb) - 1;
>> +	group_last_block = group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1);
>>  
>>  	if ((rsv->_rsv_start > group_last_block) ||
>>  	    (rsv->_rsv_end < group_first_block))
>> @@ -897,7 +897,7 @@ static int alloc_new_reservation(struct 
>>  	spinlock_t *rsv_lock = &EXT3_SB(sb)->s_rsv_window_lock;
>>  
>>  	group_first_block = ext3_group_first_block_no(sb, group);
>> -	group_end_block = group_first_block + EXT3_BLOCKS_PER_GROUP(sb) - 1;
>> +	group_end_block = group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1);
>>  
>>  	if (grp_goal < 0)
>>  		start_block = group_first_block;
> 
> I don't see how these can make a difference?  Surely, if the intermediate
> sum overflows it will then underflow when "- 1" is done?  Not that I mind,
> per-se, just curious why you think this fixes anything.

Well, you're right, if it overflows then it will underflow again.  And I've not 
observed any actual failures, and I don't expect to.  But personally I guess I'd 
rather avoid the whole overflow in the first place... maybe I'm being silly.  :)

If you think it's unnecessary code churn then we can not make this change...

-Eric