Re: [PATCH 12/13] ext4: remove unnecessary check for avoiding multiple update_backups in ext4_flex_group_add

[Kemeng Shi]

on 8/17/2023 10:11 PM, Theodore Ts'o wrote:
> On Thu, Aug 17, 2023 at 11:53:52AM +0800, Kemeng Shi wrote:
>>
>>
>> on 8/16/2023 11:47 AM, Theodore Ts'o wrote:
>>> On Thu, Jun 29, 2023 at 08:00:43PM +0800, Kemeng Shi wrote:
>>>> Commit 0acdb8876fead ("ext4: don't call update_backups() multiple times
>>>> for the same bg") add check in ext4_flex_group_add to avoid call
>>>> update_backups multiple times for block group descriptors in the same
>>>> group descriptor block. However, we have already call update_backups in
>>>> block step, so the added check is unnecessary.
>>>
>>> I'm having trouble understaind this comit.  What do you mean by the
>>> "block step" in the last paragraph?
>>>
>> Sorry for the confusing stuff. I mean we foreach in group descriptor block
>> step instead of foreach in group descriptor step to update backup.
>> So there is no chance to call update_backups for different descriptors
>> in the same bg.
> 
> I'm still confused.  This commit is not so much removing an
> unnecessary check as much as forcing update_backups() to be called for
> every single block group.  Right?
> 
Ah, I guess here is the thing I missed that make this confusing:
sbi->s_group_desc contains only primary block of each group. i.e.
sbi->s_group_desc['i'] is the primary gdb block of group 'i'. It's clear
that primary gdb blocks of different groups will not be the same. Then
all blocks in s_group_desc[*] should be different and the removed check
is unnecessary.
>From add_new_gdb and add_new_gdb_meta_bg, we can find that we always
add primary gdb block of new group to s_group_desc. To be more specific:
add_new_gdb
	gdblock = EXT4_SB(sb)->s_sbh->b_blocknr + 1 + gdb_num;
	gdb_bh = ext4_sb_bread(sb, gdblock, 0);
	n_group_desc[gdb_num] = gdb_bh;

add_new_gdb_meta_bg
	gdblock = ext4_meta_bg_first_block_no(sb, group) +
		  ext4_bg_has_super(sb, group);
	gdb_bh = ext4_sb_bread(sb, gdblock, 0);
	n_group_desc[gdb_num] = gdb_bh;
	
> So either we're doing extra work, or (b) we're fixing a problem
> because we actually *need* to call update_backups() for every single
> block group.
> 
> More generally, this whole patch series is making it clear that the
> online resize code is hard to understand, because it's super
> complicated, leading to potential bugs, and very clearly code which is
> very hard to maintain.  So this may mean we need better comments to
> make it clear *when* the backup block groups are going to be
> accomplished, given the various different cases (e.g., no flex_bg or
> meta_bg, with flex_bg, or with meat_bg).
> 
Yes, I agree with this. I wonder if a series to add comments of some
common rules is good to you. Like the information mentioned above
that s_group_desc contains primary gdb block of each group.

> 							- Ted
>