Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <29b3bbe6-24dc-d0ee-8426-7cb3b6cfbc1e@bytedance.com>
Date:   Thu, 16 Jun 2022 14:09:21 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.7.0
Subject: Re: [External] Re: [PATCH] ext4: fix trim range leak
To:     Lukas Czerner <lczerner@redhat.com>
Cc:     tytso@mit.edu, adilger.kernel@dilger.ca,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220614044647.21846-1-hanjinke.666@bytedance.com>
 <20220615084017.xwexup5ckrrpevhe@fedora>
From:   hanjinke <hanjinke.666@bytedance.com>
In-Reply-To: <20220615084017.xwexup5ckrrpevhe@fedora>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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