[PATCH 0/2] xfs: fix unecessary percpu counter overhead

[PATCH 0/2] xfs: fix unecessary percpu counter overhead

[Dave Chinner]

Hi folks,

This is a resend of a patch from months ago that can be found here:

https://lore.kernel.org/linux-xfs/20191121004437.9633-1-david@fromorbit.com/

I've split it into two patches and cleaned it up further and
retested it, and all is good now.

Essentially it solves the problem of production systems taking
percpu_counter_sum() overhead in a hot path when the sum is only
used for debug purposes and not actually compiled in to production
kernels. As a further cleanup of this code, the error handling
never returns errors at all to the caller, so it's only
for debug purposes. Given that the error handling logic is wrong and
we throw it away on debug kernels anyway, just get rid of all of it.

Cheers,

Dave.

[PATCH 1/2] xfs: gut error handling in xfs_trans_unreserve_and_mod_sb()

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The error handling in xfs_trans_unreserve_and_mod_sb() is largely
incorrect - rolling back the changes in the transaction if only one
counter underruns makes all the other counters incorrect. We still
allow the change to proceed and committing the transaction, except
now we have multiple incorrect counters instead of a single
underflow.

Further, we don't actually report the error to the caller, so this
is completely silent except on debug kernels that will assert on
failure before we even get to the rollback code.  Hence this error
handling is broken, untested, and largely unnecessary complexity.

Just remove it.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_trans.c | 170 +++++++--------------------------------------
 1 file changed, 27 insertions(+), 143 deletions(-)

diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 28b983ff8b113..4522ceaaf57ba 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -534,57 +534,9 @@ xfs_trans_apply_sb_deltas(
 				  sizeof(sbp->sb_frextents) - 1);
 }
 
-STATIC int
-xfs_sb_mod8(
-	uint8_t			*field,
-	int8_t			delta)
-{
-	int8_t			counter = *field;
-
-	counter += delta;
-	if (counter < 0) {
-		ASSERT(0);
-		return -EINVAL;
-	}
-	*field = counter;
-	return 0;
-}
-
-STATIC int
-xfs_sb_mod32(
-	uint32_t		*field,
-	int32_t			delta)
-{
-	int32_t			counter = *field;
-
-	counter += delta;
-	if (counter < 0) {
-		ASSERT(0);
-		return -EINVAL;
-	}
-	*field = counter;
-	return 0;
-}
-
-STATIC int
-xfs_sb_mod64(
-	uint64_t		*field,
-	int64_t			delta)
-{
-	int64_t			counter = *field;
-
-	counter += delta;
-	if (counter < 0) {
-		ASSERT(0);
-		return -EINVAL;
-	}
-	*field = counter;
-	return 0;
-}
-
 /*
- * xfs_trans_unreserve_and_mod_sb() is called to release unused reservations
- * and apply superblock counter changes to the in-core superblock.  The
+ * xfs_trans_unreserve_and_mod_sb() is called to release unused reservations and
+ * apply superblock counter changes to the in-core superblock.  The
  * t_res_fdblocks_delta and t_res_frextents_delta fields are explicitly NOT
  * applied to the in-core superblock.  The idea is that that has already been
  * done.
@@ -629,20 +581,17 @@ xfs_trans_unreserve_and_mod_sb(
 	/* apply the per-cpu counters */
 	if (blkdelta) {
 		error = xfs_mod_fdblocks(mp, blkdelta, rsvd);
-		if (error)
-			goto out;
+		ASSERT(!error);
 	}
 
 	if (idelta) {
 		error = xfs_mod_icount(mp, idelta);
-		if (error)
-			goto out_undo_fdblocks;
+		ASSERT(!error);
 	}
 
 	if (ifreedelta) {
 		error = xfs_mod_ifree(mp, ifreedelta);
-		if (error)
-			goto out_undo_icount;
+		ASSERT(!error);
 	}
 
 	if (rtxdelta == 0 && !(tp->t_flags & XFS_TRANS_SB_DIRTY))
