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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Reiser Filesystem Development]     [Ceph FS]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite National Park]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Media]

  Powered by Linux