diff mbox series

[v3,RESEND] xfs: remove redundant batch variables for serialization

Message ID 20240417120734.853960-1-alexjlzheng@tencent.com (mailing list archive)
State Accepted, archived
Headers show
Series [v3,RESEND] xfs: remove redundant batch variables for serialization | expand

Commit Message

Jinliang Zheng April 17, 2024, 12:07 p.m. UTC 
From: Jinliang Zheng <alexjlzheng@tencent.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 used a custom batch
size. In xfs_mod_freecounter(), originally named xfs_mod_fdblocks(),
this patch attempted to serialize the program using a smaller batch size
as 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". Then percpu_counter_compare() was
replaced with __percpu_counter_compare() with parameter
XFS_FDBLOCKS_BATCH.

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 this patch is proposed to simplify the code by
removing it.

Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
---
Changelog:

v3: Resend for the second time 

v2: https://lore.kernel.org/linux-xfs/20230918043344.890817-1-alexjlzheng@tencent.com/

v1: https://lore.kernel.org/linux-xfs/20230908235713.GP28202@frogsfrogsfrogs/T/#t
---
 fs/xfs/xfs_mount.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

Comments

Darrick J. Wong April 17, 2024, 3:27 p.m. UTC | #1 
On Wed, Apr 17, 2024 at 08:07:35PM +0800, alexjlzheng@gmail.com wrote:
> From: Jinliang Zheng <alexjlzheng@tencent.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 used a custom batch
> size. In xfs_mod_freecounter(), originally named xfs_mod_fdblocks(),
> this patch attempted to serialize the program using a smaller batch size
> as 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". Then percpu_counter_compare() was
> replaced with __percpu_counter_compare() with parameter
> XFS_FDBLOCKS_BATCH.
> 
> 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 this patch is proposed to simplify the code by
> removing it.
> 
> Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
> ---
> Changelog:
> 
> v3: Resend for the second time 
> 
> v2: https://lore.kernel.org/linux-xfs/20230918043344.890817-1-alexjlzheng@tencent.com/
> 
> v1: https://lore.kernel.org/linux-xfs/20230908235713.GP28202@frogsfrogsfrogs/T/#t

...you still haven't answered my question from V1: What problem are you
solving with this patch?

--D

> ---
>  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 aed5be5508fe..8e47a3040893 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.39.3
> 
>
Jinliang Zheng April 17, 2024, 3:54 p.m. UTC | #2 
On Wed, 17 Apr 2024 08:27:13 -0700, djwong@kernel.org wrote:
> On Wed, Apr 17, 2024 at 08:07:35PM +0800, alexjlzheng@gmail.com wrote:
> > From: Jinliang Zheng <alexjlzheng@tencent.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 used a custom batch
> > size. In xfs_mod_freecounter(), originally named xfs_mod_fdblocks(),
> > this patch attempted to serialize the program using a smaller batch size
> > as 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". Then percpu_counter_compare() was
> > replaced with __percpu_counter_compare() with parameter
> > XFS_FDBLOCKS_BATCH.
> > 
> > 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 this patch is proposed to simplify the code by
> > removing it.
> > 
> > Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
> > ---
> > Changelog:
> > 
> > v3: Resend for the second time 
> > 
> > v2: https://lore.kernel.org/linux-xfs/20230918043344.890817-1-alexjlzheng@tencent.com/
> > 
> > v1: https://lore.kernel.org/linux-xfs/20230908235713.GP28202@frogsfrogsfrogs/T/#t
> 
> ...you still haven't answered my question from V1: What problem are you
> solving with this patch?

Hi, thank you for your reply. :)

I'm trying to simplify the code. When percpu_counter_add_batch() and
__percpu_counter_compare() use the same batch size, percpu_counter can count
correctly, so there is no need to reduce the batch size to 1, which will cause
unnecessary serialization.

Best regards,
Jinliang Zheng

> 
> --D
Dave Chinner April 17, 2024, 10:30 p.m. UTC | #3 
On Wed, Apr 17, 2024 at 11:54:38PM +0800, Jinliang Zheng wrote:
> On Wed, 17 Apr 2024 08:27:13 -0700, djwong@kernel.org wrote:
> > On Wed, Apr 17, 2024 at 08:07:35PM +0800, alexjlzheng@gmail.com wrote:
> > > From: Jinliang Zheng <alexjlzheng@tencent.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 used a custom batch
> > > size. In xfs_mod_freecounter(), originally named xfs_mod_fdblocks(),
> > > this patch attempted to serialize the program using a smaller batch size
> > > as 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". Then percpu_counter_compare() was
> > > replaced with __percpu_counter_compare() with parameter
> > > XFS_FDBLOCKS_BATCH.
> > > 
> > > 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 this patch is proposed to simplify the code by
> > > removing it.
> > > 
> > > Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
> > > ---
> > > Changelog:
> > > 
> > > v3: Resend for the second time 
> > > 
> > > v2: https://lore.kernel.org/linux-xfs/20230918043344.890817-1-alexjlzheng@tencent.com/
> > > 
> > > v1: https://lore.kernel.org/linux-xfs/20230908235713.GP28202@frogsfrogsfrogs/T/#t
> > 
> > ...you still haven't answered my question from V1: What problem are you
> > solving with this patch?
> 
> Hi, thank you for your reply. :)
> 
> I'm trying to simplify the code. When percpu_counter_add_batch() and
> __percpu_counter_compare() use the same batch size, percpu_counter can count
> correctly, so there is no need to reduce the batch size to 1, which will cause
> unnecessary serialization.

One of the reasons the batch size gets set to 1 when we are really
near to enospc is so that anyone else reading the counter will see
the update immediately via percpu_counter_read*(). i.e. this forces
the global count to be updated immediately rather than use the
percpu accumulators.

This behaviour helps external, unsynchronised visibility be more
accurate (e.g. in speculative preallocation) as we get very close to
ENOSPC. We don't care if we serialise near ENOSPC - we're going to
be going through allocation slow paths anyway, so there's no benefit
to be gained by maintaining wide concurrency when we are this close
to ENOSPC.

There is, however, benefit to near-ENOSPC allocation behaviour by
being more accurate, hence the batch size change when we near the
zero threshold.

-Dave.
diff mbox series

Patch

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index aed5be5508fe..8e47a3040893 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! */