@@ -650,95 +599,30 @@ xfs_trans_unreserve_and_mod_sb(
 
 	/* apply remaining deltas */
 	spin_lock(&mp->m_sb_lock);
-	if (rtxdelta) {
-		error = xfs_sb_mod64(&mp->m_sb.sb_frextents, rtxdelta);
-		if (error)
-			goto out_undo_ifree;
-	}
-
-	if (tp->t_dblocks_delta != 0) {
-		error = xfs_sb_mod64(&mp->m_sb.sb_dblocks, tp->t_dblocks_delta);
-		if (error)
-			goto out_undo_frextents;
-	}
-	if (tp->t_agcount_delta != 0) {
-		error = xfs_sb_mod32(&mp->m_sb.sb_agcount, tp->t_agcount_delta);
-		if (error)
-			goto out_undo_dblocks;
-	}
-	if (tp->t_imaxpct_delta != 0) {
-		error = xfs_sb_mod8(&mp->m_sb.sb_imax_pct, tp->t_imaxpct_delta);
-		if (error)
-			goto out_undo_agcount;
-	}
-	if (tp->t_rextsize_delta != 0) {
-		error = xfs_sb_mod32(&mp->m_sb.sb_rextsize,
-				     tp->t_rextsize_delta);
-		if (error)
-			goto out_undo_imaxpct;
-	}
-	if (tp->t_rbmblocks_delta != 0) {
-		error = xfs_sb_mod32(&mp->m_sb.sb_rbmblocks,
-				     tp->t_rbmblocks_delta);
-		if (error)
-			goto out_undo_rextsize;
-	}
-	if (tp->t_rblocks_delta != 0) {
-		error = xfs_sb_mod64(&mp->m_sb.sb_rblocks, tp->t_rblocks_delta);
-		if (error)
-			goto out_undo_rbmblocks;
-	}
-	if (tp->t_rextents_delta != 0) {
-		error = xfs_sb_mod64(&mp->m_sb.sb_rextents,
-				     tp->t_rextents_delta);
-		if (error)
-			goto out_undo_rblocks;
-	}
-	if (tp->t_rextslog_delta != 0) {
-		error = xfs_sb_mod8(&mp->m_sb.sb_rextslog,
-				     tp->t_rextslog_delta);
-		if (error)
-			goto out_undo_rextents;
-	}
+	mp->m_sb.sb_frextents += rtxdelta;
+	mp->m_sb.sb_dblocks += tp->t_dblocks_delta;
+	mp->m_sb.sb_agcount += tp->t_agcount_delta;
+	mp->m_sb.sb_imax_pct += tp->t_imaxpct_delta;
+	mp->m_sb.sb_rextsize += tp->t_rextsize_delta;
+	mp->m_sb.sb_rbmblocks += tp->t_rbmblocks_delta;
+	mp->m_sb.sb_rblocks += tp->t_rblocks_delta;
+	mp->m_sb.sb_rextents += tp->t_rextents_delta;
+	mp->m_sb.sb_rextslog += tp->t_rextslog_delta;
 	spin_unlock(&mp->m_sb_lock);
-	return;
 
-out_undo_rextents:
-	if (tp->t_rextents_delta)
-		xfs_sb_mod64(&mp->m_sb.sb_rextents, -tp->t_rextents_delta);
-out_undo_rblocks:
-	if (tp->t_rblocks_delta)
-		xfs_sb_mod64(&mp->m_sb.sb_rblocks, -tp->t_rblocks_delta);
-out_undo_rbmblocks:
-	if (tp->t_rbmblocks_delta)
-		xfs_sb_mod32(&mp->m_sb.sb_rbmblocks, -tp->t_rbmblocks_delta);
-out_undo_rextsize:
-	if (tp->t_rextsize_delta)
-		xfs_sb_mod32(&mp->m_sb.sb_rextsize, -tp->t_rextsize_delta);
-out_undo_imaxpct:
-	if (tp->t_rextsize_delta)
-		xfs_sb_mod8(&mp->m_sb.sb_imax_pct, -tp->t_imaxpct_delta);
-out_undo_agcount:
-	if (tp->t_agcount_delta)
-		xfs_sb_mod32(&mp->m_sb.sb_agcount, -tp->t_agcount_delta);
-out_undo_dblocks:
-	if (tp->t_dblocks_delta)
-		xfs_sb_mod64(&mp->m_sb.sb_dblocks, -tp->t_dblocks_delta);
-out_undo_frextents:
-	if (rtxdelta)
-		xfs_sb_mod64(&mp->m_sb.sb_frextents, -rtxdelta);
-out_undo_ifree:
-	spin_unlock(&mp->m_sb_lock);
-	if (ifreedelta)
-		xfs_mod_ifree(mp, -ifreedelta);
-out_undo_icount:
-	if (idelta)
-		xfs_mod_icount(mp, -idelta);
-out_undo_fdblocks:
-	if (blkdelta)
-		xfs_mod_fdblocks(mp, -blkdelta, rsvd);
-out:
-	ASSERT(error == 0);
+	/*
+	 * Debug checks outside of the spinlock so they don't lock up the
+	 * machine if they fail.
+	 */
+	ASSERT(&mp->m_sb.sb_frextents >= 0);
+	ASSERT(&mp->m_sb.sb_dblocks >= 0);
+	ASSERT(&mp->m_sb.sb_agcount >= 0);
+	ASSERT(&mp->m_sb.sb_imax_pct >= 0);
+	ASSERT(&mp->m_sb.sb_rextsize >= 0);
+	ASSERT(&mp->m_sb.sb_rbmblocks >= 0);
+	ASSERT(&mp->m_sb.sb_rblocks >= 0);
+	ASSERT(&mp->m_sb.sb_rextents >= 0);
+	ASSERT(&mp->m_sb.sb_rextslog >= 0);
 	return;
 }
 
-- 
2.26.2.761.g0e0b3e54be

[PATCH 2/2] xfs: reduce free inode accounting overhead

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Shaokun Zhang reported that XFs was using substantial CPU time in
percpu_count_sum() when running a single threaded benchmark on
a high CPU count (128p) machine from xfs_mod_ifree(). The issue
is that the filesystem is empty when the benchmark runs, so inode
allocation is running with a very low inode free count.

