[PATCH] ext4: fix online resize group descriptors corruption

[PATCH] ext4: fix online resize group descriptors corruption

[Frédéric Bohé]

From: Frederic Bohe <frederic.bohe@bull.net>

This is the patch for the group descriptor table corruption during
online resize pointed out by Theodore Tso.
The issue was due to the ext4 group descriptor which can be either
32 or 64 bytes long.
Only the 64 bytes structure was taken into account.

Signed-off-by: Frederic Bohe <frederic.bohe@bull.net>
---
URL for the discussion about this issue: http://article.gmane.org/gmane.comp.file-systems.ext4/7226

The patch has been tested with linux-2.6.26-rc4-ext4-1 and
e2fsprogs-git-master-2008-06-07-365857912e27914afa8857af5adf74ee19ca9e03
under kvm and loopback device and without mballoc.

It has also been tested with linux-2.6.26-rc5, 
ext4-patch-queue-f245e945ae491e7931076706217b63d4bf0b5a1b
and e2fsprogs-365857912e27914afa8857af5adf74ee19ca9e03
with a physical hard disk and without mballoc

The patch does not correct the mballoc case (Oops in ext4_mb_release).
There is still an issue when resizing a flex_bg filesystem.

 resize.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff -rup a/fs/ext4/resize.c b/fs/ext4/resize.c
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -855,7 +855,8 @@ int ext4_group_add(struct super_block *s
 	 */
 
 	/* Update group descriptor block for new group */
-	gdp = (struct ext4_group_desc *)primary->b_data + gdb_off;
+	gdp = (struct ext4_group_desc *)(
+	       (__u8 *)primary->b_data + gdb_off * EXT4_DESC_SIZE(sb));
 
 	ext4_block_bitmap_set(sb, gdp, input->block_bitmap); /* LV FIXME */
 	ext4_inode_bitmap_set(sb, gdp, input->inode_bitmap); /* LV FIXME */

Re: [PATCH] ext4: fix online resize group descriptors corruption

[Andreas Dilger]

On Jun 12, 2008  11:42 +0200, Fr�d�ric Boh� wrote:
> From: Frederic Bohe <frederic.bohe@bull.net>
> 
> This is the patch for the group descriptor table corruption during
> online resize pointed out by Theodore Tso.
> The issue was due to the ext4 group descriptor which can be either
> 32 or 64 bytes long.
> Only the 64 bytes structure was taken into account.
> 
> diff -rup a/fs/ext4/resize.c b/fs/ext4/resize.c
> --- a/fs/ext4/resize.c
> +++ b/fs/ext4/resize.c
> @@ -855,7 +855,8 @@ int ext4_group_add(struct super_block *s
>  	 */
>  
>  	/* Update group descriptor block for new group */
> -	gdp = (struct ext4_group_desc *)primary->b_data + gdb_off;
> +	gdp = (struct ext4_group_desc *)(
> +	       (__u8 *)primary->b_data + gdb_off * EXT4_DESC_SIZE(sb));

Normally pointer arithmetic is done by casting to (char *)...
Otherwise, patch looks sensible, though it could be reformatted to 
match the normal coding style a bit better:

	gdp = (struct ext4_group_desc *)((char *)primary->b_data +
					 gdb_off * EXT4_DESC_SIZE(sb));

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] ext4: fix online resize group descriptors corruption

[Frédéric Bohé]

Le jeudi 12 juin 2008 à 04:59 -0600, Andreas Dilger a écrit :
> On Jun 12, 2008  11:42 +0200, Fr�d�ric Boh� wrote:
> > From: Frederic Bohe <frederic.bohe@bull.net>
> > 
> > This is the patch for the group descriptor table corruption during
> > online resize pointed out by Theodore Tso.
> > The issue was due to the ext4 group descriptor which can be either
> > 32 or 64 bytes long.
> > Only the 64 bytes structure was taken into account.
> > 
> > diff -rup a/fs/ext4/resize.c b/fs/ext4/resize.c
> > --- a/fs/ext4/resize.c
> > +++ b/fs/ext4/resize.c
> > @@ -855,7 +855,8 @@ int ext4_group_add(struct super_block *s
> >  	 */
> >  
> >  	/* Update group descriptor block for new group */
> > -	gdp = (struct ext4_group_desc *)primary->b_data + gdb_off;
> > +	gdp = (struct ext4_group_desc *)(
> > +	       (__u8 *)primary->b_data + gdb_off * EXT4_DESC_SIZE(sb));
> 
> Normally pointer arithmetic is done by casting to (char *)...
> Otherwise, patch looks sensible, though it could be reformatted to 
> match the normal coding style a bit better:
> 
> 	gdp = (struct ext4_group_desc *)((char *)primary->b_data +
> 					 gdb_off * EXT4_DESC_SIZE(sb));
> 
> Cheers, Andreas
> --
> Andreas Dilger
> Sr. Staff Engineer, Lustre Group
> Sun Microsystems of Canada, Inc.
> 
> 

> From: Frederic Bohe <frederic.bohe@bull.net>
> > 
> > This is the patch for the group descriptor table corruption during
> > online resize pointed out by Theodore Tso.
> > The issue was due to the ext4 group descriptor which can be either
> > 32 or 64 bytes long.
> > Only the 64 bytes structure was taken into account.
> 
From: Frederic Bohe <frederic.bohe@bull.net> 

This is the patch for the group descriptor table corruption during
online resize pointed out by Theodore Tso.
The issue was due to the ext4 group descriptor which can be either
32 or 64 bytes long.
Only the 64 bytes structure was taken into account. 

Signed-off-by: Frederic Bohe <frederic.bohe@bull.net>
---
 resize.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff -rup a/fs/ext4/resize.c b/fs/ext4/resize.c
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -855,7 +855,8 @@ int ext4_group_add(struct super_block *s
 	 */
 
 	/* Update group descriptor block for new group */
-	gdp = (struct ext4_group_desc *)primary->b_data + gdb_off;
+	gdp = (struct ext4_group_desc *)((char *)primary->b_data +
+	                                 gdb_off * EXT4_DESC_SIZE(sb));
 
 	ext4_block_bitmap_set(sb, gdp, input->block_bitmap); /* LV FIXME */
 	ext4_inode_bitmap_set(sb, gdp, input->inode_bitmap); /* LV FIXME */


--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] ext4: fix online resize group descriptors corruption

[Frédéric Bohé]

There was a tab issue in the v2 patch, sorry for the spam...

From: Frederic Bohe <frederic.bohe@bull.net>

This is the patch for the group descriptor table corruption during
online resize pointed out by Theodore Tso.
The issue was due to the ext4 group descriptor which can be either
32 or 64 bytes long.
Only the 64 bytes structure was taken into account.t. 

Signed-off-by: Frederic Bohe <frederic.bohe@bull.net>
---
 resize.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff -rup a/fs/ext4/resize.c b/fs/ext4/resize.c
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -855,7 +855,8 @@ int ext4_group_add(struct super_block *s
 	 */
 
 	/* Update group descriptor block for new group */
-	gdp = (struct ext4_group_desc *)primary->b_data + gdb_off;
+	gdp = (struct ext4_group_desc *)((char *)primary->b_data +
+					 gdb_off * EXT4_DESC_SIZE(sb));
 
 	ext4_block_bitmap_set(sb, gdp, input->block_bitmap); /* LV FIXME */
 	ext4_inode_bitmap_set(sb, gdp, input->inode_bitmap); /* LV FIXME */




--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ext4: fix online resize group descriptors corruption

[Jean-Pierre Dion]

Hi Andreas,

thank you for this review of Frédéric's patch
and your comments that help.

Frédéric just joined our team a few days ago and
this kind of help is very usefull.

Frédéric has not a great experience in Linux
but he has a strong one in embedded systems
and telecoms. Correct me where I am wrong Frédéric. ;-)


jean-pierre


Andreas Dilger a écrit :
> On Jun 12, 2008  11:42 +0200, Fr�d�ric Boh� wrote:
>   
>> From: Frederic Bohe <frederic.bohe@bull.net>
>>
>> This is the patch for the group descriptor table corruption during
>> online resize pointed out by Theodore Tso.
>> The issue was due to the ext4 group descriptor which can be either
>> 32 or 64 bytes long.
>> Only the 64 bytes structure was taken into account.
>>
>> diff -rup a/fs/ext4/resize.c b/fs/ext4/resize.c
>> --- a/fs/ext4/resize.c
>> +++ b/fs/ext4/resize.c
>> @@ -855,7 +855,8 @@ int ext4_group_add(struct super_block *s
>>  	 */
>>  
>>  	/* Update group descriptor block for new group */
>> -	gdp = (struct ext4_group_desc *)primary->b_data + gdb_off;
>> +	gdp = (struct ext4_group_desc *)(
>> +	       (__u8 *)primary->b_data + gdb_off * EXT4_DESC_SIZE(sb));
>>     
>
> Normally pointer arithmetic is done by casting to (char *)...
> Otherwise, patch looks sensible, though it could be reformatted to 
> match the normal coding style a bit better:
>
> 	gdp = (struct ext4_group_desc *)((char *)primary->b_data +
> 					 gdb_off * EXT4_DESC_SIZE(sb));
>
> Cheers, Andreas
> --
> Andreas Dilger
> Sr. Staff Engineer, Lustre Group
> Sun Microsystems of Canada, Inc.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>   

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ext4: fix online resize group descriptors corruption

[Theodore Tso]

On Thu, Jun 12, 2008 at 11:42:34AM +0200, Frédéric Bohé wrote:
> From: Frederic Bohe <frederic.bohe@bull.net>

Frederic,

Many thanks!  How much testing have you done with the patch?  Does it
fix growing the filesystems from a filesystem with say, 18 block
groups to 32 block groups?  Have you tried with with different
filesystem features, including with and without flex_bg and uninit_bg?

Regards,

	   	     	       	    			- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ext4: fix online resize group descriptors corruption

[Frédéric Bohé]

Ted,

Concerning flex_bg and uninit_bg : it's currently not possible to resize
a filesystem with those features. It seems there are issues with
resize2fs.
We have also used nomballoc switch when mounting because of the oops
when unmounting.

We have tried both physical hard disk drive and loop back devices.
We have tested several filesystem resize with few (adding 1 group) to
many (adding 250 groups) more groups. All give correct group descriptors
using dump2fs and no problem when writing files and unmounting
filesystems.

This is a typical test case:

mkfs.ext4 -E test_fs -O ^flex_bg /dev/sdc1 10M
mount -t ext4dev -o nomballoc /dev/sdc1 /mnt/test
dumpe2fs /dev/sdc1
resize2fs /dev/sdc1 100M
dumpe2fs /dev/sdc1
dd if=/dev/urandom of=/mnt/test/data bs=95M count=1
umount /mnt/test
fsck.ext4 -f /dev/sdc1

This test being run for several sizes of filesystem (from 10M with 1K
blocks to 1G with 4K blocks).

Let me know if we have missed something or if you want more testings.

Regards




Le jeudi 12 juin 2008 à 15:04 -0400, Theodore Tso a écrit :
> On Thu, Jun 12, 2008 at 11:42:34AM +0200, Frédéric Bohé wrote:
> > From: Frederic Bohe <frederic.bohe@bull.net>
> 
> Frederic,
> 
> Many thanks!  How much testing have you done with the patch?  Does it
> fix growing the filesystems from a filesystem with say, 18 block
> groups to 32 block groups?  Have you tried with with different
> filesystem features, including with and without flex_bg and uninit_bg?
> 
> Regards,
> 
> 	   	     	       	    			- Ted
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html