[PATCH] ext4: fix trim range leak

[PATCH] ext4: fix trim range leak

[Jinke Han]

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.

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

Re: [PATCH] ext4: fix trim range leak

[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
> 

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@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
>>
>