Don't overrun the block budget in server_log_merge_free_work().

This fixes a potential fence post failure like the following:

error: 1 holders exceeded alloc budget av: bef 7407 now 7392, fr: bef 8185 now 7672

The code is only accounting for the freed btree blocks, not the dirtying of
other items. So it's possible to be at exactly (COMMIT_HOLD_ALLOC_BUDGET / 2),
dirty some log btree blocks, loop again, then consume another
(COMMIT_HOLD_ALLOC_BUDGET / 2) and blow past the total budget.

In this example, we went over by 13 blocks.

By only consuming up to 1/8 of the budget on each loop, and committing when we
have consumed 3/4 of the budget, we can avoid the fence post condition.

Signed-off-by: Chris Kirby <ckirby@versity.com>

ReleaseNotes.md

@@ -1,27 +1,6 @@
 Versity ScoutFS Release Notes
 =============================
 
----
-v1.25
-\
-*Jun 3, 2025*
-
-Fix a bug that could cause indefinite retries of failed client commits.
-Under specific error conditions the client and server's understanding of
-the current client commit could get out of sync.  The client would retry
-commits indefinitely that could never succeed.  This manifested as
-infinite "critical transaction commit failure" messages in the kernel
-log on the client and matching "error <nr> committing client logs" on
-the server.
-
-Fix a bug in a specific case of server error handling that could result
-in sending references to unwritten blocks to the client.  The client
-would try to read blocks that hadn't been written and return spurious
-errors.  This was seen under low free space conditions on the server and
-resulted in error messages with error code 116 (The errno enum for
-ESTALE, the client's indication that it couldn't read the blocks that it
-expected.)
-
 ---
 v1.24
 \

kmod/src/alloc.c

