[PATCH v3] ext4: improve trim efficiency

[PATCH v3] ext4: improve trim efficiency

[Fengnan Chang]

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 <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202307171455.ee68ef8b-oliver.sang@intel.com
Signed-off-by: Fengnan Chang <changfengnan@bytedance.com>
---
 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)

Re: [PATCH v3] ext4: improve trim efficiency

[Guoqing Jiang]

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 <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202307171455.ee68ef8b-oliver.sang@intel.com
> Signed-off-by: Fengnan Chang <changfengnan@bytedance.com>
> ---
>   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

Re: [External] Re: [PATCH v3] ext4: improve trim efficiency

[Fengnan Chang]

Hi Ted, Andreas:
    Any comments ?

Thanks.

Guoqing Jiang <guoqing.jiang@linux.dev> 于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 <oliver.sang@intel.com>
> > Closes: https://lore.kernel.org/oe-lkp/202307171455.ee68ef8b-oliver.sang@intel.com
> > Signed-off-by: Fengnan Chang <changfengnan@bytedance.com>
> > ---
> >   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

Re: [External] Re: [PATCH v3] ext4: improve trim efficiency

[Fengnan Chang]

ping

Fengnan Chang <changfengnan@bytedance.com> 于2023年7月31日周一 20:52写道：
>
> Hi Ted, Andreas:
>     Any comments ?
>
> Thanks.
>
> Guoqing Jiang <guoqing.jiang@linux.dev> 于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 <oliver.sang@intel.com>
> > > Closes: https://lore.kernel.org/oe-lkp/202307171455.ee68ef8b-oliver.sang@intel.com
> > > Signed-off-by: Fengnan Chang <changfengnan@bytedance.com>
> > > ---
> > >   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

Re: [PATCH v3] ext4: improve trim efficiency

[Theodore Ts'o]

On Tue, Jul 25, 2023 at 08:18:48PM +0800, 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.

Thanks for the patch.  A few things that I'd like to see changed.

> 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)

The function ext4_trim_extent() is used in one place, by
ext4_try_to_trim_range().  So instead of adding the new parameters
noalloc and extryp...

> @@ -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;
> +	}
> +

... I think it might be better to move the allocation and
initialization the ext4_free_data structure to ext4_trim_extent()'s
caller.

In the current patch, we are adding the entry to the linked list, and
we actually *use* the linked list in ext4_try_to_trim_range().  By
move the code which allocates the entry to the same place, we
eliminate some extra variables added to the ext4_trim_extent()
function, and it makes the code easier to read.

In fact, given that ext4_trim_extent() is used only once by its
caller, we could just inline the code (which isn't actually all that
much) into ext4_Try_to_trim_range().  That would eliminate the need
for the __acquires(bitlock) and __release(bitlock) sparse annotations,
as well as the "assert_spin_locked()".

That also keeps the mb_mark_used() and mb_free_blocks() calls in the
same function, which again improves code readability.

Thanks,

						- Ted

Re: [External] Re: [PATCH v3] ext4: improve trim efficiency

[Fengnan Chang]

Theodore Ts'o <tytso@mit.edu> 于2023年8月18日周五 11:43写道：
>
> On Tue, Jul 25, 2023 at 08:18:48PM +0800, 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.
>
> Thanks for the patch.  A few things that I'd like to see changed.
>
> > 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)
>
> The function ext4_trim_extent() is used in one place, by
> ext4_try_to_trim_range().  So instead of adding the new parameters
> noalloc and extryp...
>
> > @@ -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;
> > +     }
> > +
>
> ... I think it might be better to move the allocation and
> initialization the ext4_free_data structure to ext4_trim_extent()'s
> caller.
If we move the allocation and initialization the ext4_free_data
structure to ext4_try_to_trim_range, we need move
ext4_lock_group too, because we can't do alloc memory when
hold lock in ioctl context.
How about just remove ext4_trim_extent, and do all work in
ext4_try_to_trim_range?  it will be easier to read.

>
> In the current patch, we are adding the entry to the linked list, and
> we actually *use* the linked list in ext4_try_to_trim_range().  By
> move the code which allocates the entry to the same place, we
> eliminate some extra variables added to the ext4_trim_extent()
> function, and it makes the code easier to read.
>
> In fact, given that ext4_trim_extent() is used only once by its
> caller, we could just inline the code (which isn't actually all that
> much) into ext4_Try_to_trim_range().  That would eliminate the need
> for the __acquires(bitlock) and __release(bitlock) sparse annotations,
> as well as the "assert_spin_locked()".
>
> That also keeps the mb_mark_used() and mb_free_blocks() calls in the
> same function, which again improves code readability.
>
> Thanks,
>
>                                                 - Ted

Re: [External] Re: [PATCH v3] ext4: improve trim efficiency

[Theodore Ts'o]

On Fri, Aug 18, 2023 at 03:14:34PM +0800, Fengnan Chang wrote:
> If we move the allocation and initialization the ext4_free_data
> structure to ext4_try_to_trim_range, we need move
> ext4_lock_group too, because we can't do alloc memory when
> hold lock in ioctl context.
> How about just remove ext4_trim_extent, and do all work in
> ext4_try_to_trim_range?  it will be easier to read.

Yes, agreed.

					- Ted