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

[Kemeng Shi]

on 8/17/2023 10:03 PM, Theodore Ts'o wrote:
> On Thu, Aug 17, 2023 at 11:38:34AM +0800, Kemeng Shi wrote:
>>
>>
>> on 8/16/2023 11:45 AM, Theodore Ts'o wrote:
>>> On Thu, Jun 29, 2023 at 08:00:42PM +0800, Kemeng Shi wrote:
>>>> In add_new_gdb_meta_bg, we assume that group could be non first
>>>> group in meta block group as we call ext4_meta_bg_first_block_no
>>>> to get first block of meta block group rather than call
>>>> ext4_group_first_block_no for passed group directly. Then ext4_bg_has_super
>>>> should be called with first group in meta group rather than new added
>>>> group. Or we can call ext4_group_first_block_no instead of
>>>> ext4_meta_bg_first_block_no to assume only first group of
>>>> meta group will be passed.
>>>> Either way, ext4_meta_bg_first_block_no will be useless and
>>>> could be removed.
>>>
>>> Unfortunately, I spent more time trying to understand the commit
>>> description than the C code.  Perhaps this might be a better way of
>>> describing the situation?
>>>
>> Sorry for my poor language again, :( I will try to improve this.
>>> The ext4_new descs() function calls ext4_meta_bg_first_block_no() with
>>> the group paramter when the group is the first group of a meta_bg
>>> (e.g., when (group % EXT4_DESC_PER_BLOCK) is zero.  So we can simplify
>>> things a bit by removing ext4_meta_bg_first_block_no() and an open
>>> coding its logic.
>>>
>>> Does this make more sense to tou?
>>>
>> This patch tries to correct gdbblock calculation in add_new_gdb_meta_bg
>> in case group from caller is not the first group of meta_bg which is
>> supposed to be handled by add_new_gdb_meta_bg.
>> We should call ext4_bg_has_super with first group in meta_bg instead
>> of group which could be non first group in meta_bg to calculate gdb
>> of meta_bg.
>> Fortunately, the only caller ext4_add_new_descs always call
>> add_new_gdb_meta_bg with first group of meta_bg and no real issue
>> will happen.
> 
> To be clear, this doesn't have a functional change given how the code
> is going to be used, right?  It's really more of a cleanup with a goal
> of making the code easier to understand.  If so, we should make this
> explicit at the beginning of the commit description, as opposed to
> putting it at the end.
> 
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.
Maybe it's more of a bugfix to as add_new_gdb_meta_bg treats group
as any group of meta_bg instead of first group of meta_bg. And it's
more of a cleanup as only first group is passed from caller.
> In journalism this is referred to as "burying the lede"[1], where the
> "lede" the most important/key piece of information.  In general, it is
> desirable not to "bury the lede".  That is, the most important
> information, including why people should care, and what this is doing,
> at the beginning of the commit description (or article in the case of
> journalsm).
> 
> [1] https://www.masterclass.com/articles/bury-the-lede-explained
> 
> 					- Ted
> 
Thanks for this information. But I'm little confused weather a cleanup
or a bugfix I should mention at the beginning. Any more advise is
appreciated!