linux-ext4 - 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20230818034330.GE3464136@mit.edu>
Date:   Thu, 17 Aug 2023 23:43:30 -0400
From:   "Theodore Ts'o" <tytso@....edu>
To:     Fengnan Chang <changfengnan@...edance.com>
Cc:     adilger.kernel@...ger.ca, guoqing.jiang@...ux.dev,
        linux-ext4@...r.kernel.org,
        kernel test robot <oliver.sang@...el.com>
Subject: Re: [PATCH v3] ext4: improve trim efficiency

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