diff mbox series

[v6] ext4: improve trim efficiency

Message ID 20230901092820.33757-1-changfengnan@bytedance.com
State New
Headers show
Series [v6] ext4: improve trim efficiency | expand

Commit Message

Fengnan Chang Sept. 1, 2023, 9:28 a.m. UTC 
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.

Signed-off-by: Fengnan Chang <changfengnan@bytedance.com>
---
 fs/ext4/mballoc.c | 95 +++++++++++++++++++++++++----------------------
 1 file changed, 51 insertions(+), 44 deletions(-)

Comments

Fengnan Chang Sept. 15, 2023, 9:19 a.m. UTC | #1 
ping

Fengnan Chang <changfengnan@bytedance.com> 于2023年9月1日周五 17:28写道：
>
> 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.
>
> Signed-off-by: Fengnan Chang <changfengnan@bytedance.com>
> ---
>  fs/ext4/mballoc.c | 95 +++++++++++++++++++++++++----------------------
>  1 file changed, 51 insertions(+), 44 deletions(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 1e4c667812a9..9fc69a92c496 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -6874,70 +6874,61 @@ int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
>         return err;
>  }
>
> -/**
> - * ext4_trim_extent -- function to TRIM one single free extent in the group
> - * @sb:                super block for the file system
> - * @start:     starting block of the free extent in the alloc. group
> - * @count:     number of blocks to TRIM
> - * @e4b:       ext4 buddy for the group
> - *
> - * Trim "count" blocks starting at "start" in the "group". To assure that no
> - * one will allocate those blocks, mark it as used in buddy bitmap. This must
> - * be called with under the group lock.
> - */
> -static int ext4_trim_extent(struct super_block *sb,
> -               int start, int count, struct ext4_buddy *e4b)
> -__releases(bitlock)
> -__acquires(bitlock)
> -{
> -       struct ext4_free_extent ex;
> -       ext4_group_t group = e4b->bd_group;
> -       int ret = 0;
> -
> -       trace_ext4_trim_extent(sb, group, start, count);
> -
> -       assert_spin_locked(ext4_group_lock_ptr(sb, group));
> -
> -       ex.fe_start = start;
> -       ex.fe_group = group;
> -       ex.fe_len = count;
> -
> -       /*
> -        * Mark blocks used, so no one can reuse them while
> -        * being trimmed.
> -        */
> -       mb_mark_used(e4b, &ex);
> -       ext4_unlock_group(sb, group);
> -       ret = ext4_issue_discard(sb, group, start, count, NULL);
> -       ext4_lock_group(sb, group);
> -       mb_free_blocks(NULL, e4b, start, ex.fe_len);
> -       return ret;
> -}
> -
>  static int ext4_try_to_trim_range(struct super_block *sb,
>                 struct ext4_buddy *e4b, ext4_grpblk_t start,
>                 ext4_grpblk_t max, ext4_grpblk_t minblocks)
>  __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;
> +       ext4_group_t group = e4b->bd_group;
> +       struct ext4_free_extent ex;
> +       bool noalloc = false;
> +       int ret = 0;
> +
> +       INIT_LIST_HEAD(&discard_data_list);
>
>         bitmap = e4b->bd_bitmap;
>         start = max(e4b->bd_info->bb_first_free, start);
>         count = 0;
>         free_count = 0;
>
> +       blk_start_plug(&plug);
>         while (start <= max) {
>                 start = mb_find_next_zero_bit(bitmap, max + 1, start);
>                 if (start > max)
>                         break;
> +               bak = start;
>                 next = mb_find_next_bit(bitmap, max + 1, start);
> -
>                 if ((next - start) >= minblocks) {
> -                       int ret = ext4_trim_extent(sb, start, next - start, e4b);
> +                       /* when only one segment, there is no need to alloc entry */
> +                       noalloc = (free_count == 0) && (next >= max);
>
> -                       if (ret && ret != -EOPNOTSUPP)
> +                       trace_ext4_trim_extent(sb, group, start, next - start);
> +                       ex.fe_start = start;
> +                       ex.fe_group = group;
> +                       ex.fe_len = next - start;
> +                       /*
> +                        * Mark blocks used, so no one can reuse them while
> +                        * being trimmed.
> +                        */
> +                       mb_mark_used(e4b, &ex);
> +                       ext4_unlock_group(sb, group);
> +                       ret = ext4_issue_discard(sb, group, start, next - start, &discard_bio);
> +                       if (!noalloc) {
> +                               entry = kmem_cache_alloc(ext4_free_data_cachep,
> +                                                       GFP_NOFS|__GFP_NOFAIL);
> +                               entry->efd_start_cluster = start;
> +                               entry->efd_count = next - start;
> +                               list_add_tail(&entry->efd_list, &discard_data_list);
> +                       }
> +                       ext4_lock_group(sb, group);
> +                       if (ret < 0)
>                                 break;
>                         count += next - start;
>                 }
> @@ -6959,6 +6950,22 @@ __releases(ext4_group_lock_ptr(sb, e4b->bd_group))
>                         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 && free_count)
> +               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.20.1
>
Fengnan Chang Oct. 12, 2023, 11:57 a.m. UTC | #2 
Hi Ted:
    any new comments ?

