Re: s_first_meta_bg treatment incompatibility between kernel and e2fsprogs

Damien Guibouret
2009-11-16T15:50:29Z

Hello,

Theodore Tso wrote:
> On Sun, Nov 15, 2009 at 11:28:13AM +0100, Damien Guibouret wrote:

[...]

> As far as the matter of taste issue is concerned, I think we already
> have too many static functions with a single caller, and it actually
> makes the code harder to understand. So adding yet another simple
> static function I think is a bad thing, not a good thing.
>

It was just to mimic the existing function, but I agree with you.
The other difference is that it shall be applied on ext3 also.

>
>>And I think there is some other places where kernel should be fixed when
>>it uses s_gdb_count (but here my knowledge of the sources are not deep
>>enough to be sure on what shall be performed).
>
>
> I've looked through the other areas, and the one place where I see a
> problem is in the block validity checks in ext4_iget() for the
> extended attribute block and in block_validity.c. The former can and
> should be fixed to use the latter.
>
> Here's the fix that I plan to be using. Comments, anyone?
>

For the first one (on block_validity.c), as far as I understand,
presence of superblock and descriptors blocks in a group are no more
related in case of meta_bg group, so shouldn't be the code divided into
2 distincts part: one to treat super block, second to treat descriptor
blocks (I do not understand the ((i < 5) || ((i % flex_size) == 0) part
into the test, so add it if it is trully needed), something as:
ext4_fsblk_t firstSystemBlock = ext4_group_first_block_no(sb, i);
unsigned long nbDescBlocks;
if (ext4_bg_has_super(sb, i)) {
add_system_zone(sbi, firstSystemBlock,
1);
firstSystemBlock++;
}
nbDescBlocks = ext4_bg_num_gdb(sb, i);
if (nbDescBlocks != 0)
add_system_zone(sbi, firstSystemBlock,
nbDescBlocks);

Regards,

Damien

> - Ted
>
> ext4: fix block validity checks so they work correctly with meta_bg
>
> The block validity checks used by ext4_data_block_valid() wasn't
> correctly written to check file systems with the meta_bg feature. Fix
> this.
>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
> fs/ext4/block_validity.c | 2 +-
> fs/ext4/inode.c | 5 +----
> 2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
> index 50784ef..dc79b75 100644
> --- a/fs/ext4/block_validity.c
> +++ b/fs/ext4/block_validity.c
> @@ -160,7 +160,7 @@ int ext4_setup_system_zone(struct super_block *sb)
> if (ext4_bg_has_super(sb, i) &&
> ((i < 5) || ((i % flex_size) == 0)))
> add_system_zone(sbi, ext4_group_first_block_no(sb, i),
> - sbi->s_gdb_count + 1);
> + ext4_bg_num_gdb(sb, i) + 1);
> gdp = ext4_get_group_desc(sb, i, NULL);
> ret = add_system_zone(sbi, ext4_block_bitmap(sb, gdp), 1);
> if (ret)
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index b5cdb88..c62ca93 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4886,10 +4886,7 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
>
> ret = 0;
> if (ei->i_file_acl &&
> - ((ei->i_file_acl <
> - (le32_to_cpu(EXT4_SB(sb)->s_es->s_first_data_block) +
> - EXT4_SB(sb)->s_gdb_count)) ||
> - (ei->i_file_acl >= ext4_blocks_count(EXT4_SB(sb)->s_es)))) {
> + !ext4_data_block_valid(EXT4_SB(sb), ei->i_file_acl, 1)) {
> ext4_error(sb, __func__,
> "bad extended attribute block %llu in inode #%lu",
> ei->i_file_acl, inode->i_ino);
>
>