With the percpu counter batching, this means comparisons when the
counter is less that 128 * 256 = 32768 use the slow path of adding
up all the counters across the CPUs, and this is expensive on high
CPU count machines.

The summing in xfs_mod_ifree() is only used to fire an assert if an
underrun occurs. The error is ignored by the higher level code.
Hence this is really just debug code and we don't need to run it
on production kernels, nor do we need such debug checks to return
error values just to trigger an assert.

Finally, xfs_mod_icount/xfs_mod_ifree are only called from
xfs_trans_unreserve_and_mod_sb(), so get rid of them and just
directly call the percpu_counter_add/percpu_counter_compare
functions. The compare functions are now run only on debug builds as
they are internal to ASSERT() checks and so only compiled in when
ASSERTs are active (CONFIG_XFS_DEBUG=y or CONFIG_XFS_WARN=y).

Reported-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_mount.c | 33 ---------------------------------
 fs/xfs/xfs_mount.h |  2 --
 fs/xfs/xfs_trans.c | 17 +++++++++++++----
 3 files changed, 13 insertions(+), 39 deletions(-)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index bb91f04266b9a..d5dcf98698600 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1189,39 +1189,6 @@ xfs_log_sbcount(xfs_mount_t *mp)
 	return xfs_sync_sb(mp, true);
 }
 
-/*
- * Deltas for the inode count are +/-64, hence we use a large batch size
- * of 128 so we don't need to take the counter lock on every update.
- */
-#define XFS_ICOUNT_BATCH	128
-int
-xfs_mod_icount(
-	struct xfs_mount	*mp,
-	int64_t			delta)
-{
-	percpu_counter_add_batch(&mp->m_icount, delta, XFS_ICOUNT_BATCH);
-	if (__percpu_counter_compare(&mp->m_icount, 0, XFS_ICOUNT_BATCH) < 0) {
-		ASSERT(0);
-		percpu_counter_add(&mp->m_icount, -delta);
-		return -EINVAL;
-	}
-	return 0;
-}
-
-int
-xfs_mod_ifree(
-	struct xfs_mount	*mp,
-	int64_t			delta)
-{
-	percpu_counter_add(&mp->m_ifree, delta);
-	if (percpu_counter_compare(&mp->m_ifree, 0) < 0) {
-		ASSERT(0);
-		percpu_counter_add(&mp->m_ifree, -delta);
-		return -EINVAL;
-	}
-	return 0;
-}
-
 /*
  * Deltas for the block count can vary from 1 to very large, but lock contention
  * only occurs on frequent small block count updates such as in the delayed
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index aba5a15792792..4835581f3eb00 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -392,8 +392,6 @@ extern int	xfs_initialize_perag(xfs_mount_t *mp, xfs_agnumber_t agcount,
 				     xfs_agnumber_t *maxagi);
 extern void	xfs_unmountfs(xfs_mount_t *);
 
-extern int	xfs_mod_icount(struct xfs_mount *mp, int64_t delta);
-extern int	xfs_mod_ifree(struct xfs_mount *mp, int64_t delta);
 extern int	xfs_mod_fdblocks(struct xfs_mount *mp, int64_t delta,
 				 bool reserved);
 extern int	xfs_mod_frextents(struct xfs_mount *mp, int64_t delta);
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 4522ceaaf57ba..b055a5ab53465 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -545,7 +545,12 @@ xfs_trans_apply_sb_deltas(
  * used block counts are not updated in the on disk superblock. In this case,
  * XFS_TRANS_SB_DIRTY will not be set when the transaction is updated but we
  * still need to update the incore superblock with the changes.
+ *
+ * Deltas for the inode count are +/-64, hence we use a large batch size of 128
+ * so we don't need to take the counter lock on every update.
  */
+#define XFS_ICOUNT_BATCH	128
+
 void
 xfs_trans_unreserve_and_mod_sb(
 	struct xfs_trans	*tp)
@@ -585,13 +590,17 @@ xfs_trans_unreserve_and_mod_sb(
 	}
 
 	if (idelta) {
-		error = xfs_mod_icount(mp, idelta);
-		ASSERT(!error);
+		percpu_counter_add_batch(&mp->m_icount, idelta,
+					 XFS_ICOUNT_BATCH);
+		if (idelta < 0)
+			ASSERT(__percpu_counter_compare(&mp->m_icount, 0,
+							XFS_ICOUNT_BATCH) >= 0);
 	}
 
 	if (ifreedelta) {
-		error = xfs_mod_ifree(mp, ifreedelta);
-		ASSERT(!error);
+		percpu_counter_add(&mp->m_ifree, ifreedelta);
+		if (ifreedelta < 0)
+			ASSERT(percpu_counter_compare(&mp->m_ifree, 0) >= 0);
 	}
 
 	if (rtxdelta == 0 && !(tp->t_flags & XFS_TRANS_SB_DIRTY))
-- 
2.26.2.761.g0e0b3e54be

Re: [PATCH 1/2] xfs: gut error handling in xfs_trans_unreserve_and_mod_sb()

[Christoph Hellwig]

