Re: [PATCH 3/4] ext4: avoid bb_free and bb_fragments inconsistency in mb_free_blocks()

[Baokun Li]

On 2023/12/18 23:14, Jan Kara wrote:
> On Mon 18-12-23 22:18:13, Baokun Li wrote:
> > After updating bb_free in mb_free_blocks, it is possible to return without
> > updating bb_fragments because the block being freed is found to have
> > already been freed, which leads to inconsistency between bb_free and
> > bb_fragments.
> >
> > Since the group may be unlocked in ext4_grp_locked_error(), this can lead
> > to problems such as dividing by zero when calculating the average fragment
> > length. Therefore, to ensure consistency, move the update of bb_free to
> > after the block double-free check.
> >
> > Fixes: eabe0444df90 ("ext4: speed-up releasing blocks on commit")
> > CC: stable@xxxxxxxxxxxxxxx # 3.10
> > Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
> I agree there's no point in updating the allocation info if the bitmap is
> corrupted. We will not try to allocate (or free) blocks in that group
> anymore. I'm just a bit unsure about the EXT4_FC_REPLAY state where we
> don't mark the bitmap as corrupted although some blocks were already marked
> as freed. So in this case the free space statistics tracking will go
> permanently wrong. I'm kind of wondering in which case does fast-commit
> free already freed blocks. Ted, any idea?
>
> 								Honza
Some additional information, this judgment was introduced in
commit 8016e29f4362 ("ext4: fast commit recovery path") in v5.10-rc1,
at which point mb_regenerate_buddy() was called to regenerate the
buddy when it was found to be freeing a block that had already been
freed, so there was no problem. Until v5.11-rc1 commit 6bd97bf273bd
("ext4: remove redundant mb_regenerate_buddy()") removes the logic
to regenerate the buddy, it looks like the free space statistics will
remain wrong. If this normal scenario exists, perhaps buddy should
be regenerated here?

Thanks,
Baokun
> > ---
> >   fs/ext4/mballoc.c | 13 ++++++-------
> >   1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> > index a95fa6e2b0f9..2fbee0f0f5c3 100644
> > --- a/fs/ext4/mballoc.c
> > +++ b/fs/ext4/mballoc.c
> > @@ -1892,11 +1892,6 @@ static void mb_free_blocks(struct inode *inode, struct ext4_buddy *e4b,
> >   	mb_check_buddy(e4b);
> >   	mb_free_blocks_double(inode, e4b, first, count);
> >   
> > -	this_cpu_inc(discard_pa_seq);
> > -	e4b->bd_info->bb_free += count;
> > -	if (first < e4b->bd_info->bb_first_free)
> > -		e4b->bd_info->bb_first_free = first;
> > -
> >   	/* access memory sequentially: check left neighbour,
> >   	 * clear range and then check right neighbour
> >   	 */
> > @@ -1922,9 +1917,14 @@ static void mb_free_blocks(struct inode *inode, struct ext4_buddy *e4b,
> >   				sb, e4b->bd_group,
> >   				EXT4_GROUP_INFO_BBITMAP_CORRUPT);
> >   		}
> > -		goto done;
> > +		return;
> >   	}
> >   
> > +	this_cpu_inc(discard_pa_seq);
> > +	e4b->bd_info->bb_free += count;
> > +	if (first < e4b->bd_info->bb_first_free)
> > +		e4b->bd_info->bb_first_free = first;
> > +
> >   	/* let's maintain fragments counter */
> >   	if (left_is_free && right_is_free)
> >   		e4b->bd_info->bb_fragments--;
> > @@ -1949,7 +1949,6 @@ static void mb_free_blocks(struct inode *inode, struct ext4_buddy *e4b,
> >   	if (first <= last)
> >   		mb_buddy_mark_free(e4b, first >> 1, last >> 1);
> >   
> > -done:
> >   	mb_set_largest_free_order(sb, e4b->bd_info);
> >   	mb_update_avg_fragment_size(sb, e4b->bd_info);
> >   	mb_check_buddy(e4b);
> > --
> > 2.31.1