diff mbox

e2fsprogs: allocate inode table wholly within group

Message ID 51DB2EB9.4010903@redhat.com
State New, archived
Headers show

Commit Message

Eric Sandeen July 8, 2013, 9:27 p.m. UTC 
Ok, resetting a little on this one.

The actual problem seems to be that the test does successive "-M" minimal resizes, and eventually we resize into the middle of an inode table, leaving the end of the table beyond the fs.

Point "resize2fs -M" at the attached image once or twice w/ fscks in between and you should see it.

It seems like the obvious fix would be to move the inode table if necessary, as with the following patch.

But then of course we find that we've run out of room to move the table, and the resize fails; so we've let resize2fs choose its own minimum size - which which it cannot handle.  Presumably something's gone wrong in calculate_minimum_resize_size(), though I can't tell what.

Truth be told, I'm not enjoying resize2fs right now!

Comments

Theodore Ts'o Oct. 1, 2013, 1:57 a.m. UTC | #1 
On Mon, Jul 08, 2013 at 04:27:21PM -0500, Eric Sandeen wrote:
> 
> The actual problem seems to be that the test does successive "-M" minimal resizes, and eventually we resize into the middle of an inode table, leaving the end of the table beyond the fs.
> 
> Point "resize2fs -M" at the attached image once or twice w/ fscks in between and you should see it.

I've been going through my patch backlog, so I finally had a chance to
take a very close look at your test image.  I now understand why
things are failing.

1) The test image (which you said was generated on a ppc e2fsprogs?)
was doing something very weird as far as the location of the
allocation bitmaps and inode table:

Filesystem features:      ext_attr dir_index filetype sparse_super
Inode count:              512
Block count:              1247
   ...

Group 0: (Blocks 1-1024)
  Primary superblock at 1, Group descriptors at 2-2
  Block bitmap at 66 (+65), Inode bitmap at 67 (+66)
  Inode table at 68-99 (+67)

Group 1: (Blocks 1025-1246)
  Backup superblock at 1025, Group descriptors at 1026-1026
  Block bitmap at 1090 (+65), Inode bitmap at 1091 (+66)
  Inode table at 1092-1123 (+67)

Compare and contrast this with what x86 and Debian's ppc mke2fs creates:

Group 0: (Blocks 1-1024)
  Primary superblock at 1, Group descriptors at 2-2
  Block bitmap at 3 (+2), Inode bitmap at 4 (+3)
  Inode table at 5-14 (+4)

Group 1: (Blocks 1025-1246)
  Backup superblock at 1025, Group descriptors at 1026-1026
  Block bitmap at 1027 (+2), Inode bitmap at 1028 (+3)
  Inode table at 1029-1038 (+4)

So I'm not sure why Fedora's ppc mke2fs is creating file systems in
this way, but that's one of the causes of the bug.


2) The second cause of the bug is that
calculate_minimum_resize_size(), when we calculate the number of
blocks for the last group, the code has an implicit assumption that
the metadata blocks are located at the very beginning of the block
group.   That's an easy fix.

> It seems like the obvious fix would be to move the inode table if
> necessary, as with the following patch.

Your patch is a good one, but at least in the context of resize2fs -M,
we should be fixing calculate_minimum_resize_size() so we can avoid
needing to move the inode table (since even if it can succeed, it's
not worth the danger).

I'll send out some patches to address this.  Thanks for sending the
test image; and my apologies for not having time to get back to this
until now.

					- 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
Eric Sandeen Oct. 1, 2013, 3:26 p.m. UTC | #2 
On 9/30/13 8:57 PM, Theodore Ts'o wrote:
> On Mon, Jul 08, 2013 at 04:27:21PM -0500, Eric Sandeen wrote:
>>
>> The actual problem seems to be that the test does successive "-M" minimal resizes, and eventually we resize into the middle of an inode table, leaving the end of the table beyond the fs.
>>
>> Point "resize2fs -M" at the attached image once or twice w/ fscks in between and you should see it.
> 
> I've been going through my patch backlog, so I finally had a chance to
> take a very close look at your test image.  I now understand why
> things are failing.
> 
> 1) The test image (which you said was generated on a ppc e2fsprogs?)
> was doing something very weird as far as the location of the
> allocation bitmaps and inode table:

Yes, this was just during a fedora build, during the "make check" phase.

https://bugzilla.redhat.com/show_bug.cgi?id=980519

No idea why things should be coming out differently, that's a bit
alarming in and of itself.

(Fedora isn't carrying any interesting patches to speak of).

-Eric

> Filesystem features:      ext_attr dir_index filetype sparse_super
> Inode count:              512
> Block count:              1247
>    ...
> 
> Group 0: (Blocks 1-1024)
>   Primary superblock at 1, Group descriptors at 2-2
>   Block bitmap at 66 (+65), Inode bitmap at 67 (+66)
>   Inode table at 68-99 (+67)
> 
> Group 1: (Blocks 1025-1246)
>   Backup superblock at 1025, Group descriptors at 1026-1026
>   Block bitmap at 1090 (+65), Inode bitmap at 1091 (+66)
>   Inode table at 1092-1123 (+67)
> 
> Compare and contrast this with what x86 and Debian's ppc mke2fs creates:
> 
> Group 0: (Blocks 1-1024)
>   Primary superblock at 1, Group descriptors at 2-2
>   Block bitmap at 3 (+2), Inode bitmap at 4 (+3)
>   Inode table at 5-14 (+4)
> 
> Group 1: (Blocks 1025-1246)
>   Backup superblock at 1025, Group descriptors at 1026-1026
>   Block bitmap at 1027 (+2), Inode bitmap at 1028 (+3)
>   Inode table at 1029-1038 (+4)
> 
> So I'm not sure why Fedora's ppc mke2fs is creating file systems in
> this way, but that's one of the causes of the bug.
> 
> 
> 2) The second cause of the bug is that
> calculate_minimum_resize_size(), when we calculate the number of
> blocks for the last group, the code has an implicit assumption that
> the metadata blocks are located at the very beginning of the block
> group.   That's an easy fix.
> 
>> It seems like the obvious fix would be to move the inode table if
>> necessary, as with the following patch.
> 
> Your patch is a good one, but at least in the context of resize2fs -M,
> we should be fixing calculate_minimum_resize_size() so we can avoid
> needing to move the inode table (since even if it can succeed, it's
> not worth the danger).
> 
> I'll send out some patches to address this.  Thanks for sending the
> test image; and my apologies for not having time to get back to this
> until now.
> 
> 					- 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
Eric Sandeen Oct. 1, 2013, 3:35 p.m. UTC | #3 
On 10/1/13 10:26 AM, Eric Sandeen wrote:
> On 9/30/13 8:57 PM, Theodore Ts'o wrote:
>> On Mon, Jul 08, 2013 at 04:27:21PM -0500, Eric Sandeen wrote:
>>>
>>> The actual problem seems to be that the test does successive "-M" minimal resizes, and eventually we resize into the middle of an inode table, leaving the end of the table beyond the fs.
>>>
>>> Point "resize2fs -M" at the attached image once or twice w/ fscks in between and you should see it.
>>
>> I've been going through my patch backlog, so I finally had a chance to
>> take a very close look at your test image.  I now understand why
>> things are failing.
>>
>> 1) The test image (which you said was generated on a ppc e2fsprogs?)
>> was doing something very weird as far as the location of the
>> allocation bitmaps and inode table:
> 
> Yes, this was just during a fedora build, during the "make check" phase.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=980519
> 
> No idea why things should be coming out differently, that's a bit
> alarming in and of itself.
> 
> (Fedora isn't carrying any interesting patches to speak of).

But I am doing this:

 %check
+# XXX ERS Hack for now; this bug has existed for a while,
+# i.e. it is not a regression in this release, but there
+# is no fix yet, and we need to get this package building.
+# See Bug 987133 - resize2fs tests failing on ppc, s390
+rm -rf tests/r_1024_small_bg*
+rm -rf tests/r_64bit_big_expand*
+rm -rf tests/r_bigalloc_big_expand*
+rm -rf tests/r_ext4_big_expand*
 make check

I'll retest w/ your patches, thanks.

-Eric

--
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
Eric Sandeen Oct. 1, 2013, 4:29 p.m. UTC | #4 
On 10/1/13 10:35 AM, Eric Sandeen wrote:
> On 10/1/13 10:26 AM, Eric Sandeen wrote:
>> On 9/30/13 8:57 PM, Theodore Ts'o wrote:
>>> On Mon, Jul 08, 2013 at 04:27:21PM -0500, Eric Sandeen wrote:
>>>>
>>>> The actual problem seems to be that the test does successive "-M" minimal resizes, and eventually we resize into the middle of an inode table, leaving the end of the table beyond the fs.
>>>>
>>>> Point "resize2fs -M" at the attached image once or twice w/ fscks in between and you should see it.
>>>
>>> I've been going through my patch backlog, so I finally had a chance to
>>> take a very close look at your test image.  I now understand why
>>> things are failing.
>>>
>>> 1) The test image (which you said was generated on a ppc e2fsprogs?)
>>> was doing something very weird as far as the location of the
>>> allocation bitmaps and inode table:
>>
>> Yes, this was just during a fedora build, during the "make check" phase.
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=980519
>>
>> No idea why things should be coming out differently, that's a bit
>> alarming in and of itself.
>>
>> (Fedora isn't carrying any interesting patches to speak of).
> 
> But I am doing this:
> 
>  %check
> +# XXX ERS Hack for now; this bug has existed for a while,
> +# i.e. it is not a regression in this release, but there
> +# is no fix yet, and we need to get this package building.
> +# See Bug 987133 - resize2fs tests failing on ppc, s390
> +rm -rf tests/r_1024_small_bg*
> +rm -rf tests/r_64bit_big_expand*
> +rm -rf tests/r_bigalloc_big_expand*
> +rm -rf tests/r_ext4_big_expand*
>  make check
> 
> I'll retest w/ your patches, thanks.

Now all are passing on ppc64, last 3 are still failing on s390.  :(

-Eric

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

Patch

diff --git a/resize/resize2fs.c b/resize/resize2fs.c
index 204b10a..27dc5e6 100644
--- a/resize/resize2fs.c
+++ b/resize/resize2fs.c
@@ -891,18 +891,25 @@  static errcode_t blocks_to_move(ext2_resize_t rfs)
 	new_size = ext2fs_blocks_count(fs->super);
 	if (new_size < ext2fs_blocks_count(old_fs->super)) {
 		for (g = 0; g < fs->group_desc_count; g++) {
+			int realloc = 0;
 			/*
 			 * ext2fs_allocate_group_table re-allocates bitmaps
 			 * which are set to block 0.
 			 */
 			if (ext2fs_block_bitmap_loc(fs, g) >= new_size) {
 				ext2fs_block_bitmap_loc_set(fs, g, 0);
-				retval = ext2fs_allocate_group_table(fs, g, 0);
-				if (retval)
-					return retval;
+				realloc = 1;
 			}
 			if (ext2fs_inode_bitmap_loc(fs, g) >= new_size) {
 				ext2fs_inode_bitmap_loc_set(fs, g, 0);
+				realloc = 1;
+			}
+			if (ext2fs_inode_table_loc(fs, g) + fs->inode_blocks_per_group > new_size) {
+				ext2fs_inode_table_loc_set(fs, g, 0);
+				realloc = 1;
+			}
+
+			if (realloc) {
 				retval = ext2fs_allocate_group_table(fs, g, 0);
 				if (retval)
 					return retval;