Avoid dp_lock in the dirty space accounting

dsl_pool_dirty_space() and dsl_pool_undirty_space() are called once per
logical block, from open context and from every ZIO completion, all
serializing on the single pool-wide dp_lock.  On a 12-thread 32KB
overwrite benchmark they took 33% of CPU, almost all of it spinning.

The lock only protected the counters against each other, so make
dp_dirty_pertxg/dp_dirty_total and dp_sync_reserve_pertxg/_total plain
atomics.  The subtractions clamp at zero via a CAS loop.  Reading these
counters without the lock was already the norm.

The cv_signal() on every accounting event is only ever needed by
dmu_tx_wait(), which sleeps solely at the hard zfs_dirty_data_max wall,
so gate it on a count of waiters.  Both sides need membar_sync(), since
the atomics used here provide no ordering.  dmu_tx_wait() in turn only
registers as a waiter if it may really sleep, keeping dp_lock off the
soft throttle path taken by every delayed transaction.

This gives 20.6 -> 26.0 GiB/s on the above benchmark, with the code
almost disappearing from CPU profiles.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Closes #19126

include/sys/dsl_pool.h

@@ -103,13 +103,16 @@ typedef struct dsl_pool {
 
 	struct dsl_scan *dp_scan;
 
-	/* Uses dp_lock */
-	kmutex_t dp_lock;
-	kcondvar_t dp_spaceavail_cv;
+	/* Updated with atomics, read without a lock */
 	uint64_t dp_dirty_pertxg[TXG_SIZE];
 	uint64_t dp_dirty_total;
 	uint64_t dp_sync_reserve_pertxg[TXG_SIZE];
 	uint64_t dp_sync_reserve_total;
+
+	/* Uses dp_lock */
+	kmutex_t dp_lock;
+	kcondvar_t dp_spaceavail_cv;
+	uint64_t dp_dirty_waiters;	/* also read unlocked */
 	uint64_t dp_long_free_dirty_pertxg[TXG_SIZE];
 
 	aggsum_t dp_wrlog_pertxg[TXG_SIZE];

module/zfs/dmu_tx.c

@@ -972,9 +972,9 @@ dmu_tx_delay(dmu_tx_t *tx, uint64_t dirty)
 	    zfs_dirty_data_max * zfs_delay_min_dirty_percent / 100;
 	if (dirty >= zfs_dirty_data_max) {
 		/*
-		 * The caller has already waited until the dirty data is
-		 * under the max, but the sync context reservations added
-		 * on top of it may push the sum beyond it.
+		 * The sync context reservations added on top of the dirty
+		 * data may push the sum beyond the max, as may the dirty
+		 * data itself growing after the caller has checked it.
 		 */
 		tx_time = zfs_delay_max_ns;
 	} else if (dirty > delay_min_bytes) {
@@ -1357,15 +1357,26 @@ dmu_tx_wait(dmu_tx_t *tx)
 		/*
 		 * dmu_tx_try_assign() has determined that we need to wait
 		 * because we've consumed much or all of the dirty buffer
-		 * space.
+		 * space.  The hard limit though is rarely reached, so check
+		 * it unlocked, taking the lock only if we may have to wait.
 		 */
-		mutex_enter(&dp->dp_lock);
-		if (dp->dp_dirty_total >= zfs_dirty_data_max)
+		if (dp->dp_dirty_total >= zfs_dirty_data_max) {
 			DMU_TX_STAT_BUMP(dmu_tx_dirty_over_max);
-		while (dp->dp_dirty_total >= zfs_dirty_data_max)
-			cv_wait(&dp->dp_spaceavail_cv, &dp->dp_lock);
+			mutex_enter(&dp->dp_lock);
+			/*
+			 * Register before loading dp_dirty_total, since
+			 * dsl_pool_dirty_delta() only signals when it sees
+			 * dp_dirty_waiters.  See the comment there about
+			 * ordering.
+			 */
+			dp->dp_dirty_waiters++;
+			membar_sync();
+			while (dp->dp_dirty_total >= zfs_dirty_data_max)
+				cv_wait(&dp->dp_spaceavail_cv, &dp->dp_lock);
+			dp->dp_dirty_waiters--;
+			mutex_exit(&dp->dp_lock);
+		}
 		dirty = dp->dp_dirty_total + dp->dp_sync_reserve_total;
-		mutex_exit(&dp->dp_lock);
 
 		dmu_tx_delay(tx, dirty);
 

module/zfs/dsl_pool.c

@@ -589,22 +589,49 @@ dsl_pool_sync_mos(dsl_pool_t *dp, dmu_tx_t *tx)
 	spa_set_rootblkptr(dp->dp_spa, &dp->dp_meta_rootbp);
 }
 
+/*
+ * Subtract up to space from one of the per-txg counters, returning the
+ * amount actually subtracted.  The counters never go negative, since the
+ * callers may try to give back more than the counter was charged.
+ */
+static uint64_t
+dsl_pool_sub_pertxg(uint64_t *pertxg, int64_t space)
+{
+	uint64_t cur, sub;
+
+	do {
+		cur = *pertxg;
+		sub = MIN((uint64_t)space, cur);
+	} while (atomic_cas_64(pertxg, cur, cur - sub) != cur);
+
+	return (sub);
+}
+
 static void
 dsl_pool_dirty_delta(dsl_pool_t *dp, int64_t delta)
 {
-	ASSERT(MUTEX_HELD(&dp->dp_lock));
-
-	if (delta < 0)
-		ASSERT3U(-delta, <=, dp->dp_dirty_total);
-
-	dp->dp_dirty_total += delta;
+	uint64_t total = atomic_add_64_nv(&dp->dp_dirty_total, delta);
+	ASSERT3S((int64_t)total, >=, 0);
 
 	/*
 	 * Note: we signal even when increasing dp_dirty_total.
 	 * This ensures forward progress -- each thread wakes the next waiter.
 	 */
-	if (dp->dp_dirty_total < zfs_dirty_data_max)
+	if (total >= zfs_dirty_data_max)
+		return;
+
+	/*
+	 * atomic_add_64_nv() above provides no ordering, so explicitly order
+	 * the store to dp_dirty_total against the load of dp_dirty_waiters.
+	 * dmu_tx_wait() does the reverse, so at least one of the two sees
+	 * the other and the wakeup can not be lost.
+	 */
+	membar_sync();
+	if (dp->dp_dirty_waiters > 0) {
+		mutex_enter(&dp->dp_lock);
 		cv_signal(&dp->dp_spaceavail_cv);
+		mutex_exit(&dp->dp_lock);
+	}
 }
 
 void
@@ -992,14 +1019,12 @@ void
 dsl_pool_dirty_space(dsl_pool_t *dp, int64_t space, dmu_tx_t *tx)
 {
 	if (space > 0) {
-		mutex_enter(&dp->dp_lock);
-		dp->dp_dirty_pertxg[tx->tx_txg & TXG_MASK] += space;
+		atomic_add_64(&dp->dp_dirty_pertxg[tx->tx_txg & TXG_MASK],
+		    space);
 		dsl_pool_dirty_delta(dp, space);
-		boolean_t needsync = !dmu_tx_is_syncing(tx) &&
-		    dsl_pool_need_dirty_sync(dp, tx->tx_txg);
-		mutex_exit(&dp->dp_lock);
 
-		if (needsync)
+		if (!dmu_tx_is_syncing(tx) &&
+		    dsl_pool_need_dirty_sync(dp, tx->tx_txg))
 			txg_kick(dp, tx->tx_txg);
 	}
 }
@@ -1028,15 +1053,14 @@ dsl_pool_dirty_mos_space(dsl_pool_t *dp, int64_t space, dmu_tx_t *tx)
 		return;
 
 	uint64_t txgoff = tx->tx_txg & TXG_MASK;
-	mutex_enter(&dp->dp_lock);
-	uint64_t resv = MIN((uint64_t)space,
-	    dp->dp_sync_reserve_pertxg[txgoff]);
-	dp->dp_sync_reserve_pertxg[txgoff] -= resv;
-	ASSERT3U(dp->dp_sync_reserve_total, >=, resv);
-	dp->dp_sync_reserve_total -= resv;
-	dp->dp_dirty_pertxg[txgoff] += space;
+	uint64_t resv = dsl_pool_sub_pertxg(
+	    &dp->dp_sync_reserve_pertxg[txgoff], space);
+	uint64_t left = atomic_add_64_nv(&dp->dp_sync_reserve_total,
+	    -(int64_t)resv);
+	ASSERT3S((int64_t)left, >=, 0);
+
+	atomic_add_64(&dp->dp_dirty_pertxg[txgoff], space);
 	dsl_pool_dirty_delta(dp, space);
-	mutex_exit(&dp->dp_lock);
 }
 
 void
