linux-ext4 - Re: [PATCH v2 07/16] ext4: convert sbi->s_mb_free_pending to atomic

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: <kn22zrgititzedfjutzudq6i3qn26ukmg26g7nzmgwahkrn3r7@bhpccisxsvz2>
Date: Fri, 27 Jun 2025 20:33:23 +0200
From: Jan Kara <jack@...e.cz>
To: Baokun Li <libaokun1@...wei.com>
Cc: linux-ext4@...r.kernel.org, tytso@....edu, jack@...e.cz, 
	adilger.kernel@...ger.ca, ojaswin@...ux.ibm.com, linux-kernel@...r.kernel.org, 
	yi.zhang@...wei.com, yangerkun@...wei.com
Subject: Re: [PATCH v2 07/16] ext4: convert sbi->s_mb_free_pending to atomic_t

On Mon 23-06-25 15:32:55, Baokun Li wrote:
> Previously, s_md_lock was used to protect s_mb_free_pending during
> modifications, while smp_mb() ensured fresh reads, so s_md_lock just
> guarantees the atomicity of s_mb_free_pending. Thus we optimized it by
> converting s_mb_free_pending into an atomic variable, thereby eliminating
> s_md_lock and minimizing lock contention. This also prepares for future
> lockless merging of free extents.
> 
> Following this modification, s_md_lock is exclusively responsible for
> managing insertions and deletions within s_freed_data_list, along with
> operations involving list_splice.
> 
> Performance test data follows:
> 
> Test: Running will-it-scale/fallocate2 on CPU-bound containers.
> Observation: Average fallocate operations per container per second.
> 
>                    | Kunpeng 920 / 512GB -P80|  AMD 9654 / 1536GB -P96 |
>  Disk: 960GB SSD   |-------------------------|-------------------------|
>                    | base  |    patched      | base  |    patched      |
> -------------------|-------|-----------------|-------|-----------------|
> mb_optimize_scan=0 | 19699 | 20982 (+6.5%)   | 53093 | 50629 (-4.6%)   |
> mb_optimize_scan=1 | 9862  | 10703 (+8.5%)   | 14401 | 14856 (+3.1%)   |
> 
> Signed-off-by: Baokun Li <libaokun1@...wei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@...e.cz>

								Honza

> ---
>  fs/ext4/balloc.c  | 2 +-
>  fs/ext4/ext4.h    | 2 +-
>  fs/ext4/mballoc.c | 9 +++------
>  3 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
> index c48fd36b2d74..c9329ed5c094 100644
> --- a/fs/ext4/balloc.c
> +++ b/fs/ext4/balloc.c
> @@ -703,7 +703,7 @@ int ext4_should_retry_alloc(struct super_block *sb, int *retries)
>  	 * possible we just missed a transaction commit that did so
>  	 */
>  	smp_mb();
> -	if (sbi->s_mb_free_pending == 0) {
> +	if (atomic_read(&sbi->s_mb_free_pending) == 0) {
>  		if (test_opt(sb, DISCARD)) {
>  			atomic_inc(&sbi->s_retry_alloc_pending);
>  			flush_work(&sbi->s_discard_work);
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 294198c05cdd..003b8d3726e8 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1602,7 +1602,7 @@ struct ext4_sb_info {
>  	unsigned short *s_mb_offsets;
>  	unsigned int *s_mb_maxs;
>  	unsigned int s_group_info_size;
> -	unsigned int s_mb_free_pending;
> +	atomic_t s_mb_free_pending;
>  	struct list_head s_freed_data_list[2];	/* List of blocks to be freed
>  						   after commit completed */
>  	struct list_head s_discard_list;
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 216b332a5054..5410fb3688ee 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -3680,7 +3680,7 @@ int ext4_mb_init(struct super_block *sb)
>  	}
>  
>  	spin_lock_init(&sbi->s_md_lock);
> -	sbi->s_mb_free_pending = 0;
> +	atomic_set(&sbi->s_mb_free_pending, 0);
>  	INIT_LIST_HEAD(&sbi->s_freed_data_list[0]);
>  	INIT_LIST_HEAD(&sbi->s_freed_data_list[1]);
>  	INIT_LIST_HEAD(&sbi->s_discard_list);
> @@ -3894,10 +3894,7 @@ static void ext4_free_data_in_buddy(struct super_block *sb,
>  	/* we expect to find existing buddy because it's pinned */
>  	BUG_ON(err != 0);
>  
> -	spin_lock(&EXT4_SB(sb)->s_md_lock);
> -	EXT4_SB(sb)->s_mb_free_pending -= entry->efd_count;
> -	spin_unlock(&EXT4_SB(sb)->s_md_lock);
> -
> +	atomic_sub(entry->efd_count, &EXT4_SB(sb)->s_mb_free_pending);
>  	db = e4b.bd_info;
>  	/* there are blocks to put in buddy to make them really free */
>  	count += entry->efd_count;
> @@ -6392,7 +6389,7 @@ ext4_mb_free_metadata(handle_t *handle, struct ext4_buddy *e4b,
>  
>  	spin_lock(&sbi->s_md_lock);
>  	list_add_tail(&new_entry->efd_list, &sbi->s_freed_data_list[new_entry->efd_tid & 1]);
> -	sbi->s_mb_free_pending += clusters;
> +	atomic_add(clusters, &sbi->s_mb_free_pending);
>  	spin_unlock(&sbi->s_md_lock);
>  }
>  
> -- 
> 2.46.1
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR