linux-kernel - Re: [PATCH] xfs: remove redundant batch variables for serialization

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] [day] [month] [year] [list]

Message-ID: <20230908235713.GP28202@frogsfrogsfrogs>
Date:   Fri, 8 Sep 2023 16:57:13 -0700
From:   "Darrick J. Wong" <djwong@...nel.org>
To:     alexjlzheng@...il.com
Cc:     chandan.babu@...cle.com, linux-xfs@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Jinliang Zheng <alexjlzheng@...cent.com>
Subject: Re: [PATCH] xfs: remove redundant batch variables for serialization

On Thu, Sep 07, 2023 at 08:30:18PM +0800, alexjlzheng@...il.com wrote:
> From: Jinliang Zheng <alexjlzheng@...cent.com>
> 
> Historically, when generic percpu counters were introduced in xfs for
> free block counters by commit 0d485ada404b ("xfs: use generic percpu
> counters for free block counter"), the counters use a custom batch size.
> In xfs_mod_freecounter(), originally named xfs_mod_fdblocks(), this
> patch attempts to serialize the program using a smaller batch size as a
> parameter to the addition function as the counter approaches 0.
> 
> Commit 8c1903d3081a ("xfs: inode and free block counters need to use
> __percpu_counter_compare") pointed out the error in commit 0d485ada404b
> ("xfs: use generic percpu counters for free block counter") mentioned
> above and said that "Because the counters use a custom batch size, the
> comparison functions need to be aware of that batch size otherwise the
> comparison does not work correctly".
> 
> After commit 8c1903d3081a ("xfs: inode and free block counters need to
> use __percpu_counter_compare"), the existence of the batch variable is
> no longer necessary, so it was removed to simplify the code.

It *was removed*?  It looks like this patch is _doing_ the removing.

I don't get it, what problem are you having?

--D

> Signed-off-by: Jinliang Zheng <alexjlzheng@...cent.com>
> ---
>  fs/xfs/xfs_mount.c | 17 +----------------
>  1 file changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 0a0fd19573d8..72dab39376b7 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -1144,7 +1144,6 @@ xfs_mod_freecounter(
>  	int64_t			lcounter;
>  	long long		res_used;
>  	uint64_t		set_aside = 0;
> -	s32			batch;
>  	bool			has_resv_pool;
>  
>  	ASSERT(counter == &mp->m_fdblocks || counter == &mp->m_frextents);
> @@ -1177,20 +1176,6 @@ xfs_mod_freecounter(
>  		return 0;
>  	}
>  
> -	/*
> -	 * Taking blocks away, need to be more accurate the closer we
> -	 * are to zero.
> -	 *
> -	 * If the counter has a value of less than 2 * max batch size,
> -	 * then make everything serialise as we are real close to
> -	 * ENOSPC.
> -	 */
> -	if (__percpu_counter_compare(counter, 2 * XFS_FDBLOCKS_BATCH,
> -				     XFS_FDBLOCKS_BATCH) < 0)
> -		batch = 1;
> -	else
> -		batch = XFS_FDBLOCKS_BATCH;
> -
>  	/*
>  	 * Set aside allocbt blocks because these blocks are tracked as free
>  	 * space but not available for allocation. Technically this means that a
> @@ -1204,7 +1189,7 @@ xfs_mod_freecounter(
>  	 */
>  	if (has_resv_pool)
>  		set_aside = xfs_fdblocks_unavailable(mp);
> -	percpu_counter_add_batch(counter, delta, batch);
> +	percpu_counter_add_batch(counter, delta, XFS_FDBLOCKS_BATCH);
>  	if (__percpu_counter_compare(counter, set_aside,
>  				     XFS_FDBLOCKS_BATCH) >= 0) {
>  		/* we had space! */
> -- 
> 2.31.1
>