@@ -1046,16 +1070,11 @@ dsl_pool_undirty_space(dsl_pool_t *dp, int64_t space, uint64_t txg)
 	if (space == 0)
 		return;
 
-	mutex_enter(&dp->dp_lock);
-	if (dp->dp_dirty_pertxg[txg & TXG_MASK] < space) {
-		/* XXX writing something we didn't dirty? */
-		space = dp->dp_dirty_pertxg[txg & TXG_MASK];
-	}
-	ASSERT3U(dp->dp_dirty_pertxg[txg & TXG_MASK], >=, space);
-	dp->dp_dirty_pertxg[txg & TXG_MASK] -= space;
-	ASSERT3U(dp->dp_dirty_total, >=, space);
-	dsl_pool_dirty_delta(dp, -space);
-	mutex_exit(&dp->dp_lock);
+	/* XXX writing something we didn't dirty? */
+	uint64_t sub = dsl_pool_sub_pertxg(
+	    &dp->dp_dirty_pertxg[txg & TXG_MASK], space);
+
+	dsl_pool_dirty_delta(dp, -(int64_t)sub);
 }
 
 /*
@@ -1073,14 +1092,11 @@ dsl_pool_sync_reserve(dsl_pool_t *dp, uint64_t space, dmu_tx_t *tx)
 	if (space == 0)
 		return;
 
-	mutex_enter(&dp->dp_lock);
-	dp->dp_sync_reserve_pertxg[tx->tx_txg & TXG_MASK] += space;
-	dp->dp_sync_reserve_total += space;
-	boolean_t needsync = !dmu_tx_is_syncing(tx) &&
-	    dsl_pool_need_dirty_sync(dp, tx->tx_txg);
-	mutex_exit(&dp->dp_lock);
+	atomic_add_64(&dp->dp_sync_reserve_pertxg[tx->tx_txg & TXG_MASK],
+	    space);
+	atomic_add_64(&dp->dp_sync_reserve_total, space);
 
-	if (needsync)
+	if (!dmu_tx_is_syncing(tx) && dsl_pool_need_dirty_sync(dp, tx->tx_txg))
 		txg_kick(dp, tx->tx_txg);
 }
 
@@ -1092,12 +1108,11 @@ dsl_pool_sync_unreserve(dsl_pool_t *dp, uint64_t space, uint64_t txg)
 	if (space == 0)
 		return;
 
-	mutex_enter(&dp->dp_lock);
-	space = MIN(space, dp->dp_sync_reserve_pertxg[txg & TXG_MASK]);
-	dp->dp_sync_reserve_pertxg[txg & TXG_MASK] -= space;
-	ASSERT3U(dp->dp_sync_reserve_total, >=, space);
-	dp->dp_sync_reserve_total -= space;
-	mutex_exit(&dp->dp_lock);
+	space = dsl_pool_sub_pertxg(&dp->dp_sync_reserve_pertxg[txg & TXG_MASK],
+	    space);
+	uint64_t left = atomic_add_64_nv(&dp->dp_sync_reserve_total,
+	    -(int64_t)space);
+	ASSERT3S((int64_t)left, >=, 0);
 }
 
 static int

module/zfs/spa.c

@@ -2218,9 +2218,7 @@ spa_sync_time_logger(spa_t *spa, uint64_t txg, boolean_t force)
 	}
 	spa->spa_last_flush_txg_time = curtime;
 
-	mutex_enter(&dp->dp_lock);
 	dirty = dp->dp_dirty_pertxg[idx];
-	mutex_exit(&dp->dp_lock);
 	if (!force && dirty == 0) {
 		return;
 	}