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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 18 Aug 2023 15:14:34 +0800
From:   Fengnan Chang <changfengnan@...edance.com>
To:     "Theodore Ts'o" <tytso@....edu>
Cc:     adilger.kernel@...ger.ca, guoqing.jiang@...ux.dev,
        linux-ext4@...r.kernel.org,
        kernel test robot <oliver.sang@...el.com>
Subject: Re: [External] Re: [PATCH v3] ext4: improve trim efficiency

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