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

[Theodore Ts'o]

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.

This also makes the commit description "... to support non-first
group" incorrect, since it never gets called as with a "non-first
group".

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.

						- Ted