[6/8] ext4: add a helper function to validate metadata block

From: Chunguang Xu <brookxu@tencent.com>

There is a need to check whether a block or a segment overlaps
with metadata, since information of system_zone is incomplete,
we need a more accurate function. Now we check whether it
overlaps with block bitmap, inode bitmap, and inode table.
Perhaps it is better to add a check of super_block and block
group descriptor and provide a helper function.

Signed-off-by: Chunguang Xu <brookxu@tencent.com>
---
 fs/ext4/ext4.h    |  5 ++++-
 fs/ext4/mballoc.c | 57 +++++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 49 insertions(+), 13 deletions(-)

On Sat, Nov 07, 2020 at 11:58:16PM +0800, Chunguang Xu wrote:
> From: Chunguang Xu <brookxu@tencent.com>
> 
> There is a need to check whether a block or a segment overlaps
> with metadata, since information of system_zone is incomplete,
> we need a more accurate function. Now we check whether it
> overlaps with block bitmap, inode bitmap, and inode table.
> Perhaps it is better to add a check of super_block and block
> group descriptor and provide a helper function.

The original code was valid only for file systems that are not using
flex_bg.  I suspect the Lustre folks who implemented mballoc.c did so
before flex_bg, and fortunately, on flex_bg we the check is simply
going to have more false negaties, but not any false positives, so no
one noticed.

> +/*
> + * Returns 1 if the passed-in block region (block, block+count)
> + * overlaps with some other filesystem metadata blocks. Others,
> + * return 0.
> + */
> +int ext4_metadata_block_overlaps(struct super_block *sb,
> +				 ext4_group_t block_group,
> +				 ext4_fsblk_t block,
> +				 unsigned long count)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	struct ext4_group_desc *gdp;
> +	int gd_first = ext4_group_first_block_no(sb, block_group);
> +	int itable, gd_blk;
> +	int ret = 0;
> +
> +	gdp = ext4_get_group_desc(sb, block_group, NULL);
> +	// check block bitmap and inode bitmap
> +	if (in_range(ext4_block_bitmap(sb, gdp), block, count) ||
> +	    in_range(ext4_inode_bitmap(sb, gdp), block, count))

We are only checking a single block group descriptor; this is fine if
the allocation bitmaps and inode table are guaranteed to be located in
their own block group.  But this is no longer true when flex_bg is
enabled.

I think what we should do is to rely on the rb tree maintained by
block_validity.c (if the inode number is zero, then the entry refers
to blocks in the "system zone"); that's going to be a much more
complete check.

What do you think?

						- Ted

Theodore Y. Ts'o wrote on 2020/12/9 12:55:
> On Sat, Nov 07, 2020 at 11:58:16PM +0800, Chunguang Xu wrote:
>> From: Chunguang Xu <brookxu@tencent.com>
>>
>> There is a need to check whether a block or a segment overlaps
>> with metadata, since information of system_zone is incomplete,
>> we need a more accurate function. Now we check whether it
>> overlaps with block bitmap, inode bitmap, and inode table.
>> Perhaps it is better to add a check of super_block and block
>> group descriptor and provide a helper function.
> 
> The original code was valid only for file systems that are not using
> flex_bg.  I suspect the Lustre folks who implemented mballoc.c did so
> before flex_bg, and fortunately, on flex_bg we the check is simply
> going to have more false negaties, but not any false positives, so no
> one noticed.
> 
>> +/*
>> + * Returns 1 if the passed-in block region (block, block+count)
>> + * overlaps with some other filesystem metadata blocks. Others,
>> + * return 0.
>> + */
>> +int ext4_metadata_block_overlaps(struct super_block *sb,
>> +				 ext4_group_t block_group,
>> +				 ext4_fsblk_t block,
>> +				 unsigned long count)
>> +{
>> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
>> +	struct ext4_group_desc *gdp;
>> +	int gd_first = ext4_group_first_block_no(sb, block_group);
>> +	int itable, gd_blk;
>> +	int ret = 0;
>> +
>> +	gdp = ext4_get_group_desc(sb, block_group, NULL);
>> +	// check block bitmap and inode bitmap
>> +	if (in_range(ext4_block_bitmap(sb, gdp), block, count) ||
>> +	    in_range(ext4_inode_bitmap(sb, gdp), block, count))
> 
> We are only checking a single block group descriptor; this is fine if
> the allocation bitmaps and inode table are guaranteed to be located in
> their own block group.  But this is no longer true when flex_bg is
> enabled.

 Right, the check of bb and ib here is not very correct.

> I think what we should do is to rely on the rb tree maintained by
> block_validity.c (if the inode number is zero, then the entry refers
> to blocks in the "system zone"); that's going to be a much more
> complete check.
> 
> What do you think?

This is a good idea. After we merge ext4: add the gdt block of
meta_bg to system_zone, the metadata information of system_zone
is relatively complete. Using system_zone makes the logic
clearer. However, due to the need for additional tree search,
there is a performance risk. I will try this method later and
test the performance overhead.

> 						- Ted
>