On Wed, May 20, 2020 at 07:48:39AM +1000, Dave Chinner wrote:
> +	/*
> +	 * Debug checks outside of the spinlock so they don't lock up the
> +	 * machine if they fail.
> +	 */
> +	ASSERT(&mp->m_sb.sb_frextents >= 0);
> +	ASSERT(&mp->m_sb.sb_dblocks >= 0);
> +	ASSERT(&mp->m_sb.sb_agcount >= 0);

To stick to the theme of broken error handling I don't think this
does what you think as this takes the address of each field, which
will aways be >= 0.  I like the idea of the patch, though.

Re: [PATCH 2/2] xfs: reduce free inode accounting overhead

[Christoph Hellwig]

On Wed, May 20, 2020 at 07:48:40AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Shaokun Zhang reported that XFs was using substantial CPU time in

s/XFs/XFS/

>  	if (idelta) {
> -		error = xfs_mod_icount(mp, idelta);
> -		ASSERT(!error);
> +		percpu_counter_add_batch(&mp->m_icount, idelta,
> +					 XFS_ICOUNT_BATCH);
> +		if (idelta < 0)
> +			ASSERT(__percpu_counter_compare(&mp->m_icount, 0,
> +							XFS_ICOUNT_BATCH) >= 0);
>  	}
>  
>  	if (ifreedelta) {
> -		error = xfs_mod_ifree(mp, ifreedelta);
> -		ASSERT(!error);
> +		percpu_counter_add(&mp->m_ifree, ifreedelta);
> +		if (ifreedelta < 0)
> +			ASSERT(percpu_counter_compare(&mp->m_ifree, 0) >= 0);

I'd be tempted to just remove the ASSERTS entirely, as they are still
pretty heavy handed for debug kernels.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/2] xfs: gut error handling in xfs_trans_unreserve_and_mod_sb()

[Dave Chinner]

On Tue, May 19, 2020 at 11:53:34PM -0700, Christoph Hellwig wrote:
> On Wed, May 20, 2020 at 07:48:39AM +1000, Dave Chinner wrote:
> > +	/*
> > +	 * Debug checks outside of the spinlock so they don't lock up the
> > +	 * machine if they fail.
> > +	 */
> > +	ASSERT(&mp->m_sb.sb_frextents >= 0);
> > +	ASSERT(&mp->m_sb.sb_dblocks >= 0);
> > +	ASSERT(&mp->m_sb.sb_agcount >= 0);
> 
> To stick to the theme of broken error handling I don't think this
> does what you think as this takes the address of each field, which
> will aways be >= 0.  I like the idea of the patch, though.

Ah, search and replace fail. I'll fix it.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 1/2 V2] xfs: gut error handling in xfs_trans_unreserve_and_mod_sb()

[Dave Chinner]

xfs: gut error handling in xfs_trans_unreserve_and_mod_sb()

From: Dave Chinner <dchinner@redhat.com>

The error handling in xfs_trans_unreserve_and_mod_sb() is largely
incorrect - rolling back the changes in the transaction if only one
counter underruns makes all the other counters incorrect. We still
allow the change to proceed and committing the transaction, except
now we have multiple incorrect counters instead of a single
underflow.

Further, we don't actually report the error to the caller, so this
is completely silent except on debug kernels that will assert on
failure before we even get to the rollback code.  Hence this error
handling is broken, untested, and largely unnecessary complexity.

Just remove it.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_trans.c | 170 +++++++++--------------------------------------------
 1 file changed, 27 insertions(+), 143 deletions(-)

diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 28b983ff8b113..4522ceaaf57ba 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -534,57 +534,9 @@ xfs_trans_apply_sb_deltas(
 				  sizeof(sbp->sb_frextents) - 1);
 }
 
-STATIC int
-xfs_sb_mod8(
-	uint8_t			*field,
-	int8_t			delta)
-{
-	int8_t			counter = *field;
-
-	counter += delta;
-	if (counter < 0) {
-		ASSERT(0);
-		return -EINVAL;
-	}
-	*field = counter;
-	return 0;
-}
-
-STATIC int
-xfs_sb_mod32(
-	uint32_t		*field,
-	int32_t			delta)
-{
-	int32_t			counter = *field;
-
-	counter += delta;
-	if (counter < 0) {
-		ASSERT(0);
-		return -EINVAL;
-	}
-	*field = counter;
-	return 0;
-}
-
-STATIC int
-xfs_sb_mod64(
-	uint64_t		*field,
-	int64_t			delta)
-{
-	int64_t			counter = *field;
-
-	counter += delta;
-	if (counter < 0) {
-		ASSERT(0);
-		return -EINVAL;
-	}
-	*field = counter;
-	return 0;
-}
-
 /*
- * xfs_trans_unreserve_and_mod_sb() is called to release unused reservations
- * and apply superblock counter changes to the in-core superblock.  The
+ * xfs_trans_unreserve_and_mod_sb() is called to release unused reservations and
+ * apply superblock counter changes to the in-core superblock.  The
  * t_res_fdblocks_delta and t_res_frextents_delta fields are explicitly NOT
  * applied to the in-core superblock.  The idea is that that has already been
  * done.
@@ -629,20 +581,17 @@ xfs_trans_unreserve_and_mod_sb(
 	/* apply the per-cpu counters */
 	if (blkdelta) {
 		error = xfs_mod_fdblocks(mp, blkdelta, rsvd);
-		if (error)
-			goto out;
+		ASSERT(!error);
 	}
 
 	if (idelta) {
 		error = xfs_mod_icount(mp, idelta);
-		if (error)
-			goto out_undo_fdblocks;
+		ASSERT(!error);
 	}
 
 	if (ifreedelta) {
 		error = xfs_mod_ifree(mp, ifreedelta);
-		if (error)
-			goto out_undo_icount;
+		ASSERT(!error);
 	}
 
 	if (rtxdelta == 0 && !(tp->t_flags & XFS_TRANS_SB_DIRTY))
@@ -650,95 +599,30 @@ xfs_trans_unreserve_and_mod_sb(
 
 	/* apply remaining deltas */
 	spin_lock(&mp->m_sb_lock);
-	if (rtxdelta) {
-		error = xfs_sb_mod64(&mp->m_sb.sb_frextents, rtxdelta);
-		if (error)
-			goto out_undo_ifree;
-	}
-
-	if (tp->t_dblocks_delta != 0) {
-		error = xfs_sb_mod64(&mp->m_sb.sb_dblocks, tp->t_dblocks_delta);
-		if (error)
-			goto out_undo_frextents;
-	}
-	if (tp->t_agcount_delta != 0) {
-		error = xfs_sb_mod32(&mp->m_sb.sb_agcount, tp->t_agcount_delta);
-		if (error)
-			goto out_undo_dblocks;
-	}
-	if (tp->t_imaxpct_delta != 0) {
-		error = xfs_sb_mod8(&mp->m_sb.sb_imax_pct, tp->t_imaxpct_delta);
-		if (error)
-			goto out_undo_agcount;
-	}
-	if (tp->t_rextsize_delta != 0) {
-		error = xfs_sb_mod32(&mp->m_sb.sb_rextsize,
-				     tp->t_rextsize_delta);
-		if (error)
-			goto out_undo_imaxpct;
-	}
-	if (tp->t_rbmblocks_delta != 0) {
-		error = xfs_sb_mod32(&mp->m_sb.sb_rbmblocks,
-				     tp->t_rbmblocks_delta);
-		if (error)
-			goto out_undo_rextsize;
-	}
-	if (tp->t_rblocks_delta != 0) {
-		error = xfs_sb_mod64(&mp->m_sb.sb_rblocks, tp->t_rblocks_delta);
-		if (error)
-			goto out_undo_rbmblocks;
-	}
-	if (tp->t_rextents_delta != 0) {
-		error = xfs_sb_mod64(&mp->m_sb.sb_rextents,
-				     tp->t_rextents_delta);
-		if (error)
-			goto out_undo_rblocks;
-	}
-	if (tp->t_rextslog_delta != 0) {
-		error = xfs_sb_mod8(&mp->m_sb.sb_rextslog,
-				     tp->t_rextslog_delta);
-		if (error)
-			goto out_undo_rextents;
-	}
+	mp->m_sb.sb_frextents += rtxdelta;
+	mp->m_sb.sb_dblocks += tp->t_dblocks_delta;
+	mp->m_sb.sb_agcount += tp->t_agcount_delta;
+	mp->m_sb.sb_imax_pct += tp->t_imaxpct_delta;
+	mp->m_sb.sb_rextsize += tp->t_rextsize_delta;
+	mp->m_sb.sb_rbmblocks += tp->t_rbmblocks_delta;
+	mp->m_sb.sb_rblocks += tp->t_rblocks_delta;
+	mp->m_sb.sb_rextents += tp->t_rextents_delta;
+	mp->m_sb.sb_rextslog += tp->t_rextslog_delta;
 	spin_unlock(&mp->m_sb_lock);
-	return;
 
-out_undo_rextents:
-	if (tp->t_rextents_delta)
-		xfs_sb_mod64(&mp->m_sb.sb_rextents, -tp->t_rextents_delta);
-out_undo_rblocks:
-	if (tp->t_rblocks_delta)
-		xfs_sb_mod64(&mp->m_sb.sb_rblocks, -tp->t_rblocks_delta);
-out_undo_rbmblocks:
-	if (tp->t_rbmblocks_delta)
-		xfs_sb_mod32(&mp->m_sb.sb_rbmblocks, -tp->t_rbmblocks_delta);
-out_undo_rextsize:
-	if (tp->t_rextsize_delta)
-		xfs_sb_mod32(&mp->m_sb.sb_rextsize, -tp->t_rextsize_delta);
-out_undo_imaxpct:
-	if (tp->t_rextsize_delta)
-		xfs_sb_mod8(&mp->m_sb.sb_imax_pct, -tp->t_imaxpct_delta);
-out_undo_agcount:
-	if (tp->t_agcount_delta)
-		xfs_sb_mod32(&mp->m_sb.sb_agcount, -tp->t_agcount_delta);
-out_undo_dblocks:
-	if (tp->t_dblocks_delta)
-		xfs_sb_mod64(&mp->m_sb.sb_dblocks, -tp->t_dblocks_delta);
-out_undo_frextents:
-	if (rtxdelta)
-		xfs_sb_mod64(&mp->m_sb.sb_frextents, -rtxdelta);
-out_undo_ifree:
-	spin_unlock(&mp->m_sb_lock);
-	if (ifreedelta)
-		xfs_mod_ifree(mp, -ifreedelta);
-out_undo_icount:
-	if (idelta)
-		xfs_mod_icount(mp, -idelta);
-out_undo_fdblocks:
-	if (blkdelta)
-		xfs_mod_fdblocks(mp, -blkdelta, rsvd);
-out:
-	ASSERT(error == 0);
+	/*
+	 * Debug checks outside of the spinlock so they don't lock up the
+	 * machine if they fail.
+	 */
+	ASSERT(mp->m_sb.sb_frextents >= 0);
+	ASSERT(mp->m_sb.sb_dblocks >= 0);
+	ASSERT(mp->m_sb.sb_agcount >= 0);
+	ASSERT(mp->m_sb.sb_imax_pct >= 0);
+	ASSERT(mp->m_sb.sb_rextsize >= 0);
+	ASSERT(mp->m_sb.sb_rbmblocks >= 0);
+	ASSERT(mp->m_sb.sb_rblocks >= 0);
+	ASSERT(mp->m_sb.sb_rextents >= 0);
+	ASSERT(mp->m_sb.sb_rextslog >= 0);
 	return;
 }
 

Re: [PATCH 1/2 V2] xfs: gut error handling in xfs_trans_unreserve_and_mod_sb()

[Christoph Hellwig]

On Wed, May 20, 2020 at 05:33:58PM +1000, Dave Chinner wrote:
> +	/*
> +	 * Debug checks outside of the spinlock so they don't lock up the
> +	 * machine if they fail.
> +	 */
> +	ASSERT(mp->m_sb.sb_frextents >= 0);
> +	ASSERT(mp->m_sb.sb_dblocks >= 0);
> +	ASSERT(mp->m_sb.sb_agcount >= 0);
> +	ASSERT(mp->m_sb.sb_imax_pct >= 0);
> +	ASSERT(mp->m_sb.sb_rextsize >= 0);
> +	ASSERT(mp->m_sb.sb_rbmblocks >= 0);
> +	ASSERT(mp->m_sb.sb_rblocks >= 0);
> +	ASSERT(mp->m_sb.sb_rextents >= 0);
> +	ASSERT(mp->m_sb.sb_rextslog >= 0);
>  	return;

No need for the return here at the end of the function.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/2 V2] xfs: gut error handling in xfs_trans_unreserve_and_mod_sb()

[Darrick J. Wong]

On Wed, May 20, 2020 at 12:48:05AM -0700, Christoph Hellwig wrote:
> On Wed, May 20, 2020 at 05:33:58PM +1000, Dave Chinner wrote:
> > +	/*
> > +	 * Debug checks outside of the spinlock so they don't lock up the
> > +	 * machine if they fail.
> > +	 */
> > +	ASSERT(mp->m_sb.sb_frextents >= 0);
> > +	ASSERT(mp->m_sb.sb_dblocks >= 0);
> > +	ASSERT(mp->m_sb.sb_agcount >= 0);
> > +	ASSERT(mp->m_sb.sb_imax_pct >= 0);
> > +	ASSERT(mp->m_sb.sb_rextsize >= 0);
> > +	ASSERT(mp->m_sb.sb_rbmblocks >= 0);
> > +	ASSERT(mp->m_sb.sb_rblocks >= 0);
> > +	ASSERT(mp->m_sb.sb_rextents >= 0);
> > +	ASSERT(mp->m_sb.sb_rextslog >= 0);

Except for imax_pct and rextslog, all of these are unsigned quantities,
right?  So the asserts will /never/ trigger?

--D

> >  	return;
> 
> No need for the return here at the end of the function.
> 
> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] xfs: reduce free inode accounting overhead

[Darrick J. Wong]

On Wed, May 20, 2020 at 07:48:40AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Shaokun Zhang reported that XFs was using substantial CPU time in
> percpu_count_sum() when running a single threaded benchmark on
> a high CPU count (128p) machine from xfs_mod_ifree(). The issue
> is that the filesystem is empty when the benchmark runs, so inode
> allocation is running with a very low inode free count.
> 
> With the percpu counter batching, this means comparisons when the
> counter is less that 128 * 256 = 32768 use the slow path of adding
> up all the counters across the CPUs, and this is expensive on high
> CPU count machines.
> 
> The summing in xfs_mod_ifree() is only used to fire an assert if an
> underrun occurs. The error is ignored by the higher level code.
> Hence this is really just debug code and we don't need to run it
> on production kernels, nor do we need such debug checks to return
> error values just to trigger an assert.
> 
> Finally, xfs_mod_icount/xfs_mod_ifree are only called from
> xfs_trans_unreserve_and_mod_sb(), so get rid of them and just
> directly call the percpu_counter_add/percpu_counter_compare
> functions. The compare functions are now run only on debug builds as
> they are internal to ASSERT() checks and so only compiled in when
> ASSERTs are active (CONFIG_XFS_DEBUG=y or CONFIG_XFS_WARN=y).
> 
> Reported-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Seems like a reasonable substitution/ASSERT reduction to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_mount.c | 33 ---------------------------------
>  fs/xfs/xfs_mount.h |  2 --
>  fs/xfs/xfs_trans.c | 17 +++++++++++++----
>  3 files changed, 13 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index bb91f04266b9a..d5dcf98698600 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -1189,39 +1189,6 @@ xfs_log_sbcount(xfs_mount_t *mp)
>  	return xfs_sync_sb(mp, true);
>  }
>  
> -/*
> - * Deltas for the inode count are +/-64, hence we use a large batch size
> - * of 128 so we don't need to take the counter lock on every update.
> - */
> -#define XFS_ICOUNT_BATCH	128
> -int
> -xfs_mod_icount(
> -	struct xfs_mount	*mp,
> -	int64_t			delta)
> -{
> -	percpu_counter_add_batch(&mp->m_icount, delta, XFS_ICOUNT_BATCH);
> -	if (__percpu_counter_compare(&mp->m_icount, 0, XFS_ICOUNT_BATCH) < 0) {
> -		ASSERT(0);
> -		percpu_counter_add(&mp->m_icount, -delta);
> -		return -EINVAL;
> -	}
> -	return 0;
> -}
> -
> -int
> -xfs_mod_ifree(
> -	struct xfs_mount	*mp,
> -	int64_t			delta)
> -{
> -	percpu_counter_add(&mp->m_ifree, delta);
> -	if (percpu_counter_compare(&mp->m_ifree, 0) < 0) {
> -		ASSERT(0);
> -		percpu_counter_add(&mp->m_ifree, -delta);
> -		return -EINVAL;
> -	}
> -	return 0;
> -}
> -
>  /*
>   * Deltas for the block count can vary from 1 to very large, but lock contention
>   * only occurs on frequent small block count updates such as in the delayed
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index aba5a15792792..4835581f3eb00 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -392,8 +392,6 @@ extern int	xfs_initialize_perag(xfs_mount_t *mp, xfs_agnumber_t agcount,
>  				     xfs_agnumber_t *maxagi);
>  extern void	xfs_unmountfs(xfs_mount_t *);
>  
> -extern int	xfs_mod_icount(struct xfs_mount *mp, int64_t delta);
> -extern int	xfs_mod_ifree(struct xfs_mount *mp, int64_t delta);
>  extern int	xfs_mod_fdblocks(struct xfs_mount *mp, int64_t delta,
>  				 bool reserved);
>  extern int	xfs_mod_frextents(struct xfs_mount *mp, int64_t delta);
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 4522ceaaf57ba..b055a5ab53465 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -545,7 +545,12 @@ xfs_trans_apply_sb_deltas(
>   * used block counts are not updated in the on disk superblock. In this case,
>   * XFS_TRANS_SB_DIRTY will not be set when the transaction is updated but we
>   * still need to update the incore superblock with the changes.
> + *
> + * Deltas for the inode count are +/-64, hence we use a large batch size of 128
> + * so we don't need to take the counter lock on every update.
>   */
> +#define XFS_ICOUNT_BATCH	128
> +
>  void
>  xfs_trans_unreserve_and_mod_sb(
>  	struct xfs_trans	*tp)
> @@ -585,13 +590,17 @@ xfs_trans_unreserve_and_mod_sb(
>  	}
>  
>  	if (idelta) {
> -		error = xfs_mod_icount(mp, idelta);
> -		ASSERT(!error);
> +		percpu_counter_add_batch(&mp->m_icount, idelta,
> +					 XFS_ICOUNT_BATCH);
> +		if (idelta < 0)
> +			ASSERT(__percpu_counter_compare(&mp->m_icount, 0,
> +							XFS_ICOUNT_BATCH) >= 0);
>  	}
>  
>  	if (ifreedelta) {
> -		error = xfs_mod_ifree(mp, ifreedelta);
> -		ASSERT(!error);
> +		percpu_counter_add(&mp->m_ifree, ifreedelta);
> +		if (ifreedelta < 0)
> +			ASSERT(percpu_counter_compare(&mp->m_ifree, 0) >= 0);
>  	}
>  
>  	if (rtxdelta == 0 && !(tp->t_flags & XFS_TRANS_SB_DIRTY))
> -- 
> 2.26.2.761.g0e0b3e54be
> 

Re: [PATCH 1/2 V2] xfs: gut error handling in xfs_trans_unreserve_and_mod_sb()

[Dave Chinner]

On Wed, May 20, 2020 at 01:27:02PM -0700, Darrick J. Wong wrote:
> On Wed, May 20, 2020 at 12:48:05AM -0700, Christoph Hellwig wrote:
> > On Wed, May 20, 2020 at 05:33:58PM +1000, Dave Chinner wrote:
> > > +	/*
> > > +	 * Debug checks outside of the spinlock so they don't lock up the
> > > +	 * machine if they fail.
> > > +	 */
> > > +	ASSERT(mp->m_sb.sb_frextents >= 0);
> > > +	ASSERT(mp->m_sb.sb_dblocks >= 0);
> > > +	ASSERT(mp->m_sb.sb_agcount >= 0);
> > > +	ASSERT(mp->m_sb.sb_imax_pct >= 0);
> > > +	ASSERT(mp->m_sb.sb_rextsize >= 0);
> > > +	ASSERT(mp->m_sb.sb_rbmblocks >= 0);
> > > +	ASSERT(mp->m_sb.sb_rblocks >= 0);
> > > +	ASSERT(mp->m_sb.sb_rextents >= 0);
> > > +	ASSERT(mp->m_sb.sb_rextslog >= 0);
> 
> Except for imax_pct and rextslog, all of these are unsigned quantities,
> right?  So the asserts will /never/ trigger?