@@ -86,47 +86,18 @@ static u64 smallest_order_length(u64 len)
 }
 
 /*
- * Moving an extent between trees can dirty blocks in several ways. This
- * function calculates worst case number of blocks across these scenarions.
- * We treat the alloc and free counts independently, so the values below are
- * max(allocated, freed), not the sum.
- *
- * We track extents with two separate btree items: by block number and by size.
- *
- * If we're removing an extent from the btree (allocating), we can dirty
- * two blocks if the keys are in different leaves. If we wind up merging
- * leaves because we fall below the low water mark, we can wind up freeing
- * three leaves.
- *
- * That sequence is as follows, assuming the original keys are removed from
- * blocks A and B:
- *
- * Allocate new dirty A' and B'
- * Free old stable A and B
- * B' has fallen below the low water mark, so copy B' into A'
- * Free B'
- *
- * An extent insertion (freeing an extent) can dirty up to five distinct items
- * in the btree as it adds and removes the blkno and size sorted items for the
- * old and new lengths of the extent:
- *
- * In the by-blkno portion of the btree, we can dirty (allocate for COW) up
- * to two blocks- either by merging adjacent extents, which can cause us to
- * join leaf blocks; or by an insertion that causes a split.
- *
- * In the by-size portion, we never merge extents, so normally we just dirty
- * a single item with a size insertion. But if we merged adjacent extents in
- * the by-blkno portion of the tree, we might be working with three by-sizex
- * items: removing the two old ones that were combined in the merge; and
- * adding the new one for the larger, merged size.
- *
- * Finally, dirtying the paths to these leaves can grow the tree and grow/shrink
- * neighbours at each level, so we multiply by the height of the tree after
- * accounting for a possible new level.
+ * An extent modification dirties three distinct leaves of an allocator
+ * btree as it adds and removes the blkno and size sorted items for the
+ * old and new lengths of the extent.  Dirtying the paths to these
+ * leaves can grow the tree and grow/shrink neighbours at each level.
+ * We over-estimate the number of blocks allocated and freed (the paths
+ * share a root, growth doesn't free) to err on the simpler and safer
+ * side.  The overhead is minimal given the relatively large list blocks
+ * and relatively short allocator trees.
  */
 static u32 extent_mod_blocks(u32 height)
 {
-	return ((1 + height) * 3) * 5;
+	return ((1 + height) * 2) * 3;
 }
 
 /*

kmod/src/scoutfs_trace.h

@@ -1966,17 +1966,15 @@ DEFINE_EVENT(scoutfs_server_client_count_class, scoutfs_server_client_down,
 );
 
 DECLARE_EVENT_CLASS(scoutfs_server_commit_users_class,
-        TP_PROTO(struct super_block *sb, int holding, int applying,
-		 int nr_holders, u32 budget,
-		 u32 avail_before, u32 freed_before,
-		 int committing, int exceeded),
-        TP_ARGS(sb, holding, applying, nr_holders, budget, avail_before, freed_before, committing, exceeded),
+        TP_PROTO(struct super_block *sb, int holding, int applying, int nr_holders,
+		 u32 avail_before, u32 freed_before, int committing, int exceeded),
+        TP_ARGS(sb, holding, applying, nr_holders, avail_before, freed_before, committing,
+		exceeded),
         TP_STRUCT__entry(
 		SCSB_TRACE_FIELDS
 		__field(int, holding)
 		__field(int, applying)
 		__field(int, nr_holders)
-		__field(u32, budget)
 		__field(__u32, avail_before)
 		__field(__u32, freed_before)
 		__field(int, committing)
@@ -1987,45 +1985,35 @@ DECLARE_EVENT_CLASS(scoutfs_server_commit_users_class,
 		__entry->holding = !!holding;
 		__entry->applying = !!applying;
 		__entry->nr_holders = nr_holders;
-		__entry->budget = budget;
 		__entry->avail_before = avail_before;
 		__entry->freed_before = freed_before;
 		__entry->committing = !!committing;
 		__entry->exceeded = !!exceeded;
         ),
-	TP_printk(SCSBF" holding %u applying %u nr %u budget %u avail_before %u freed_before %u committing %u exceeded %u",
-		  SCSB_TRACE_ARGS, __entry->holding, __entry->applying,
-		  __entry->nr_holders, __entry->budget,
-		  __entry->avail_before, __entry->freed_before,
-		  __entry->committing, __entry->exceeded)
+	TP_printk(SCSBF" holding %u applying %u nr %u avail_before %u freed_before %u committing %u exceeded %u",
+		  SCSB_TRACE_ARGS, __entry->holding, __entry->applying, __entry->nr_holders,
+		  __entry->avail_before, __entry->freed_before, __entry->committing,
+		  __entry->exceeded)
 );
 DEFINE_EVENT(scoutfs_server_commit_users_class, scoutfs_server_commit_hold,
-        TP_PROTO(struct super_block *sb, int holding, int applying,
-		 int nr_holders, u32 budget,
-		 u32 avail_before, u32 freed_before,
-		 int committing, int exceeded),
-        TP_ARGS(sb, holding, applying, nr_holders, budget, avail_before, freed_before, committing, exceeded)
+        TP_PROTO(struct super_block *sb, int holding, int applying, int nr_holders,
+		 u32 avail_before, u32 freed_before, int committing, int exceeded),
+        TP_ARGS(sb, holding, applying, nr_holders, avail_before, freed_before, committing, exceeded)
 );
 DEFINE_EVENT(scoutfs_server_commit_users_class, scoutfs_server_commit_apply,
-        TP_PROTO(struct super_block *sb, int holding, int applying,
-		 int nr_holders, u32 budget,
-		 u32 avail_before, u32 freed_before,
-		 int committing, int exceeded),
-        TP_ARGS(sb, holding, applying, nr_holders, budget, avail_before, freed_before, committing, exceeded)
+        TP_PROTO(struct super_block *sb, int holding, int applying, int nr_holders,
+		 u32 avail_before, u32 freed_before, int committing, int exceeded),
+        TP_ARGS(sb, holding, applying, nr_holders, avail_before, freed_before, committing, exceeded)
 );
 DEFINE_EVENT(scoutfs_server_commit_users_class, scoutfs_server_commit_start,
-        TP_PROTO(struct super_block *sb, int holding, int applying,
-		 int nr_holders, u32 budget,
-		 u32 avail_before, u32 freed_before,
-		 int committing, int exceeded),
-        TP_ARGS(sb, holding, applying, nr_holders, budget, avail_before, freed_before, committing, exceeded)
+        TP_PROTO(struct super_block *sb, int holding, int applying, int nr_holders,
+		 u32 avail_before, u32 freed_before, int committing, int exceeded),
+        TP_ARGS(sb, holding, applying, nr_holders, avail_before, freed_before, committing, exceeded)
 );
 DEFINE_EVENT(scoutfs_server_commit_users_class, scoutfs_server_commit_end,
-        TP_PROTO(struct super_block *sb, int holding, int applying,
-		 int nr_holders, u32 budget,
-		 u32 avail_before, u32 freed_before,
-		 int committing, int exceeded),
-        TP_ARGS(sb, holding, applying, nr_holders, budget, avail_before, freed_before, committing, exceeded)
+        TP_PROTO(struct super_block *sb, int holding, int applying, int nr_holders,
+		 u32 avail_before, u32 freed_before, int committing, int exceeded),
+        TP_ARGS(sb, holding, applying, nr_holders, avail_before, freed_before, committing, exceeded)
 );
 
 #define slt_symbolic(mode)						\

kmod/src/server.c

@@ -65,7 +65,6 @@ struct commit_users {
 	struct list_head holding;
 	struct list_head applying;
 	unsigned int nr_holders;
-	u32 budget;
 	u32 avail_before;
 	u32 freed_before;
 	bool committing;
@@ -85,9 +84,8 @@ static void init_commit_users(struct commit_users *cusers)
 do {												\
 	__typeof__(cusers) _cusers = (cusers);							\
 	trace_scoutfs_server_commit_##which(sb, !list_empty(&_cusers->holding),			\
-		!list_empty(&_cusers->applying), _cusers->nr_holders, _cusers->budget,		\
-		_cusers->avail_before, _cusers->freed_before, _cusers->committing,		\
-		_cusers->exceeded);								\
+		!list_empty(&_cusers->applying), _cusers->nr_holders, _cusers->avail_before,	\
+		_cusers->freed_before, _cusers->committing, _cusers->exceeded);			\
 } while (0)
 
 struct server_info {
@@ -305,6 +303,7 @@ static void check_holder_budget(struct super_block *sb, struct server_info *serv
 	u32 freed_used;
 	u32 avail_now;
 	u32 freed_now;
+	u32 budget;
 
 	assert_spin_locked(&cusers->lock);
 
@@ -319,14 +318,15 @@ static void check_holder_budget(struct super_block *sb, struct server_info *serv
 	else
 		freed_used = SCOUTFS_ALLOC_LIST_MAX_BLOCKS - freed_now;
 
-	if (avail_used <= cusers->budget && freed_used <= cusers->budget)
+	budget = cusers->nr_holders * COMMIT_HOLD_ALLOC_BUDGET;
+	if (avail_used <= budget && freed_used <= budget)
 		return;
 
 	exceeded_once = true;
 	cusers->exceeded = cusers->nr_holders;
 
-	scoutfs_err(sb, "holders exceeded alloc budget %u av: bef %u now %u, fr: bef %u now %u",
-		    cusers->budget, cusers->avail_before, avail_now,
+	scoutfs_err(sb, "%u holders exceeded alloc budget av: bef %u now %u, fr: bef %u now %u",
+		    cusers->nr_holders, cusers->avail_before, avail_now,
 		    cusers->freed_before, freed_now);
 
 	list_for_each_entry(hold, &cusers->holding, entry) {
@@ -349,7 +349,7 @@ static bool hold_commit(struct super_block *sb, struct server_info *server,
 {
 	bool has_room;
 	bool held;
-	u32 new_budget;
+	u32 budget;
 	u32 av;
 	u32 fr;
 
@@ -367,8 +367,8 @@ static bool hold_commit(struct super_block *sb, struct server_info *server,
 	}
 
 	/* +2 for our additional hold and then for the final commit work the server does */
