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
