fix(controller): offload checkpoint_task_fn off the delaymgr thread (bug-1694)
Last of the 9 confirmed findings from the /xce-audit-invariants controller pass. checkpoint_task_fn was registered directly as a delaymgr entry and ran its full blocking body inline: checkpoint marker pwrite+fdatasync, write_snapshot()'s genuinely unbounded per-entity walk (scales with cluster state size) held under apply_pump_lock, WAL trim, and snapshot rotation/unlink -- all on the same thread that drives heartbeats/election timeouts. Unlike every other delaymgr operation (snapshot send, config-push, reconnect, catch-up), checkpoint had no offload mechanism at all -- the one gap left from the raft-drive-async hardening pass. Design: full offload via the SlowLane (slowlane_submit), default ON -- NOT the vtask pool. persist_apply_pump_run() already holds apply_pump_lock across its own per-entry walk while running ON the vtask pool (VTASK_LIVE_BLOCK); putting checkpoint's unbounded, lock-held write_snapshot() on that same pool risked starving it, exactly what ha-async-io.md's own thread-role table warns against. The SlowLane is the actual established pattern for this I/O class (push_config_transport_only, the snapshot-send path, catchup_lane_fn, demotion_lane_fn all dispatch there) -- and persist_checkpoint() already runs on the slow lane in production TODAY via ha_snapshot_do_send() -> snapshot_lane_fn, so apply_pump_lock's acquisition from a slow-lane thread is proven-safe, exercised daily, not new surface. Unlike catchup/demotion (which stay flag-gated OFF for an unrelated, still-not-root-caused hang, bug-1429), checkpoint's new flag defaults ON given this reuses an already-proven code path. Inline-fallback + WARN only on a genuine slowlane_submit failure (not -EAGAIN/-EEXIST, both non-failures) -- never silently drops a scheduled checkpoint. New test_persist_checkpoint_offload.c (3 tests). RED-phase forced the pre-fix always-inline path: the suite never printed a single result and had to be killed, proving the test genuinely detects the violation, not a tautology. GREEN: all 3 pass. Full suite: 1184 tests, 2 failures -- both the pre-existing, unrelated bug-1561 overlayfs sandbox flake (exact signature match). No live HA soak was run for this specific fix in isolation (explicitly scoped out, not hidden) -- the mandatory live-soak proof for this whole audit campaign covers it next. This closes the last of the 9 confirmed findings from the controller audit (bug-1684, bug-1687/1689/1690/1691/1692, bug-1693, bug-1694). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
This commit is contained in:
parent
d1803376d8
commit
b4eb9ec821
6 changed files with 561 additions and 8 deletions
|
|
@ -3182,12 +3182,162 @@ static void rotate_snapshots(persist_t* p)
|
|||
/* Async checkpoint scheduling (schedule_checkpoint + task fn) */
|
||||
/* ------------------------------------------------------------------ */
|
||||
|
||||
/* checkpoint_task_fn: executed by delaymgr worker thread.
|
||||
#ifdef XCE_TESTING
|
||||
/* Test-injectable substitute for the real (blocking) checkpoint work,
|
||||
* mirroring ha.c's ha_repl_send_snapshot_hook idiom exactly: when non-NULL,
|
||||
* checkpoint_lane_fn calls this INSTEAD OF persist_checkpoint() — lets a
|
||||
* test park/gate the lane worker without needing a full disk-backed
|
||||
* checkpoint to actually complete. NULL in every fixture that never installs
|
||||
* it (the overwhelming common case): checkpoint_lane_fn falls through to the
|
||||
* real call. Symbol always compiled so test TUs can `extern` it; unused in
|
||||
* a non-XCE_TESTING (production) build. */
|
||||
void (*persist_checkpoint_test_hook)(persist_t*) = NULL;
|
||||
#endif
|
||||
|
||||
/* g_persist_checkpoint_offload: checkpoint slow-lane offload feature flag
|
||||
* (xce-audit-invariants controller pass, dimension 1 async-I/O thread-role
|
||||
* fix, bug-1694).
|
||||
*
|
||||
* checkpoint_task_fn (below) was the last delaymgr-triggered path in this
|
||||
* codebase that still ran its full blocking body directly on the delaymgr
|
||||
* timer thread -- the SAME single thread that drives heartbeats and election
|
||||
* timeouts cluster-wide (ha-async-io.md thread-role contract: delaymgr must
|
||||
* only trigger work, never block). The blocking body is real: a
|
||||
* WAL_CHECKPOINT marker pwrite+fdatasync, write_snapshot()'s unbounded
|
||||
* open/write walk over every job/queue/host frame (scales with cluster state
|
||||
* size, held under apply_pump_lock), then trim_wal_segments + rotate_
|
||||
* snapshots' unlink. Every sibling long-blocking-I/O path (snapshot send,
|
||||
* config-push, reconnect) was already moved off its liveness thread by
|
||||
* raft-drive-async -- this was the one documented remaining gap.
|
||||
*
|
||||
* DEFAULT ON (1), unlike the still-experimental catchup/demotion offloads
|
||||
* (g_drive_async_catchup_offload / g_drive_async_demotion_offload in
|
||||
* config_repl.c / ha_convergence.c, both default OFF pending an unrelated,
|
||||
* real test-fixture hang -- see their own comments and bug-1429): this
|
||||
* reuses an ALREADY-PROVEN-SAFE call, not new surface. persist_checkpoint()
|
||||
* already runs on this SAME slow lane in production TODAY via
|
||||
* ha_snapshot_do_send() -> snapshot_lane_fn (g_drive_async_snapshot_offload,
|
||||
* ha.c, also default ON) -- every snapshot-send cycle already calls
|
||||
* persist_checkpoint() from a slow-lane worker thread, so apply_pump_lock's
|
||||
* acquisition inside persist_checkpoint() from a slow-lane thread is
|
||||
* exercised in production every day, not a new code path. persist_
|
||||
* checkpoint()'s own atomic_exchange guard (checkpoint_in_progress) is the
|
||||
* existing, already-relied-upon single-flight barrier that keeps this
|
||||
* offload and the snapshot-send path from ever double-running a checkpoint
|
||||
* concurrently (see g_drive_async_snapshot_offload's own comment: "at most
|
||||
* one persist_checkpoint in flight (existing atomic guard)").
|
||||
*
|
||||
* Destination is the SLOW LANE (slowlane_submit), deliberately NOT the
|
||||
* vtaskmgr N-thread pool: write_snapshot()'s per-entity walk is genuinely
|
||||
* unbounded, matching ha-async-io.md's own table entry ("vtaskmgr must NOT
|
||||
* do: long/unbounded blocking I/O (snapshot transfer, config-push) --
|
||||
* starves the pool"). The vtask pool was investigated and ruled out
|
||||
* specifically because persist_apply_pump_run() -- which ALSO acquires
|
||||
* apply_pump_lock, across its entire per-entry walk -- is itself dispatched
|
||||
* onto that SAME pool via persist_apply_pump_kick()'s
|
||||
* vtask_enq_classified(pool, task, VTASK_LIVE_BLOCK). Putting checkpoint's
|
||||
* unbounded, lock-held I/O on the identical N-thread pool would risk a
|
||||
* worker thread parked on apply_pump_lock for the whole snapshot-write
|
||||
* duration, starving the pool's liveness-critical capacity (commit
|
||||
* continuations, apply-pump drains) -- exactly the failure mode the
|
||||
* codebase's own slow-lane/vtask-pool split already exists to prevent. The
|
||||
* slow lane's dedicated, separate worker threads carry no such contention
|
||||
* with the vtask pool's own capacity, and this checkpoint offload does not
|
||||
* introduce any NEW lock-order edge: it is the same apply_pump_lock
|
||||
* acquisition the snapshot-send path already makes from a slow-lane thread.
|
||||
*
|
||||
* No synchronous-wait hazard (the actual mechanism behind the catchup/
|
||||
* demotion offloads staying OFF -- an uninitialized slow lane silently
|
||||
* swallows a submitted task with zero live workers to ever run it, and
|
||||
* something ELSE downstream then hangs forever waiting for its completion):
|
||||
* checkpoint's trigger (schedule_checkpoint, fired from the WAL-append hot
|
||||
* path when wal_record_count crosses max_wal_records) is pure
|
||||
* fire-and-forget -- nothing waits synchronously for a checkpoint to finish,
|
||||
* so a silently-swallowed submission degrades to "this checkpoint cycle is
|
||||
* skipped, retried on the next WAL threshold crossing" (a liveness/WAL-
|
||||
* growth concern in a hypothetical mis-configured fixture, never a hang).
|
||||
* Verified no existing controller_unit_test fixture depends on this trigger
|
||||
* path completing: test_ha_core_safety.c's live-cluster HA fixtures
|
||||
* deliberately pin max_wal_records to 1u<<30 specifically "so no checkpoint
|
||||
* is [triggered]" / "never auto-checkpoint during a unit test"; every other
|
||||
* max_wal_records reference in the suite is a synthetic/no-cluster check
|
||||
* that never reaches schedule_checkpoint's cluster branch at all. Production
|
||||
* always calls slowlane_init via cluster_init_phase2 before the delaymgr
|
||||
* loop can fire this trigger.
|
||||
*
|
||||
* OFF -> legacy inline blocking path on the delaymgr thread; retained only
|
||||
* so the TDD RED test can demonstrate the pre-fix stall. Compiled in ALL
|
||||
* builds; the setter is the test seam to flip RED<->GREEN. */
|
||||
static _Atomic int g_persist_checkpoint_offload = 1;
|
||||
|
||||
void persist_set_checkpoint_offload_for_test(int on)
|
||||
{
|
||||
atomic_store_explicit(&g_persist_checkpoint_offload, on ? 1 : 0,
|
||||
memory_order_release);
|
||||
}
|
||||
|
||||
int persist_checkpoint_offload_enabled(void)
|
||||
{
|
||||
return atomic_load_explicit(&g_persist_checkpoint_offload,
|
||||
memory_order_acquire);
|
||||
}
|
||||
|
||||
/* run_checkpoint_body: the actual (blocking) checkpoint work -- persist_
|
||||
* checkpoint() itself is UNCHANGED. Shared by every call site that can end
|
||||
* up running it (the slow-lane worker AND checkpoint_task_fn's flag-OFF /
|
||||
* no-cluster / submit-failure inline-fallback branches) so the XCE_TESTING
|
||||
* hook seam (persist_checkpoint_test_hook) covers all of them identically --
|
||||
* a test can park/observe the checkpoint body regardless of which path
|
||||
* dispatched it, which is exactly what proves (or, pre-fix, disproves) the
|
||||
* delaymgr thread never runs it directly. Resets wal_record_count AFTER the
|
||||
* work actually ran (unconditionally, success or failure alike -- matches
|
||||
* the pre-fix ordering exactly: the original inline checkpoint_task_fn also
|
||||
* reset it unconditionally right after persist_checkpoint() returned). */
|
||||
static void run_checkpoint_body(persist_t *persist)
|
||||
{
|
||||
#ifdef XCE_TESTING
|
||||
if (persist_checkpoint_test_hook != NULL) {
|
||||
persist_checkpoint_test_hook(persist);
|
||||
atomic_store(&persist->wal_record_count, 0);
|
||||
return;
|
||||
}
|
||||
#endif
|
||||
|
||||
persist_checkpoint(persist);
|
||||
atomic_store(&persist->wal_record_count, 0);
|
||||
}
|
||||
|
||||
/* checkpoint_lane_fn: slow-lane worker entry (offload flag ON). Runs
|
||||
* run_checkpoint_body() off the delaymgr thread. Mirrors snapshot_lane_fn /
|
||||
* configpush_lane_fn / catchup_lane_fn / demotion_lane_fn exactly. */
|
||||
static void checkpoint_lane_fn(void *arg)
|
||||
{
|
||||
persist_t *persist = (persist_t *)arg;
|
||||
if (!persist)
|
||||
return;
|
||||
|
||||
run_checkpoint_body(persist);
|
||||
}
|
||||
|
||||
/* checkpoint_task_fn: executed by delaymgr worker thread -- TRIGGER ONLY
|
||||
* (ha-async-io.md thread-role contract: delaymgr fires timers and triggers
|
||||
* work, then returns fast -- it never blocks).
|
||||
*
|
||||
* The CAS in schedule_checkpoint already holds the "in_progress" slot (=1).
|
||||
* We must clear it before calling persist_checkpoint(), which has its own
|
||||
* atomic_exchange guard. After persist_checkpoint() returns (it resets the
|
||||
* flag to 0), we also reset wal_record_count so the counter starts fresh. */
|
||||
* Release it so persist_checkpoint's own guard (inside checkpoint_lane_fn,
|
||||
* now running off-thread) can acquire it, then hand the real (blocking)
|
||||
* checkpoint work to the slow lane -- see g_persist_checkpoint_offload above
|
||||
* for the flag + full design rationale.
|
||||
*
|
||||
* Inline-fallback + WARN on slowlane_submit failure (mirrors every sibling
|
||||
* offload's own rule): a scheduled checkpoint must never be silently
|
||||
* dropped -- that risks unbounded WAL growth. -EEXIST (a checkpoint already
|
||||
* queued/running under this same lane key) and -EAGAIN (the lane's
|
||||
* admission cap is momentarily full) are NOT failures -- in both cases the
|
||||
* work is already going to run (slowlane_submit's own documented contract:
|
||||
* "In all of {0, -EAGAIN} the work is enqueued and will run exactly once";
|
||||
* -EEXIST means it is already enqueued/running under this key). Only a
|
||||
* genuine submit failure (alloc / lane stopping) falls back inline. */
|
||||
static int checkpoint_task_fn(void* arg)
|
||||
{
|
||||
persist_t* persist = (persist_t*)arg;
|
||||
|
|
@ -3195,14 +3345,39 @@ static int checkpoint_task_fn(void* arg)
|
|||
/* Release the "scheduled" slot so persist_checkpoint's guard can acquire */
|
||||
atomic_store(&persist->checkpoint_in_progress, 0);
|
||||
|
||||
persist_checkpoint(persist);
|
||||
if (!persist_checkpoint_offload_enabled() || persist->cluster == NULL) {
|
||||
/* Legacy inline path -- blocks the calling (delaymgr) thread.
|
||||
* Retained solely so the RED test can show the pre-change stall;
|
||||
* production runs with the flag ON. */
|
||||
run_checkpoint_body(persist);
|
||||
return 0;
|
||||
}
|
||||
|
||||
/* Reset the record counter so the next checkpoint window starts clean.
|
||||
* Done after persist_checkpoint returns — the flag is already 0 by now. */
|
||||
atomic_store(&persist->wal_record_count, 0);
|
||||
int srv = slowlane_submit(&persist->cluster->slowlane, /*key=*/1,
|
||||
checkpoint_lane_fn, persist);
|
||||
if (srv < 0 && srv != -EAGAIN && srv != -EEXIST) {
|
||||
WARN("persist: checkpoint slowlane_submit failed (rc=%d) -- running "
|
||||
"inline (never silently drop a scheduled checkpoint -- risk of "
|
||||
"unbounded WAL growth)", srv);
|
||||
run_checkpoint_body(persist);
|
||||
}
|
||||
/* 0 or -EAGAIN: enqueued exactly once; checkpoint_lane_fn resets
|
||||
* wal_record_count when it actually runs. -EEXIST: a checkpoint is
|
||||
* already queued/running -- nothing further to do here. */
|
||||
return 0;
|
||||
}
|
||||
|
||||
#ifdef XCE_TESTING
|
||||
/* Test seam: directly drive checkpoint_task_fn (production's only call site
|
||||
* is via delay_task, which a test would otherwise have to wait out — the
|
||||
* real delaymgr fires it 1s after schedule_checkpoint's delay_task call).
|
||||
* Returns checkpoint_task_fn's own return value (always 0). */
|
||||
int persist_checkpoint_task_fn_for_test(persist_t *p)
|
||||
{
|
||||
return checkpoint_task_fn(p);
|
||||
}
|
||||
#endif
|
||||
|
||||
/* schedule_checkpoint: idempotent async checkpoint request via delaymgr.
|
||||
* Safe to call from any thread. Uses atomic CAS to ensure at most one
|
||||
* checkpoint is scheduled or running at a time.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue