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

[hanjinke <hanjinke.666@bytedance.com>]

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@bytedance.com>
>>
>> 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@bytedance.com>
>> ---
>>   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
>>
>