From: hanjinke <[email protected]>
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 <[email protected]>
---
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
On Tue, Jun 14, 2022 at 12:46:47PM +0800, Jinke Han wrote:
> From: hanjinke <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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
>>
>