-	new_budget = max(cusers->budget, (cusers->nr_holders + 2) * COMMIT_HOLD_ALLOC_BUDGET);
-	has_room = av >= new_budget && fr >= new_budget;
+	budget = (cusers->nr_holders + 2) * COMMIT_HOLD_ALLOC_BUDGET;
+	has_room = av >= budget && fr >= budget;
 	/* checking applying so holders drain once an apply caller starts waiting */
 	held = !cusers->committing && has_room && list_empty(&cusers->applying);
 
@@ -388,7 +388,6 @@ static bool hold_commit(struct super_block *sb, struct server_info *server,
 		list_add_tail(&hold->entry, &cusers->holding);
 
 		cusers->nr_holders++;
-		cusers->budget = new_budget;
 
 	} else if (!has_room && cusers->nr_holders == 0 && !cusers->committing) {
 		cusers->committing = true;
@@ -517,7 +516,6 @@ static void commit_end(struct super_block *sb, struct commit_users *cusers, int
 	list_for_each_entry_safe(hold, tmp, &cusers->applying, entry)
 		list_del_init(&hold->entry);
 	cusers->committing = false;
-	cusers->budget = 0;
 	spin_unlock(&cusers->lock);
 
 	wake_up(&cusers->waitq);
@@ -2533,7 +2531,7 @@ static void server_log_merge_free_work(struct work_struct *work)
 
 		ret = scoutfs_btree_free_blocks(sb, &server->alloc,
 						&server->wri, &fr.key,
-						&fr.root, COMMIT_HOLD_ALLOC_BUDGET / 2);
+						&fr.root, COMMIT_HOLD_ALLOC_BUDGET / 8);
 		if (ret < 0) {
 			err_str = "freeing log btree";
 			break;
@@ -2552,7 +2550,7 @@ static void server_log_merge_free_work(struct work_struct *work)
 		/* freed blocks are in allocator, we *have* to update fr */
 		BUG_ON(ret < 0);
 
-		if (server_hold_alloc_used_since(sb, &hold) >= COMMIT_HOLD_ALLOC_BUDGET / 2) {
+		if (server_hold_alloc_used_since(sb, &hold) >= (COMMIT_HOLD_ALLOC_BUDGET * 3) / 4) {
 			mutex_unlock(&server->logs_mutex);
 			ret = server_apply_commit(sb, &hold, ret);
 			commit = false;