[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <657CF6E9.602@huawei.com>
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
>>
Powered by blists - more mailing lists