Fengnan Chang <changfengnan@bytedance.com> 于2023年9月1日周五 17:28写道：
>
> 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.
>
> Signed-off-by: Fengnan Chang <changfengnan@bytedance.com>
> ---
>  fs/ext4/mballoc.c | 95 +++++++++++++++++++++++++----------------------
>  1 file changed, 51 insertions(+), 44 deletions(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 1e4c667812a9..9fc69a92c496 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -6874,70 +6874,61 @@ int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
>         return err;
>  }
>
> -/**
> - * ext4_trim_extent -- function to TRIM one single free extent in the group
> - * @sb:                super block for the file system
> - * @start:     starting block of the free extent in the alloc. group
> - * @count:     number of blocks to TRIM
> - * @e4b:       ext4 buddy for the group
> - *
> - * Trim "count" blocks starting at "start" in the "group". To assure that no
> - * one will allocate those blocks, mark it as used in buddy bitmap. This must
> - * be called with under the group lock.
> - */
> -static int ext4_trim_extent(struct super_block *sb,
> -               int start, int count, struct ext4_buddy *e4b)
> -__releases(bitlock)
> -__acquires(bitlock)
> -{
> -       struct ext4_free_extent ex;
> -       ext4_group_t group = e4b->bd_group;
> -       int ret = 0;
> -
> -       trace_ext4_trim_extent(sb, group, start, count);
> -
> -       assert_spin_locked(ext4_group_lock_ptr(sb, group));
> -
> -       ex.fe_start = start;
> -       ex.fe_group = group;
> -       ex.fe_len = count;
> -
> -       /*
> -        * Mark blocks used, so no one can reuse them while
> -        * being trimmed.
> -        */
> -       mb_mark_used(e4b, &ex);
> -       ext4_unlock_group(sb, group);
> -       ret = ext4_issue_discard(sb, group, start, count, NULL);
> -       ext4_lock_group(sb, group);
> -       mb_free_blocks(NULL, e4b, start, ex.fe_len);
> -       return ret;
> -}
> -
>  static int ext4_try_to_trim_range(struct super_block *sb,
>                 struct ext4_buddy *e4b, ext4_grpblk_t start,
>                 ext4_grpblk_t max, ext4_grpblk_t minblocks)
>  __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;
> +       ext4_group_t group = e4b->bd_group;
> +       struct ext4_free_extent ex;
> +       bool noalloc = false;
> +       int ret = 0;
> +
> +       INIT_LIST_HEAD(&discard_data_list);
>
>         bitmap = e4b->bd_bitmap;
>         start = max(e4b->bd_info->bb_first_free, start);
>         count = 0;
>         free_count = 0;
>
> +       blk_start_plug(&plug);
>         while (start <= max) {
>                 start = mb_find_next_zero_bit(bitmap, max + 1, start);
>                 if (start > max)
>                         break;
> +               bak = start;
>                 next = mb_find_next_bit(bitmap, max + 1, start);
> -
>                 if ((next - start) >= minblocks) {
> -                       int ret = ext4_trim_extent(sb, start, next - start, e4b);
> +                       /* when only one segment, there is no need to alloc entry */
> +                       noalloc = (free_count == 0) && (next >= max);
>
> -                       if (ret && ret != -EOPNOTSUPP)
> +                       trace_ext4_trim_extent(sb, group, start, next - start);
> +                       ex.fe_start = start;
> +                       ex.fe_group = group;
> +                       ex.fe_len = next - start;
> +                       /*
> +                        * Mark blocks used, so no one can reuse them while
> +                        * being trimmed.
> +                        */
> +                       mb_mark_used(e4b, &ex);
> +                       ext4_unlock_group(sb, group);
> +                       ret = ext4_issue_discard(sb, group, start, next - start, &discard_bio);
> +                       if (!noalloc) {
> +                               entry = kmem_cache_alloc(ext4_free_data_cachep,
> +                                                       GFP_NOFS|__GFP_NOFAIL);
> +                               entry->efd_start_cluster = start;
> +                               entry->efd_count = next - start;
> +                               list_add_tail(&entry->efd_list, &discard_data_list);
> +                       }
> +                       ext4_lock_group(sb, group);
> +                       if (ret < 0)
>                                 break;
>                         count += next - start;
>                 }
> @@ -6959,6 +6950,22 @@ __releases(ext4_group_lock_ptr(sb, e4b->bd_group))
>                         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 && free_count)
> +               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.20.1
>
Jan Kara Jan. 8, 2024, 5:15 p.m. UTC | #3 
On Fri 01-09-23 17:28:20, 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.
> 
> Signed-off-by: Fengnan Chang <changfengnan@bytedance.com>

