Re: [External] Re: [PATCH] ext4: fix trim range leak

[hanjinke]

hi

thanks for your reply.

Your point mentioned in the last email is very useful to me.

I also think performance gains should be based on impeccable logic and 
the semantic of trim should be promised too.

Can I send a patch v2 based on your suggestion ?

Jinke

在 2022/6/15 下午4:40, Lukas Czerner 写道:
> On Tue, Jun 14, 2022 at 12:46:47PM +0800, Jinke Han wrote:
> > From: hanjinke <hanjinke.666@xxxxxxxxxxxxx>
> >
> > When release group lock, a large number of blocks may be alloc from
> > the group(e.g. not from the rest of target trim range). This may
> > lead end of the loop and leave the rest of trim range unprocessed.
>
> Hi,
>
> you're correct. Indeed it's possible to miss some of the blocks this
> way.
>
> But I wonder how much of a problem this actually is? I'd think that the
> optimization you just took out is very usefull, especially with larger
> minlen and more fragmented free space it'll save us a lot of cycles.
> Do you have any performance numbers for this change?
>
> Perhaps we don't have to remove it completely, rather zero the
> free_count every time bb_free changes? Would that be worth it?
>
> -Lukas
>
> > Signed-off-by: hanjinke <hanjinke.666@xxxxxxxxxxxxx>
> > ---
> >   fs/ext4/mballoc.c | 6 +-----
> >   1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> > index 9f12f29bc346..45eb9ee20947 100644
> > --- a/fs/ext4/mballoc.c
> > +++ b/fs/ext4/mballoc.c
> > @@ -6345,14 +6345,13 @@ static int ext4_try_to_trim_range(struct super_block *sb,
> >   __acquires(ext4_group_lock_ptr(sb, e4b->bd_group))
> >   __releases(ext4_group_lock_ptr(sb, e4b->bd_group))
> >   {
> > -	ext4_grpblk_t next, count, free_count;
> > +	ext4_grpblk_t next, count;
> >   	void *bitmap;
> >   
> >   	bitmap = e4b->bd_bitmap;
> >   	start = (e4b->bd_info->bb_first_free > start) ?
> >   		e4b->bd_info->bb_first_free : start;
> >   	count = 0;
> > -	free_count = 0;
> >   
> >   	while (start <= max) {
> >   		start = mb_find_next_zero_bit(bitmap, max + 1, start);
> > @@ -6367,7 +6366,6 @@ __releases(ext4_group_lock_ptr(sb, e4b->bd_group))
> >   				break;
> >   			count += next - start;
> >   		}
> > -		free_count += next - start;
> >   		start = next + 1;
> >   
> >   		if (fatal_signal_pending(current)) {
> > @@ -6381,8 +6379,6 @@ __releases(ext4_group_lock_ptr(sb, e4b->bd_group))
> >   			ext4_lock_group(sb, e4b->bd_group);
> >   		}
> >   
> > -		if ((e4b->bd_info->bb_free - free_count) < minblocks)
> > -			break;
> >   	}
> >   
> >   	return count;
> > --
> > 2.20.1