ext4: fix inconsistent between segment fstrim and full fstrim
PATCH 1/1 ext4: fix inconsistent between segment fstrim and full fstrim
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 0000111010101011There'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.
--- fs/ext4/mballoc.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
fs/ext4/mballoc.c +8 -3
@@ -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);
Signature · 1 line
2.31.1
Jan Kara
reviewerQuoted 8 lines · Ye Bin
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
Quoted 6 lines
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
Quoted 41 lines
--- fs/ext4/mballoc.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)fs/ext4/mballoc.c +8
-3@@ -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);Signature · 1 line
2.31.1
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 24 lines · Jan Kara
Quoted 8 lines · Ye Bin
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 0000111010101011Quoted 6 lines
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.
Quoted 1 line
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.
Quoted 41 lines
Quoted 41 lines
--- fs/ext4/mballoc.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)fs/ext4/mballoc.c +8
-3@@ -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);Signature · 1 line
2.31.1
Jan Kara
reviewerQuoted 28 lines · yebin (H)
Quoted 24 lines · Jan Kara
Quoted 8 lines · Ye Bin
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 0000111010101011Quoted 6 lines
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.
Quoted 1 line
Honza
Quoted 15 lines
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.
OK, we've discussed this on yesterday's ext4 call and we've came to a conclusion that we don't care that much and consistency among filesystems is good so I'll go back and review your patch.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Jan Kara
reviewerRemembered one more thing to note:
Quoted 26 lines · Jan Kara
Quoted 25 lines · yebin (H)
Quoted 24 lines · Jan Kara
Quoted 8 lines · Ye Bin
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 0000111010101011Quoted 6 lines
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.
Quoted 7 lines
Quoted 7 lines
Quoted 1 line
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.
So I agree that trimming the whole fs at once may take too long. But also note that if you make ranges smaller than block group size (128MB with 4k block size), then we will never record that the group has been fully trimmed and thus we will always trim all free extents in a group even though there was no allocation there since the last trim. So in this sense trims split to small ranges will not be equivalent to a large trim even after your change.
Quoted 4 lines
Quoted 4 lines
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.
Personally I don't see a good reason to make trim range smaller than 1GB, it just adds overhead, but I understand it may be a bit surprising.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 5 lines · Ye Bin
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];
Quoted 7 lines
2. Discard granularity is 16; Range1 Range2 1111000000000000 0000111010101011There'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:
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
Quoted 42 lines
Signed-off-by Ye Bin <[email protected]>--- fs/ext4/mballoc.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)fs/ext4/mballoc.c +8
-3@@ -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);Signature · 1 line
2.31.1
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 30 lines · Jan Kara
Quoted 5 lines · Ye Bin
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];Quoted 7 lines
2. Discard granularity is 16; Range1 Range2 1111000000000000 0000111010101011There'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 <[email protected]>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.
Quoted 2 lines
Honza
Thank you very much for your clear explanation of the patch. I'll update patch's changelog and resend a version.
Quoted 42 lines
Quoted 42 lines
Signed-off-by Ye Bin <[email protected]>--- fs/ext4/mballoc.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)fs/ext4/mballoc.c +8
-3@@ -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);Signature · 1 line
2.31.1