Re: [PATCH 11/13] ext4: correct gdblock calculation in add_new_gdb_meta_bg to support non first group

[Kemeng Shi]

on 8/18/2023 12:07 PM, Theodore Ts'o wrote:
> On Fri, Aug 18, 2023 at 10:29:52AM +0800, Kemeng Shi wrote:
>> Actually, there seems a functional change to add_new_gdb_meta_bg.
>> Assume 'group' is the new added group, 'first_group' is the first group
>> of meta_bg which contains 'group',
>> Original way to calculate gdbblock:
>> gdbblock = group_first_block('first_group') + bg_has_super(*'group'*)
>> New ay to calculate gdbblock
>> gdbblock = group_first_block('first_group') + bg_has_super(*'first_group'*)
>> If new added group is not the first group of meta_bg, add_new_gdb_meta_bg
>> get a wrong gdbblock.
> 
> If you look at the ext4_add_new_descs() function,
> add_new_gdb_meta_bg() is only called when the group is a multiple of
> EXT4_DESC_PER_BLOCK --- that is, when group % EXT4_DESC_PER_BLOCK == 0.
>
> As such, it is only called when with group is the first group in the
> meta_bg.  So there is no bug here.  The code is bit confusing, I agree
> --- I myself got confused because it's been years since I last looked
> at the code, and it's not particularly commented well, which is my fault.
> 
Yes, add_new_gdb_meta_bg is only called with first group of mebg and no real
bug here.
> This also makes the commit description "... to support non-first
> group" incorrect, since it never gets called as with a "non-first
> group".
> 
Ah, what I want to say is "support non-frist group in fulture". And if there
is definely no need in fulture, it's more intuitive just treat group from caller
as first group in meta_bg.
> The patch makes things a little simpler, but the commit description
> would confuse anyone who looked at it while doing code archeology.
> The change is fine, although at this point, given how we both
> misunderstood how the code worked without doing some deep mind-melds
> with the C code in question, it's clear that we need some better
> comments in the code.
> 
> For example, the comment "add_new_gdb_meta_bg is the sister of
> add_new_gdb" is clearly insufficient.
> Is following comment looks good to you:
When all reserved primary blocks are consumed, we create meta_bg group and
allocate new primary block at first block or block after backup superblock
(if exsiting) in first group of meta_bg group.
This function is only called when first group of meta_bg is added.
> 						- Ted
>