Theodore Ts'o <tytso@xxxxxxx> 于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
