From 25e741d8c7b4986e5b7816f86819b8ea9eb3bc77 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 30 Jun 2026 08:22:15 +0000 Subject: [PATCH] =?UTF-8?q?fix(libxce):=20break=20ABBA=20iomgr=20deadlock?= =?UTF-8?q?=20=E2=80=94=20make=20endpoint=20busy=20flag=20lockless=20(bug-?= =?UTF-8?q?1282)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root-caused via gdb (all-thread backtrace on two independently-wedged controllers during chaotic failover): an ABBA lock-order deadlock in the async dispatch path. io_worker (netio.c): holds ep->lock, blocks on q->lock (vtask_enq -> dataq_enq) vdispatcher: dataq_deq holds q->lock, blocks on ep->lock via the vtask_eligible -> endpoint_is_task_blocked match callback endpoint_is_task_blocked took ep->lock while the caller (dataq_deq) held the dataq q->lock, inverting io_worker's ep->lock -> q->lock order. Under churn the two orders collide and the iomgr wedges (no more accepts / vote replies). Fix (mirrors the dispatch_inflight precedent, bug-1197): promote endpoint_t.busy from a 1-bit bitfield to _Atomic int and make endpoint_block_tasks / endpoint_unblock_tasks / endpoint_is_task_blocked lockless (release/acquire). The ep->lock that wrapped each access protected nothing else (busy only gates the vtask queue), so removing it is safe; the inherent set-then-check TOCTOU is unchanged (the lock never prevented it — the decision is advisory). Verified: post-fix gdb soak shows ZERO threads in __lll_lock_wait and endpoint_is_task_blocked no longer appears in any lock path (deadlock gone); controller_unit_test all green (no regression). NOTE: a SEPARATE release-only restart segfault remains and still blocks the raft-ha-core-safety 7.2 gate (tracked separately) — deadlocks hang, they do not SIGSEGV, so it is a distinct memory bug. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_019dbTHR9CsEYK9RsqmQpQpC --- src/include/netio.h | 9 ++++++++- src/lib/netio.c | 23 ++++++++++++----------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/include/netio.h b/src/include/netio.h index 1118747..ee0dbc9 100644 --- a/src/include/netio.h +++ b/src/include/netio.h @@ -72,7 +72,14 @@ typedef struct endpoint_t { int fd; int idx; int flags; - unsigned int busy : 1; + /* bug-1282: `busy` is read by endpoint_is_task_blocked from the vtask- + * eligibility match callback (vtask_eligible), which runs UNDER the dataq + * q->lock. Taking ep->lock there inverted io_worker's ep->lock -> q->lock + * order (io_worker holds ep->lock and enqueues via dataq_enq), an ABBA + * deadlock that wedged the iomgr under chaotic failover churn. Promoted from + * a bitfield to a full _Atomic int (bitfields cannot be _Atomic — same reason + * as dispatch_inflight) so the eligibility check reads it locklessly. */ + _Atomic int busy; unsigned int tearing_down : 1; unsigned int ssl_handshake_done : 1; /* bug-1197: count of peer-control dispatch attempts currently between the diff --git a/src/lib/netio.c b/src/lib/netio.c index c756ca0..54aa1f1 100644 --- a/src/lib/netio.c +++ b/src/lib/netio.c @@ -981,30 +981,31 @@ uint64_t endpoint_getseq(endpoint_t* ep) return seq; } +/* bug-1282: `busy` is a self-contained eligibility flag — the ep->lock that used + * to wrap each access protected nothing else (it only gates the vtask queue). + * Reading it under ep->lock from the vtask-eligibility path + * (endpoint_is_task_blocked, called inside dataq_deq while holding the dataq + * q->lock) inverted io_worker's ep->lock -> q->lock order and deadlocked the + * iomgr (ABBA). `busy` is now an _Atomic int, so all three sites use lockless + * release/acquire and never touch ep->lock. The inherent set-then-check TOCTOU + * (busy may change right after the read) is unchanged — the lock never prevented + * it; the eligibility decision was always advisory. */ void endpoint_block_tasks(endpoint_t* ep) { TRACE("%s[%ld] block tasks for endpoint %p", __func__, pthread_self(), ep); - mylock_lock(&(ep->lock)); - ep->busy = 1; - mylock_unlock(&(ep->lock)); + atomic_store_explicit(&ep->busy, 1, memory_order_release); } void endpoint_unblock_tasks(endpoint_t* ep) { TRACE("%s[%ld] unblock tasks for endpoint %p", __func__, pthread_self(), ep); - mylock_lock(&(ep->lock)); - ep->busy = 0; - mylock_unlock(&(ep->lock)); + atomic_store_explicit(&ep->busy, 0, memory_order_release); } int endpoint_is_task_blocked(endpoint_t* ep) { - int busy; - - mylock_lock(&(ep->lock)); - busy = ep->busy; - mylock_unlock(&(ep->lock)); + int busy = atomic_load_explicit(&ep->busy, memory_order_acquire); TRACE("%s[%ld] endpoint %p task %s", __func__, pthread_self(), ep, (busy != 0) ? "blocked" : "not blocked");