On Tue, Jun 14, 2022 at 12:46:47PM +0800, Jinke Han wrote:
> From: hanjinke <hanjinke.666@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> ---
> 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
>
