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.
>
> Signed-off-by: Ye Bin <[email protected]>
OK, I agree that there is this behavioral difference. However is that a
practical problem? I mean I would not expect the range to be particularly
small, rather something like 1GB and then these boundary conditions don't
really matter. This is also sensible so that we can properly track whether
the whole block group was trimmed or not. Finally I'd also argue that
trimming outside of specified range might be unexpected for the user. So a
*fix* for this in my opinion lays in userspace which needs to select
sensible ranges to use for trimming.
Honza
> ---
> 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
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On 2023/12/14 16:58, 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.
>>
>> Signed-off-by: Ye Bin <[email protected]>
> OK, I agree that there is this behavioral difference. However is that a
> practical problem? I mean I would not expect the range to be particularly
> small, rather something like 1GB and then these boundary conditions don't
> really matter. This is also sensible so that we can properly track whether
> the whole block group was trimmed or not. Finally I'd also argue that
> trimming outside of specified range might be unexpected for the user. So a
> *fix* for this in my opinion lays in userspace which needs to select
> sensible ranges to use for trimming.
>
> Honza
Thanks for your reply.
Our product fstrim entire file system, found to take a long time, thus
affecting other processes.
So they want to segment the file system fstrim based on the IO of the
system. But they found
that fragmented fstrims didn't work the same as fstrim for the entire
file system.
Users do not know the distribution of free blocks in the file system,
and they do not know the
reasonable range. The user's simple perception is that the effect of
segmented fstrim and full
fstrim should be consistent.
I researched the implementation of fstrim on the XFS file system, and
for the scenario described
in my patch, the results of both operations are consistent.
>> ---
>> 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
>>