In commit a015434480dc("ext4: send parallel discards on commit
completions"), issue all discard commands in parallel make all
bios could merged into one request, so lowlevel drive can issue
multi segments in one time which is more efficiency, but commit
55cdd0af2bc5 ("ext4: get discard out of jbd2 commit kthread contex")
seems broke this way, let's fix it.
In my test:
1. create 10 normal files, each file size is 10G.
2. deallocate file, punch a 16k holes every 32k.
3. trim all fs.
the time of fstrim fs reduce from 6.7s to 1.3s.
Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-lkp/[email protected]
Signed-off-by: Fengnan Chang <[email protected]>
---
fs/ext4/mballoc.c | 49 +++++++++++++++++++++++++++++++++++++++++------
1 file changed, 43 insertions(+), 6 deletions(-)
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index a2475b8c9fb5..b75ca1df0d30 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -6790,7 +6790,8 @@ int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
* be called with under the group lock.
*/
static int ext4_trim_extent(struct super_block *sb,
- int start, int count, struct ext4_buddy *e4b)
+ int start, int count, bool noalloc, struct ext4_buddy *e4b,
+ struct bio **biop, struct ext4_free_data **entryp)
__releases(bitlock)
__acquires(bitlock)
{
@@ -6812,9 +6813,16 @@ __acquires(bitlock)
*/
mb_mark_used(e4b, &ex);
ext4_unlock_group(sb, group);
- ret = ext4_issue_discard(sb, group, start, count, NULL);
+ ret = ext4_issue_discard(sb, group, start, count, biop);
+ if (!ret && !noalloc) {
+ struct ext4_free_data *entry = kmem_cache_alloc(ext4_free_data_cachep,
+ GFP_NOFS|__GFP_NOFAIL);
+ entry->efd_start_cluster = start;
+ entry->efd_count = count;
+ *entryp = entry;
+ }
+
ext4_lock_group(sb, group);
- mb_free_blocks(NULL, e4b, start, ex.fe_len);
return ret;
}
@@ -6824,26 +6832,40 @@ 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, bak;
void *bitmap;
+ struct ext4_free_data *entry = NULL, *fd, *nfd;
+ struct list_head discard_data_list;
+ struct bio *discard_bio = NULL;
+ struct blk_plug plug;
+ bool noalloc = false;
+
+ INIT_LIST_HEAD(&discard_data_list);
bitmap = e4b->bd_bitmap;
start = (e4b->bd_info->bb_first_free > start) ?
e4b->bd_info->bb_first_free : start;
count = 0;
free_count = 0;
+ bak = start;
+ blk_start_plug(&plug);
while (start <= max) {
start = mb_find_next_zero_bit(bitmap, max + 1, start);
if (start > max)
break;
next = mb_find_next_bit(bitmap, max + 1, start);
+ /* when only one segment, there is no need to alloc entry */
+ noalloc = (free_count == 0) && (next >= max);
if ((next - start) >= minblocks) {
- int ret = ext4_trim_extent(sb, start, next - start, e4b);
+ int ret = ext4_trim_extent(sb, start, next - start, noalloc, e4b,
+ &discard_bio, &entry);
- if (ret && ret != -EOPNOTSUPP)
+ if (ret < 0)
break;
+ if (entry)
+ list_add_tail(&entry->efd_list, &discard_data_list);
count += next - start;
}
free_count += next - start;
@@ -6863,6 +6885,21 @@ __releases(ext4_group_lock_ptr(sb, e4b->bd_group))
if ((e4b->bd_info->bb_free - free_count) < minblocks)
break;
}
+ if (discard_bio) {
+ ext4_unlock_group(sb, e4b->bd_group);
+ submit_bio_wait(discard_bio);
+ bio_put(discard_bio);
+ ext4_lock_group(sb, e4b->bd_group);
+ }
+ blk_finish_plug(&plug);
+
+ if (noalloc)
+ mb_free_blocks(NULL, e4b, bak, free_count);
+
+ list_for_each_entry_safe(fd, nfd, &discard_data_list, efd_list) {
+ mb_free_blocks(NULL, e4b, fd->efd_start_cluster, fd->efd_count);
+ kmem_cache_free(ext4_free_data_cachep, fd);
+ }
return count;
}
--
2.37.1 (Apple Git-137.1)
On 7/25/23 20:18, Fengnan Chang wrote:
> In commit a015434480dc("ext4: send parallel discards on commit
> completions"), issue all discard commands in parallel make all
> bios could merged into one request, so lowlevel drive can issue
> multi segments in one time which is more efficiency, but commit
> 55cdd0af2bc5 ("ext4: get discard out of jbd2 commit kthread contex")
> seems broke this way, let's fix it.
> In my test:
> 1. create 10 normal files, each file size is 10G.
> 2. deallocate file, punch a 16k holes every 32k.
> 3. trim all fs.
>
> the time of fstrim fs reduce from 6.7s to 1.3s.
>
> Reported-by: kernel test robot <[email protected]>
> Closes: https://lore.kernel.org/oe-lkp/[email protected]
> Signed-off-by: Fengnan Chang <[email protected]>
> ---
> fs/ext4/mballoc.c | 49 +++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 43 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index a2475b8c9fb5..b75ca1df0d30 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -6790,7 +6790,8 @@ int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
> * be called with under the group lock.
> */
> static int ext4_trim_extent(struct super_block *sb,
> - int start, int count, struct ext4_buddy *e4b)
> + int start, int count, bool noalloc, struct ext4_buddy *e4b,
> + struct bio **biop, struct ext4_free_data **entryp)
> __releases(bitlock)
> __acquires(bitlock)
> {
> @@ -6812,9 +6813,16 @@ __acquires(bitlock)
> */
> mb_mark_used(e4b, &ex);
> ext4_unlock_group(sb, group);
> - ret = ext4_issue_discard(sb, group, start, count, NULL);
> + ret = ext4_issue_discard(sb, group, start, count, biop);
> + if (!ret && !noalloc) {
> + struct ext4_free_data *entry = kmem_cache_alloc(ext4_free_data_cachep,
> + GFP_NOFS|__GFP_NOFAIL);
> + entry->efd_start_cluster = start;
> + entry->efd_count = count;
> + *entryp = entry;
> + }
> +
> ext4_lock_group(sb, group);
> - mb_free_blocks(NULL, e4b, start, ex.fe_len);
> return ret;
> }
>
> @@ -6824,26 +6832,40 @@ 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, bak;
> void *bitmap;
> + struct ext4_free_data *entry = NULL, *fd, *nfd;
> + struct list_head discard_data_list;
> + struct bio *discard_bio = NULL;
> + struct blk_plug plug;
> + bool noalloc = false;
> +
> + INIT_LIST_HEAD(&discard_data_list);
>
> bitmap = e4b->bd_bitmap;
> start = (e4b->bd_info->bb_first_free > start) ?
> e4b->bd_info->bb_first_free : start;
> count = 0;
> free_count = 0;
> + bak = start;
>
> + blk_start_plug(&plug);
> while (start <= max) {
> start = mb_find_next_zero_bit(bitmap, max + 1, start);
> if (start > max)
> break;
> next = mb_find_next_bit(bitmap, max + 1, start);
> + /* when only one segment, there is no need to alloc entry */
> + noalloc = (free_count == 0) && (next >= max);
>
> if ((next - start) >= minblocks) {
> - int ret = ext4_trim_extent(sb, start, next - start, e4b);
> + int ret = ext4_trim_extent(sb, start, next - start, noalloc, e4b,
> + &discard_bio, &entry);
>
> - if (ret && ret != -EOPNOTSUPP)
> + if (ret < 0)
> break;
> + if (entry)
> + list_add_tail(&entry->efd_list, &discard_data_list);
> count += next - start;
> }
> free_count += next - start;
> @@ -6863,6 +6885,21 @@ __releases(ext4_group_lock_ptr(sb, e4b->bd_group))
> if ((e4b->bd_info->bb_free - free_count) < minblocks)
> break;
> }
> + if (discard_bio) {
> + ext4_unlock_group(sb, e4b->bd_group);
> + submit_bio_wait(discard_bio);
> + bio_put(discard_bio);
> + ext4_lock_group(sb, e4b->bd_group);
> + }
> + blk_finish_plug(&plug);
> +
> + if (noalloc)
> + mb_free_blocks(NULL, e4b, bak, free_count);
> +
> + list_for_each_entry_safe(fd, nfd, &discard_data_list, efd_list) {
> + mb_free_blocks(NULL, e4b, fd->efd_start_cluster, fd->efd_count);
> + kmem_cache_free(ext4_free_data_cachep, fd);
> + }
>
> return count;
> }
With the new version, I don't see big difference from my test.
Thanks,
Guoqing
Hi Ted, Andreas:
Any comments ?
Thanks.
Guoqing Jiang <[email protected]> 于2023年7月31日周一 10:27写道：
>
>
>
> On 7/25/23 20:18, Fengnan Chang wrote:
> > In commit a015434480dc("ext4: send parallel discards on commit
> > completions"), issue all discard commands in parallel make all
> > bios could merged into one request, so lowlevel drive can issue
> > multi segments in one time which is more efficiency, but commit
> > 55cdd0af2bc5 ("ext4: get discard out of jbd2 commit kthread contex")
> > seems broke this way, let's fix it.
> > In my test:
> > 1. create 10 normal files, each file size is 10G.
> > 2. deallocate file, punch a 16k holes every 32k.
> > 3. trim all fs.
> >
> > the time of fstrim fs reduce from 6.7s to 1.3s.
> >
> > Reported-by: kernel test robot <[email protected]>
> > Closes: https://lore.kernel.org/oe-lkp/[email protected]
> > Signed-off-by: Fengnan Chang <[email protected]>
> > ---
> > fs/ext4/mballoc.c | 49 +++++++++++++++++++++++++++++++++++++++++------
> > 1 file changed, 43 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> > index a2475b8c9fb5..b75ca1df0d30 100644
> > --- a/fs/ext4/mballoc.c
> > +++ b/fs/ext4/mballoc.c
> > @@ -6790,7 +6790,8 @@ int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
> > * be called with under the group lock.
> > */
> > static int ext4_trim_extent(struct super_block *sb,
> > - int start, int count, struct ext4_buddy *e4b)
> > + int start, int count, bool noalloc, struct ext4_buddy *e4b,
> > + struct bio **biop, struct ext4_free_data **entryp)
> > __releases(bitlock)
> > __acquires(bitlock)
> > {
> > @@ -6812,9 +6813,16 @@ __acquires(bitlock)
> > */
> > mb_mark_used(e4b, &ex);
> > ext4_unlock_group(sb, group);
> > - ret = ext4_issue_discard(sb, group, start, count, NULL);
> > + ret = ext4_issue_discard(sb, group, start, count, biop);
> > + if (!ret && !noalloc) {
> > + struct ext4_free_data *entry = kmem_cache_alloc(ext4_free_data_cachep,
> > + GFP_NOFS|__GFP_NOFAIL);
> > + entry->efd_start_cluster = start;
> > + entry->efd_count = count;
> > + *entryp = entry;
> > + }
> > +
> > ext4_lock_group(sb, group);
> > - mb_free_blocks(NULL, e4b, start, ex.fe_len);
> > return ret;
> > }
> >
> > @@ -6824,26 +6832,40 @@ 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, bak;
> > void *bitmap;
> > + struct ext4_free_data *entry = NULL, *fd, *nfd;
> > + struct list_head discard_data_list;
> > + struct bio *discard_bio = NULL;
> > + struct blk_plug plug;
> > + bool noalloc = false;
> > +
> > + INIT_LIST_HEAD(&discard_data_list);
> >
> > bitmap = e4b->bd_bitmap;
> > start = (e4b->bd_info->bb_first_free > start) ?
> > e4b->bd_info->bb_first_free : start;
> > count = 0;
> > free_count = 0;
> > + bak = start;
> >
> > + blk_start_plug(&plug);
> > while (start <= max) {
> > start = mb_find_next_zero_bit(bitmap, max + 1, start);
> > if (start > max)
> > break;
> > next = mb_find_next_bit(bitmap, max + 1, start);
> > + /* when only one segment, there is no need to alloc entry */
> > + noalloc = (free_count == 0) && (next >= max);
> >
> > if ((next - start) >= minblocks) {
> > - int ret = ext4_trim_extent(sb, start, next - start, e4b);
> > + int ret = ext4_trim_extent(sb, start, next - start, noalloc, e4b,
> > + &discard_bio, &entry);
> >
> > - if (ret && ret != -EOPNOTSUPP)
> > + if (ret < 0)
> > break;
> > + if (entry)
> > + list_add_tail(&entry->efd_list, &discard_data_list);
> > count += next - start;
> > }
> > free_count += next - start;
> > @@ -6863,6 +6885,21 @@ __releases(ext4_group_lock_ptr(sb, e4b->bd_group))
> > if ((e4b->bd_info->bb_free - free_count) < minblocks)
> > break;
> > }
> > + if (discard_bio) {
> > + ext4_unlock_group(sb, e4b->bd_group);
> > + submit_bio_wait(discard_bio);
> > + bio_put(discard_bio);
> > + ext4_lock_group(sb, e4b->bd_group);
> > + }
> > + blk_finish_plug(&plug);
> > +
> > + if (noalloc)
> > + mb_free_blocks(NULL, e4b, bak, free_count);
> > +
> > + list_for_each_entry_safe(fd, nfd, &discard_data_list, efd_list) {
> > + mb_free_blocks(NULL, e4b, fd->efd_start_cluster, fd->efd_count);
> > + kmem_cache_free(ext4_free_data_cachep, fd);
> > + }
> >
> > return count;
> > }
>
> With the new version, I don't see big difference from my test.
>
> Thanks,
> Guoqing
ping
Fengnan Chang <[email protected]> 于2023年7月31日周一 20:52写道：
>
> Hi Ted, Andreas:
> Any comments ?
>
> Thanks.
>
> Guoqing Jiang <[email protected]> 于2023年7月31日周一 10:27写道：
> >
> >
> >
> > On 7/25/23 20:18, Fengnan Chang wrote:
> > > In commit a015434480dc("ext4: send parallel discards on commit
> > > completions"), issue all discard commands in parallel make all
> > > bios could merged into one request, so lowlevel drive can issue
> > > multi segments in one time which is more efficiency, but commit
> > > 55cdd0af2bc5 ("ext4: get discard out of jbd2 commit kthread contex")
> > > seems broke this way, let's fix it.
> > > In my test:
> > > 1. create 10 normal files, each file size is 10G.
> > > 2. deallocate file, punch a 16k holes every 32k.
> > > 3. trim all fs.
> > >
> > > the time of fstrim fs reduce from 6.7s to 1.3s.
> > >
> > > Reported-by: kernel test robot <[email protected]>
> > > Closes: https://lore.kernel.org/oe-lkp/[email protected]
> > > Signed-off-by: Fengnan Chang <[email protected]>
> > > ---
> > > fs/ext4/mballoc.c | 49 +++++++++++++++++++++++++++++++++++++++++------
> > > 1 file changed, 43 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> > > index a2475b8c9fb5..b75ca1df0d30 100644
> > > --- a/fs/ext4/mballoc.c
> > > +++ b/fs/ext4/mballoc.c
> > > @@ -6790,7 +6790,8 @@ int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
> > > * be called with under the group lock.
> > > */
> > > static int ext4_trim_extent(struct super_block *sb,
> > > - int start, int count, struct ext4_buddy *e4b)
> > > + int start, int count, bool noalloc, struct ext4_buddy *e4b,
> > > + struct bio **biop, struct ext4_free_data **entryp)
> > > __releases(bitlock)
> > > __acquires(bitlock)
> > > {
> > > @@ -6812,9 +6813,16 @@ __acquires(bitlock)
> > > */
> > > mb_mark_used(e4b, &ex);
> > > ext4_unlock_group(sb, group);
> > > - ret = ext4_issue_discard(sb, group, start, count, NULL);
> > > + ret = ext4_issue_discard(sb, group, start, count, biop);
> > > + if (!ret && !noalloc) {
> > > + struct ext4_free_data *entry = kmem_cache_alloc(ext4_free_data_cachep,
> > > + GFP_NOFS|__GFP_NOFAIL);
> > > + entry->efd_start_cluster = start;
> > > + entry->efd_count = count;
> > > + *entryp = entry;
> > > + }
> > > +
> > > ext4_lock_group(sb, group);
> > > - mb_free_blocks(NULL, e4b, start, ex.fe_len);
> > > return ret;
> > > }
> > >
> > > @@ -6824,26 +6832,40 @@ 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, bak;
> > > void *bitmap;
> > > + struct ext4_free_data *entry = NULL, *fd, *nfd;
> > > + struct list_head discard_data_list;
> > > + struct bio *discard_bio = NULL;
> > > + struct blk_plug plug;
> > > + bool noalloc = false;
> > > +
> > > + INIT_LIST_HEAD(&discard_data_list);
> > >
> > > bitmap = e4b->bd_bitmap;
> > > start = (e4b->bd_info->bb_first_free > start) ?
> > > e4b->bd_info->bb_first_free : start;
> > > count = 0;
> > > free_count = 0;
> > > + bak = start;
> > >
> > > + blk_start_plug(&plug);
> > > while (start <= max) {
> > > start = mb_find_next_zero_bit(bitmap, max + 1, start);
> > > if (start > max)
> > > break;
> > > next = mb_find_next_bit(bitmap, max + 1, start);
> > > + /* when only one segment, there is no need to alloc entry */
> > > + noalloc = (free_count == 0) && (next >= max);
> > >
> > > if ((next - start) >= minblocks) {
> > > - int ret = ext4_trim_extent(sb, start, next - start, e4b);
> > > + int ret = ext4_trim_extent(sb, start, next - start, noalloc, e4b,
> > > + &discard_bio, &entry);
> > >
> > > - if (ret && ret != -EOPNOTSUPP)
> > > + if (ret < 0)
> > > break;
> > > + if (entry)
> > > + list_add_tail(&entry->efd_list, &discard_data_list);
> > > count += next - start;
> > > }
> > > free_count += next - start;
> > > @@ -6863,6 +6885,21 @@ __releases(ext4_group_lock_ptr(sb, e4b->bd_group))
> > > if ((e4b->bd_info->bb_free - free_count) < minblocks)
> > > break;
> > > }
> > > + if (discard_bio) {
> > > + ext4_unlock_group(sb, e4b->bd_group);
> > > + submit_bio_wait(discard_bio);
> > > + bio_put(discard_bio);
> > > + ext4_lock_group(sb, e4b->bd_group);
> > > + }
> > > + blk_finish_plug(&plug);
> > > +
> > > + if (noalloc)
> > > + mb_free_blocks(NULL, e4b, bak, free_count);
> > > +
> > > + list_for_each_entry_safe(fd, nfd, &discard_data_list, efd_list) {
> > > + mb_free_blocks(NULL, e4b, fd->efd_start_cluster, fd->efd_count);
> > > + kmem_cache_free(ext4_free_data_cachep, fd);
> > > + }
> > >
> > > return count;
> > > }
> >
> > With the new version, I don't see big difference from my test.
> >
> > Thanks,
> > Guoqing