| Message ID | 20220614044647.21846-1-hanjinke.666@bytedance.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | ext4: fix trim range leak | expand |
On Tue, Jun 14, 2022 at 12:46:47PM +0800, Jinke Han wrote: > From: hanjinke <hanjinke.666@bytedance.com> > > 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@bytedance.com> > --- > 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 >
hi thanks for your reply. Your point mentioned in the last email is very useful to me. I also think performance gains should be based on impeccable logic and the semantic of trim should be promised too. Can I send a patch v2 based on your suggestion ? Jinke 在 2022/6/15 下午4:40, Lukas Czerner 写道: > On Tue, Jun 14, 2022 at 12:46:47PM +0800, Jinke Han wrote: >> From: hanjinke <hanjinke.666@bytedance.com> >> >> 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@bytedance.com> >> --- >> 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 >> >
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;