In truth, I didn't look that far. I just assumed that because all
the xfs_sb_mod*() functions used signed math that they could all
underflow/overflow.  IOWs, the checking for overflow/underflow was
completely wrong in the first place.

Should I just remove the ASSERT()s entirely?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2 V2] xfs: gut error handling in xfs_trans_unreserve_and_mod_sb()

[Darrick J. Wong]

On Thu, May 21, 2020 at 07:55:30AM +1000, Dave Chinner wrote:
> On Wed, May 20, 2020 at 01:27:02PM -0700, Darrick J. Wong wrote:
> > On Wed, May 20, 2020 at 12:48:05AM -0700, Christoph Hellwig wrote:
> > > On Wed, May 20, 2020 at 05:33:58PM +1000, Dave Chinner wrote:
> > > > +	/*
> > > > +	 * Debug checks outside of the spinlock so they don't lock up the
> > > > +	 * machine if they fail.
> > > > +	 */
> > > > +	ASSERT(mp->m_sb.sb_frextents >= 0);
> > > > +	ASSERT(mp->m_sb.sb_dblocks >= 0);
> > > > +	ASSERT(mp->m_sb.sb_agcount >= 0);
> > > > +	ASSERT(mp->m_sb.sb_imax_pct >= 0);
> > > > +	ASSERT(mp->m_sb.sb_rextsize >= 0);
> > > > +	ASSERT(mp->m_sb.sb_rbmblocks >= 0);
> > > > +	ASSERT(mp->m_sb.sb_rblocks >= 0);
> > > > +	ASSERT(mp->m_sb.sb_rextents >= 0);
> > > > +	ASSERT(mp->m_sb.sb_rextslog >= 0);
> > 
> > Except for imax_pct and rextslog, all of these are unsigned quantities,
> > right?  So the asserts will /never/ trigger?
> 
> In truth, I didn't look that far. I just assumed that because all
> the xfs_sb_mod*() functions used signed math that they could all
> underflow/overflow.  IOWs, the checking for overflow/underflow was
> completely wrong in the first place.
> 
> Should I just remove the ASSERT()s entirely?

It causes a bunch of gcc 9.3 warnings, so yes please. :)

(Granted, I ripped out all the asserts except for the two I mentioned
above, so if nobody else have complaints then no need to resend.)

--D

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

Re: [PATCH 1/2 V2] xfs: gut error handling in xfs_trans_unreserve_and_mod_sb()

[Dave Chinner]

On Wed, May 20, 2020 at 03:28:21PM -0700, Darrick J. Wong wrote:
> On Thu, May 21, 2020 at 07:55:30AM +1000, Dave Chinner wrote:
> > On Wed, May 20, 2020 at 01:27:02PM -0700, Darrick J. Wong wrote:
> > > On Wed, May 20, 2020 at 12:48:05AM -0700, Christoph Hellwig wrote:
> > > > On Wed, May 20, 2020 at 05:33:58PM +1000, Dave Chinner wrote:
> > > > > +	/*
> > > > > +	 * Debug checks outside of the spinlock so they don't lock up the
> > > > > +	 * machine if they fail.
> > > > > +	 */
> > > > > +	ASSERT(mp->m_sb.sb_frextents >= 0);
> > > > > +	ASSERT(mp->m_sb.sb_dblocks >= 0);
> > > > > +	ASSERT(mp->m_sb.sb_agcount >= 0);
> > > > > +	ASSERT(mp->m_sb.sb_imax_pct >= 0);
> > > > > +	ASSERT(mp->m_sb.sb_rextsize >= 0);
> > > > > +	ASSERT(mp->m_sb.sb_rbmblocks >= 0);
> > > > > +	ASSERT(mp->m_sb.sb_rblocks >= 0);
> > > > > +	ASSERT(mp->m_sb.sb_rextents >= 0);
> > > > > +	ASSERT(mp->m_sb.sb_rextslog >= 0);
> > > 
> > > Except for imax_pct and rextslog, all of these are unsigned quantities,
> > > right?  So the asserts will /never/ trigger?
> > 
> > In truth, I didn't look that far. I just assumed that because all
> > the xfs_sb_mod*() functions used signed math that they could all
> > underflow/overflow.  IOWs, the checking for overflow/underflow was
> > completely wrong in the first place.
> > 
> > Should I just remove the ASSERT()s entirely?
> 
> It causes a bunch of gcc 9.3 warnings, so yes please. :)
> 
> (Granted, I ripped out all the asserts except for the two I mentioned
> above, so if nobody else have complaints then no need to resend.)

Fine by me. FWIW, gcc 9.2 doesn't complain at all about these.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com