This seems to have fallen through the cracks... I'm sorry for that.

>  static int ext4_try_to_trim_range(struct super_block *sb,
>  		struct ext4_buddy *e4b, ext4_grpblk_t start,
>  		ext4_grpblk_t max, ext4_grpblk_t minblocks)
>  __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;
> +	ext4_group_t group = e4b->bd_group;
> +	struct ext4_free_extent ex;
> +	bool noalloc = false;
> +	int ret = 0;
> +
> +	INIT_LIST_HEAD(&discard_data_list);
>  
>  	bitmap = e4b->bd_bitmap;
>  	start = max(e4b->bd_info->bb_first_free, start);
>  	count = 0;
>  	free_count = 0;
>  
> +	blk_start_plug(&plug);
>  	while (start <= max) {
>  		start = mb_find_next_zero_bit(bitmap, max + 1, start);
>  		if (start > max)
>  			break;
> +		bak = start;
>  		next = mb_find_next_bit(bitmap, max + 1, start);
> -
>  		if ((next - start) >= minblocks) {
> -			int ret = ext4_trim_extent(sb, start, next - start, e4b);
> +			/* when only one segment, there is no need to alloc entry */
> +			noalloc = (free_count == 0) && (next >= max);

Is the single extent case really worth the complications to save one
allocation? I don't think it is but maybe I'm missing something. Otherwise
the patch looks good to me!

								Honza

>  
> -			if (ret && ret != -EOPNOTSUPP)
> +			trace_ext4_trim_extent(sb, group, start, next - start);
> +			ex.fe_start = start;
> +			ex.fe_group = group;
> +			ex.fe_len = next - start;
> +			/*
> +			 * Mark blocks used, so no one can reuse them while
> +			 * being trimmed.
> +			 */
> +			mb_mark_used(e4b, &ex);
> +			ext4_unlock_group(sb, group);
> +			ret = ext4_issue_discard(sb, group, start, next - start, &discard_bio);
> +			if (!noalloc) {
> +				entry = kmem_cache_alloc(ext4_free_data_cachep,
> +							GFP_NOFS|__GFP_NOFAIL);
> +				entry->efd_start_cluster = start;
> +				entry->efd_count = next - start;
> +				list_add_tail(&entry->efd_list, &discard_data_list);
> +			}
> +			ext4_lock_group(sb, group);
> +			if (ret < 0)
>  				break;
>  			count += next - start;
>  		}
> @@ -6959,6 +6950,22 @@ __releases(ext4_group_lock_ptr(sb, e4b->bd_group))
>  			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 && free_count)
> +		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.20.1
>
Fengnan Chang Jan. 9, 2024, 11:28 a.m. UTC | #4 
Jan Kara <jack@suse.cz> 于2024年1月9日周二 01:15写道：
>
> On Fri 01-09-23 17:28:20, 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.
> >
> > Signed-off-by: Fengnan Chang <changfengnan@bytedance.com>
>
> This seems to have fallen through the cracks... I'm sorry for that.
>
> >  static int ext4_try_to_trim_range(struct super_block *sb,
> >               struct ext4_buddy *e4b, ext4_grpblk_t start,
> >               ext4_grpblk_t max, ext4_grpblk_t minblocks)
> >  __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;
> > +     ext4_group_t group = e4b->bd_group;
> > +     struct ext4_free_extent ex;
> > +     bool noalloc = false;
> > +     int ret = 0;
> > +
> > +     INIT_LIST_HEAD(&discard_data_list);
> >
> >       bitmap = e4b->bd_bitmap;
> >       start = max(e4b->bd_info->bb_first_free, start);
> >       count = 0;
> >       free_count = 0;
> >
> > +     blk_start_plug(&plug);
> >       while (start <= max) {
> >               start = mb_find_next_zero_bit(bitmap, max + 1, start);
> >               if (start > max)
> >                       break;
> > +             bak = start;
> >               next = mb_find_next_bit(bitmap, max + 1, start);
> > -
> >               if ((next - start) >= minblocks) {
> > -                     int ret = ext4_trim_extent(sb, start, next - start, e4b);
> > +                     /* when only one segment, there is no need to alloc entry */
> > +                     noalloc = (free_count == 0) && (next >= max);
>
> Is the single extent case really worth the complications to save one
> allocation? I don't think it is but maybe I'm missing something. Otherwise
> the patch looks good to me!
yeah, it's necessary, if there is only one segment, alloc memory may cause
performance regression.
Refer to this https://lore.kernel.org/linux-ext4/CALWNXx-6y0=ZDBMicv2qng9pKHWcpJbCvUm9TaRBwg81WzWkWQ@mail.gmail.com/

Thanks.

>
>                                                                 Honza
>
> >
> > -                     if (ret && ret != -EOPNOTSUPP)
> > +                     trace_ext4_trim_extent(sb, group, start, next - start);
> > +                     ex.fe_start = start;
> > +                     ex.fe_group = group;
> > +                     ex.fe_len = next - start;
> > +                     /*
> > +                      * Mark blocks used, so no one can reuse them while
> > +                      * being trimmed.
> > +                      */
> > +                     mb_mark_used(e4b, &ex);
> > +                     ext4_unlock_group(sb, group);
> > +                     ret = ext4_issue_discard(sb, group, start, next - start, &discard_bio);
> > +                     if (!noalloc) {
> > +                             entry = kmem_cache_alloc(ext4_free_data_cachep,
> > +                                                     GFP_NOFS|__GFP_NOFAIL);
> > +                             entry->efd_start_cluster = start;
> > +                             entry->efd_count = next - start;
> > +                             list_add_tail(&entry->efd_list, &discard_data_list);
> > +                     }
> > +                     ext4_lock_group(sb, group);
> > +                     if (ret < 0)
> >                               break;
> >                       count += next - start;
> >               }
> > @@ -6959,6 +6950,22 @@ __releases(ext4_group_lock_ptr(sb, e4b->bd_group))
> >                       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 && free_count)
> > +             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.20.1
> >
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
Jan Kara Jan. 9, 2024, 12:08 p.m. UTC | #5 
On Tue 09-01-24 19:28:07, Fengnan Chang wrote:
> Jan Kara <jack@suse.cz> 于2024年1月9日周二 01:15写道：
> >
> > On Fri 01-09-23 17:28:20, 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.
> > >
> > > Signed-off-by: Fengnan Chang <changfengnan@bytedance.com>
> >
> > This seems to have fallen through the cracks... I'm sorry for that.
> >
> > >  static int ext4_try_to_trim_range(struct super_block *sb,
> > >               struct ext4_buddy *e4b, ext4_grpblk_t start,
> > >               ext4_grpblk_t max, ext4_grpblk_t minblocks)
> > >  __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;
> > > +     ext4_group_t group = e4b->bd_group;
> > > +     struct ext4_free_extent ex;
> > > +     bool noalloc = false;
> > > +     int ret = 0;
> > > +
> > > +     INIT_LIST_HEAD(&discard_data_list);
> > >
> > >       bitmap = e4b->bd_bitmap;
> > >       start = max(e4b->bd_info->bb_first_free, start);
> > >       count = 0;
> > >       free_count = 0;
> > >
> > > +     blk_start_plug(&plug);
> > >       while (start <= max) {
> > >               start = mb_find_next_zero_bit(bitmap, max + 1, start);
> > >               if (start > max)
> > >                       break;
> > > +             bak = start;
> > >               next = mb_find_next_bit(bitmap, max + 1, start);
> > > -
> > >               if ((next - start) >= minblocks) {
> > > -                     int ret = ext4_trim_extent(sb, start, next - start, e4b);
> > > +                     /* when only one segment, there is no need to alloc entry */
> > > +                     noalloc = (free_count == 0) && (next >= max);
> >
> > Is the single extent case really worth the complications to save one
> > allocation? I don't think it is but maybe I'm missing something. Otherwise
> > the patch looks good to me!
> yeah, it's necessary, if there is only one segment, alloc memory may cause
> performance regression.
> Refer to this https://lore.kernel.org/linux-ext4/CALWNXx-6y0=ZDBMicv2qng9pKHWcpJbCvUm9TaRBwg81WzWkWQ@mail.gmail.com/

Ah, thanks for the reference! Then what I'd suggest is something like:

	struct ext4_free_data first_entry;
	/*
	 * We preallocate the first entry on stack to optimize for the common
	 * case of trimming single extent in each group. It has measurable
	 * performance impact.
	 */
	struct ext4_free_data *entry = &first_entry;

then when we allocate we do:

		if (!entry)
			entry = kmem_cache_alloc(...)
		entry->efd_start_cluster = start;
		entry->efd_count = next - start;
		list_add_tail(&entry->efd_list, &discard_data_list);
		entry = NULL;

and then when freeing we can have:

	list_for_each_entry_safe(fd, nfd, &discard_data_list, efd_list) {
		mb_free_blocks(NULL, e4b, fd->efd_start_cluster, fd->efd_count);
		if (fd != &first_entry)
			kmem_cache_free(ext4_free_data_cachep, fd);
	}

Then it is more understandable what's going on...

								Honza
Fengnan Chang Jan. 10, 2024, 1:55 a.m. UTC | #6 
Jan Kara <jack@suse.cz> 于2024年1月9日周二 20:09写道：
>
> On Tue 09-01-24 19:28:07, Fengnan Chang wrote:
> > Jan Kara <jack@suse.cz> 于2024年1月9日周二 01:15写道：
> > >
> > > On Fri 01-09-23 17:28:20, 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.
> > > >
> > > > Signed-off-by: Fengnan Chang <changfengnan@bytedance.com>
> > >
> > > This seems to have fallen through the cracks... I'm sorry for that.
> > >
> > > >  static int ext4_try_to_trim_range(struct super_block *sb,
> > > >               struct ext4_buddy *e4b, ext4_grpblk_t start,
> > > >               ext4_grpblk_t max, ext4_grpblk_t minblocks)
> > > >  __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;
> > > > +     ext4_group_t group = e4b->bd_group;
> > > > +     struct ext4_free_extent ex;
> > > > +     bool noalloc = false;
> > > > +     int ret = 0;
> > > > +
> > > > +     INIT_LIST_HEAD(&discard_data_list);
> > > >
> > > >       bitmap = e4b->bd_bitmap;
> > > >       start = max(e4b->bd_info->bb_first_free, start);
> > > >       count = 0;
> > > >       free_count = 0;
> > > >
> > > > +     blk_start_plug(&plug);
> > > >       while (start <= max) {
> > > >               start = mb_find_next_zero_bit(bitmap, max + 1, start);
> > > >               if (start > max)
> > > >                       break;
> > > > +             bak = start;
> > > >               next = mb_find_next_bit(bitmap, max + 1, start);
> > > > -
> > > >               if ((next - start) >= minblocks) {
> > > > -                     int ret = ext4_trim_extent(sb, start, next - start, e4b);
> > > > +                     /* when only one segment, there is no need to alloc entry */
> > > > +                     noalloc = (free_count == 0) && (next >= max);
> > >
> > > Is the single extent case really worth the complications to save one
> > > allocation? I don't think it is but maybe I'm missing something. Otherwise
> > > the patch looks good to me!
> > yeah, it's necessary, if there is only one segment, alloc memory may cause
> > performance regression.
> > Refer to this https://lore.kernel.org/linux-ext4/CALWNXx-6y0=ZDBMicv2qng9pKHWcpJbCvUm9TaRBwg81WzWkWQ@mail.gmail.com/
>
> Ah, thanks for the reference! Then what I'd suggest is something like:
>
>         struct ext4_free_data first_entry;
>         /*
>          * We preallocate the first entry on stack to optimize for the common
>          * case of trimming single extent in each group. It has measurable
>          * performance impact.
>          */
>         struct ext4_free_data *entry = &first_entry;
>
> then when we allocate we do:
>
>                 if (!entry)
>                         entry = kmem_cache_alloc(...)
>                 entry->efd_start_cluster = start;
>                 entry->efd_count = next - start;
>                 list_add_tail(&entry->efd_list, &discard_data_list);
>                 entry = NULL;
>
> and then when freeing we can have:
>
>         list_for_each_entry_safe(fd, nfd, &discard_data_list, efd_list) {
>                 mb_free_blocks(NULL, e4b, fd->efd_start_cluster, fd->efd_count);
>                 if (fd != &first_entry)
>                         kmem_cache_free(ext4_free_data_cachep, fd);
>         }
>
> Then it is more understandable what's going on...
Looks better, I'll modify it in the next version.
Thanks.
>
>                                                                 Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
diff mbox series

Patch

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 1e4c667812a9..9fc69a92c496 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -6874,70 +6874,61 @@  int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
 	return err;
 }
 
-/**
- * ext4_trim_extent -- function to TRIM one single free extent in the group
- * @sb:		super block for the file system
- * @start:	starting block of the free extent in the alloc. group
- * @count:	number of blocks to TRIM
- * @e4b:	ext4 buddy for the group
- *
- * Trim "count" blocks starting at "start" in the "group". To assure that no
- * one will allocate those blocks, mark it as used in buddy bitmap. This must
- * be called with under the group lock.
- */
-static int ext4_trim_extent(struct super_block *sb,
-		int start, int count, struct ext4_buddy *e4b)
-__releases(bitlock)
-__acquires(bitlock)
-{
-	struct ext4_free_extent ex;
-	ext4_group_t group = e4b->bd_group;
-	int ret = 0;
-
-	trace_ext4_trim_extent(sb, group, start, count);
-
-	assert_spin_locked(ext4_group_lock_ptr(sb, group));
-
-	ex.fe_start = start;
-	ex.fe_group = group;
-	ex.fe_len = count;
-
-	/*
-	 * Mark blocks used, so no one can reuse them while
-	 * being trimmed.
-	 */
-	mb_mark_used(e4b, &ex);
-	ext4_unlock_group(sb, group);
-	ret = ext4_issue_discard(sb, group, start, count, NULL);
-	ext4_lock_group(sb, group);
-	mb_free_blocks(NULL, e4b, start, ex.fe_len);
-	return ret;
-}
-
 static int ext4_try_to_trim_range(struct super_block *sb,
 		struct ext4_buddy *e4b, ext4_grpblk_t start,
 		ext4_grpblk_t max, ext4_grpblk_t minblocks)
 __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;
+	ext4_group_t group = e4b->bd_group;
+	struct ext4_free_extent ex;
+	bool noalloc = false;
+	int ret = 0;
+
+	INIT_LIST_HEAD(&discard_data_list);
 
 	bitmap = e4b->bd_bitmap;
 	start = max(e4b->bd_info->bb_first_free, start);
 	count = 0;
 	free_count = 0;
 
+	blk_start_plug(&plug);
 	while (start <= max) {
 		start = mb_find_next_zero_bit(bitmap, max + 1, start);
 		if (start > max)
 			break;
+		bak = start;
 		next = mb_find_next_bit(bitmap, max + 1, start);
-
 		if ((next - start) >= minblocks) {
-			int ret = ext4_trim_extent(sb, start, next - start, e4b);
+			/* when only one segment, there is no need to alloc entry */
+			noalloc = (free_count == 0) && (next >= max);
 
-			if (ret && ret != -EOPNOTSUPP)
+			trace_ext4_trim_extent(sb, group, start, next - start);
+			ex.fe_start = start;
+			ex.fe_group = group;
+			ex.fe_len = next - start;
+			/*
+			 * Mark blocks used, so no one can reuse them while
+			 * being trimmed.
+			 */
+			mb_mark_used(e4b, &ex);
+			ext4_unlock_group(sb, group);
+			ret = ext4_issue_discard(sb, group, start, next - start, &discard_bio);
+			if (!noalloc) {
+				entry = kmem_cache_alloc(ext4_free_data_cachep,
+							GFP_NOFS|__GFP_NOFAIL);
+				entry->efd_start_cluster = start;
+				entry->efd_count = next - start;
+				list_add_tail(&entry->efd_list, &discard_data_list);
+			}
+			ext4_lock_group(sb, group);
+			if (ret < 0)
 				break;
 			count += next - start;
 		}
@@ -6959,6 +6950,22 @@  __releases(ext4_group_lock_ptr(sb, e4b->bd_group))
 			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 && free_count)
+		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;
 }