linux-kernel - Re: [PATCH] ext4: fix inconsistent between segment fstrim and full fstrim

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Sat, 16 Dec 2023 09:01:29 +0800
From: "yebin (H)" <yebin10@...wei.com>
To: Jan Kara <jack@...e.cz>
CC: <tytso@....edu>, <adilger.kernel@...ger.ca>, <linux-ext4@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ext4: fix inconsistent between segment fstrim and full
 fstrim



On 2023/12/15 19:41, Jan Kara wrote:
> On Thu 14-12-23 14:46:35, Ye Bin wrote:
>> There will not issue discard cmd when do segment fstrim for ext4 fs, however,
>> if full fstrim for the same fs will issue discard cmd.
>> Above issue may happens as follows:
>> Precondition:
>> 1. Fstrim range [0, 15] and [16, 31];
>> 2. Discard granularity is 16;
>>              Range1          Range2
>>        1111000000000000 0000111010101011
>> There's no free space length large or equal than 16 in 'Range1' or 'Range2'.
>> As ext4_try_to_trim_range() only search free space among range which user
>> specified. However, there's maximum free space length 16 in 'Range1'+ 'Range2'.
>> To solve above issue, we need to find the longest free space to discard.
> The patch looks good so feel free to add:
>
> Reviewed-by: Jan Kara <jack@...e.cz>
>
> I'd just rephrase the changelog to make it a bit easier to read:
>
> Suppose we issue two FITRIM ioctls for ranges [0,15] and [16,31] with
> mininum length of trimmed range set to 8 blocks. If we have say a range of
> blocks 10-22 free, this range will not be trimmed because it straddles the
> boundary of the two FITRIM ranges and neither part is big enough. This is a
> bit surprising to some users that call FITRIM on smaller ranges of blocks
> to limit impact on the system. Also XFS trims all free space extents that
> overlap with the specified range so we are inconsistent among filesystems.
> Let's change ext4_try_to_trim_range() to consider for trimming the whole
> free space extent that straddles the end of specified range, not just the
> part of it within the range.
>
> 								Honza
>   
Thank you very much for your clear explanation of the patch.  I'll 
update patch's
changelog and resend a version.
>> Signed-off-by: Ye Bin <yebin10@...wei.com>
>> ---
>>   fs/ext4/mballoc.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>> index d72b5e3c92ec..d195461123d8 100644
>> --- a/fs/ext4/mballoc.c
>> +++ b/fs/ext4/mballoc.c
>> @@ -6753,13 +6753,15 @@ 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, free_count, last, origin_start;
>>   	bool set_trimmed = false;
>>   	void *bitmap;
>>   
>> +	last = ext4_last_grp_cluster(sb, e4b->bd_group);
>>   	bitmap = e4b->bd_bitmap;
>> -	if (start == 0 && max >= ext4_last_grp_cluster(sb, e4b->bd_group))
>> +	if (start == 0 && max >= last)
>>   		set_trimmed = true;
>> +	origin_start = start;
>>   	start = max(e4b->bd_info->bb_first_free, start);
>>   	count = 0;
>>   	free_count = 0;
>> @@ -6768,7 +6770,10 @@ __releases(ext4_group_lock_ptr(sb, e4b->bd_group))
>>   		start = mb_find_next_zero_bit(bitmap, max + 1, start);
>>   		if (start > max)
>>   			break;
>> -		next = mb_find_next_bit(bitmap, max + 1, start);
>> +
>> +		next = mb_find_next_bit(bitmap, last + 1, start);
>> +		if (origin_start == 0 && next >= last)
>> +			set_trimmed = true;
>>   
>>   		if ((next - start) >= minblocks) {
>>   			int ret = ext4_trim_extent(sb, start, next - start, e4b);
>> -- 
>> 2.31.1
>>