2008-07-17 09:33:42

by Frédéric Bohé

Subject: [PATCH][e2fsprogs] Fix inode table allocation with flexbg

From: Frederic Bohe <[email protected]>

Disordered inode tables may appear when inode_blocks_per_group is lesser
or equal to the number of groups in a flex group.

Signed-off-by: Frederic Bohe <[email protected]>
---
This bug can be reproduced with:
mkfs.ext4 -t ext4dev -G512 70G

In that case, you can see with dump2fs that inode tables for groups 510
and 511 are placed just after group 51's inode table instead of being
placed after group 509's inode table.

alloc_tables.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)

Index: e2fsprogs/lib/ext2fs/alloc_tables.c
===================================================================
--- e2fsprogs.orig/lib/ext2fs/alloc_tables.c 2008-07-17 10:33:56.000000000 +0200
+++ e2fsprogs/lib/ext2fs/alloc_tables.c 2008-07-17 10:46:49.000000000 +0200
@@ -34,9 +34,10 @@
* tables can be allocated continously and in order.
*/
static blk_t flexbg_offset(ext2_filsys fs, dgrp_t group, blk_t start_blk,
- ext2fs_block_bitmap bmap, int offset, int size)
+ ext2fs_block_bitmap bmap, int offset, int size,
+ int elem_size)
{
- int flexbg, flexbg_size, elem_size;
+ int flexbg, flexbg_size;
blk_t last_blk, first_free = 0;
dgrp_t last_grp;

@@ -54,10 +55,6 @@ static blk_t flexbg_offset(ext2_filsys f
* search is still valid.
*/
if (start_blk && group % flexbg_size) {
- if (size > flexbg_size)
- elem_size = fs->inode_blocks_per_group;
- else
- elem_size = 1;
if (ext2fs_test_block_bitmap_range(bmap, start_blk + elem_size,
size))
return start_blk + elem_size;
@@ -126,7 +123,7 @@ errcode_t ext2fs_allocate_group_table(ex
if (group && fs->group_desc[group-1].bg_block_bitmap)
prev_block = fs->group_desc[group-1].bg_block_bitmap;
start_blk = flexbg_offset(fs, group, prev_block, bmap,
- 0, rem_grps);
+ 0, rem_grps, 1);
last_blk = ext2fs_group_last_block(fs, last_grp);
}

@@ -154,7 +151,7 @@ errcode_t ext2fs_allocate_group_table(ex
if (group && fs->group_desc[group-1].bg_inode_bitmap)
prev_block = fs->group_desc[group-1].bg_inode_bitmap;
start_blk = flexbg_offset(fs, group, prev_block, bmap,
- flexbg_size, rem_grps);
+ flexbg_size, rem_grps, 1);
last_blk = ext2fs_group_last_block(fs, last_grp);
}

@@ -187,7 +184,8 @@ errcode_t ext2fs_allocate_group_table(ex
group_blk = flexbg_offset(fs, group, prev_block, bmap,
flexbg_size * 2,
fs->inode_blocks_per_group *
- rem_grps);
+ rem_grps,
+ fs->inode_blocks_per_group);
last_blk = ext2fs_group_last_block(fs, last_grp);
}

--

2008-07-17 13:48:14

by Jose R. Santos

Subject: Re: [PATCH][e2fsprogs] Fix inode table allocation with flexbg

On Thu, 17 Jul 2008 13:34:41 +0200
Frédéric Bohé <[email protected]> wrote:

> From: Frederic Bohe <[email protected]>
>
> Disordered inode tables may appear when inode_blocks_per_group is lesser
> or equal to the number of groups in a flex group.
>

Acked-by: Jose R. Santos <[email protected]> with some comments bellow

> Signed-off-by: Frederic Bohe <[email protected]>
> ---
> This bug can be reproduced with:
> mkfs.ext4 -t ext4dev -G512 70G
>
> In that case, you can see with dump2fs that inode tables for groups 510
> and 511 are placed just after group 51's inode table instead of being
> placed after group 509's inode table.
>
> alloc_tables.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> Index: e2fsprogs/lib/ext2fs/alloc_tables.c
> ===================================================================
> --- e2fsprogs.orig/lib/ext2fs/alloc_tables.c 2008-07-17 10:33:56.000000000 +0200
> +++ e2fsprogs/lib/ext2fs/alloc_tables.c 2008-07-17 10:46:49.000000000 +0200
> @@ -34,9 +34,10 @@
> * tables can be allocated continously and in order.
> */
> static blk_t flexbg_offset(ext2_filsys fs, dgrp_t group, blk_t start_blk,
> - ext2fs_block_bitmap bmap, int offset, int size)
> + ext2fs_block_bitmap bmap, int offset, int size,
> + int elem_size)

While there is nothing technically wrong with the patch I think it
exposes an issue with the original flexbg_offset function. The routine
right now requires to many arguments and you need to do some
calculations within some of the arguments which make this look ugly
and error prone if someone decides to modify this code later on.

I've been thinking about rewriting this function so it looks something
like this:

static blk64_t flexbg_offset(ext2_filsys fs, ext2fs_block_bitmap bmap,
dgrp_t group, unsigned int type)

and do all the offset, start_blk, size, elem_size calculations inside
the routine. I need to make a long term fix for this issue since I
think 7 args for this routine is just to much.

-JRS