diff mbox series

[v2] xfs: remove redundant batch variables for serialization

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

Commit Message

Jinliang Zheng Sept. 9, 2023, 7:17 a.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:

v2: Modify ambiguous commit message.

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

Dave Chinner Sept. 10, 2023, 9:49 p.m. UTC | #1 
On Sat, Sep 09, 2023 at 03:17:51PM +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.

Hmmmm. Fiddling with percpu counter batch thresholds can expose
unexpected corner case behaviours.  What testing have you done on
this change?

Cheers,

Dave.
Jinliang Zheng Sept. 13, 2023, 3:41 a.m. UTC | #2 
On Mon, 11 Sep 2023 07:49:51 +1000, Dave Chinner <david@fromorbit.com> wrote:
> On Sat, Sep 09, 2023 at 03:17:51PM +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.
> 
> Hmmmm. Fiddling with percpu counter batch thresholds can expose
> unexpected corner case behaviours.  What testing have you done on
> this change?

Hi, Dave,

Thank you for your reply.

I have tested the patch using _filebench_ and _fio_.

In fact, corner cases often occur when the addition function and comparison
function use different batch thresholds. This is why the batch parameter of
the addition function must be chosen carefully when we use a additon function
with a variable parameter *percpu_counter_batch*.

The percpu_counter_compare() with variable parameter *percpu_counter_batch*
was replaced with __percpu_counter_compare() with fixed parameter
*XFS_FDBLOCKS_BATCH* by commit 8c1903d3081a ("xfs: inode and free block
counters need to use __percpu_counter_compare").

Then there is no need to detect the batch threshold. Just use the fixed
*XFS_FDBLOCKS_BATCH* to simplify the code.

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Best regards，

Jinliang Zheng.
Jinliang Zheng Sept. 13, 2023, 3:58 a.m. UTC | #3 
On Wed, 13 Sep 2023 11:41:15 +0800, Jinliang Zheng <alexjlzheng@gmail.com> wrote:
> On Mon, 11 Sep 2023 07:49:51 +1000, Dave Chinner <david@fromorbit.com> wrote:
> > On Sat, Sep 09, 2023 at 03:17:51PM +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.
> > 
> > Hmmmm. Fiddling with percpu counter batch thresholds can expose
> > unexpected corner case behaviours.  What testing have you done on
> > this change?
> 
> Hi, Dave,
> 
> Thank you for your reply.
> 
> I have tested the patch using _filebench_ and _fio_.
> 
> In fact, corner cases often occur when the addition function and comparison
> function use different batch thresholds. This is why the batch parameter of
> the addition function must be chosen carefully when we use a additon function
> with a variable parameter *percpu_counter_batch*.

There is a typo here, it should be:

This is why the batch parameter of the addition function must be chosen
carefully when we use a compare function with a variable parameter
*percpu_counter_batch*.

Sorry, please forgive my carelessness.

> 
> The percpu_counter_compare() with variable parameter *percpu_counter_batch*
> was replaced with __percpu_counter_compare() with fixed parameter
> *XFS_FDBLOCKS_BATCH* by commit 8c1903d3081a ("xfs: inode and free block
> counters need to use __percpu_counter_compare").
> 
> Then there is no need to detect the batch threshold. Just use the fixed
> *XFS_FDBLOCKS_BATCH* to simplify the code.
> 
> > 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> 
> Best regards，
> 
> Jinliang Zheng.

Best regards,

Jinliang Zheng.
Dave Chinner Sept. 13, 2023, 11:44 p.m. UTC | #4 
On Wed, Sep 13, 2023 at 11:41:15AM +0800, alexjlzheng@gmail.com wrote:
> On Mon, 11 Sep 2023 07:49:51 +1000, Dave Chinner <david@fromorbit.com> wrote:
> > On Sat, Sep 09, 2023 at 03:17:51PM +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.
> > 
> > Hmmmm. Fiddling with percpu counter batch thresholds can expose
> > unexpected corner case behaviours.  What testing have you done on
> > this change?
> 
> Hi, Dave,
> 
> Thank you for your reply.
> 
> I have tested the patch using _filebench_ and _fio_.

What about all the ENOSPC and shutdown tests in fstests?

If you haven't exercised ENOSPC conditions in your testing, then you
haven't actually tested whether the new code can accurately and
correctly detect ENOSPC conditions....

Cheers,

Dave.
Jinliang Zheng Sept. 18, 2023, 4:33 a.m. UTC | #5 
On Thu, 14 Sep 2023 09:44:42 +1000, Dave Chinner <david@fromorbit.com> wrote:
> On Wed, Sep 13, 2023 at 11:41:15AM +0800, alexjlzheng@gmail.com wrote:
> > On Mon, 11 Sep 2023 07:49:51 +1000, Dave Chinner <david@fromorbit.com> wrote:
> > > On Sat, Sep 09, 2023 at 03:17:51PM +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.
> > > 
> > > Hmmmm. Fiddling with percpu counter batch thresholds can expose
> > > unexpected corner case behaviours.  What testing have you done on
> > > this change?
> > 
> > Hi, Dave,
> > 
> > Thank you for your reply.
> > 
> > I have tested the patch using _filebench_ and _fio_.
> 
> What about all the ENOSPC and shutdown tests in fstests?

Sorry for the late reply.

I tested this patch using xfstests last week, and all enospc and shutdown
related tests passed.

> 
> If you haven't exercised ENOSPC conditions in your testing, then you
> haven't actually tested whether the new code can accurately and
> correctly detect ENOSPC conditions....
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Best regards,

Jinliang Zheng.
diff mbox series

Patch

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! */