mirror of
https://github.com/rustfs/rustfs.git
synced 2026-08-28 07:57:01 +00:00
fix(lock): let waiters hear releases and let acquisition succeed past registered waiters (#5670)
* fix(lock): let waiters hear releases and let acquisition succeed past registered waiters Same-key write contention scaled superlinearly with writer count: 8 concurrent conditional PUTs on one key cost ~340-460 ms, 16 cost ~700 ms, 32 cost ~5 s, against ~4 ms per uncontended write and ~10 ms actual lock holds (measured via RUSTFS_OBJECT_LOCK_DIAG at 1 ms thresholds). Outcomes were always correct; the cost was pure waiting. Two coupled defects in fast_lock caused it: 1. The slow path's early retries slept without subscribing to anything. notify_writer()/notify_readers() are gated on the waiter counters, which a sleeper never increments, so a release during the backoff woke nobody. The lock sat free while every loser slept out its full backoff, and the ladder compounded: successive acquires landed at the cumulative ladder offsets (10+20+40+80+100... ms). 2. try_acquire_exclusive demanded the entire packed state word be zero, including the readers_waiting/writers_waiting counter bits. A lock with registered waiters could be acquired by no one - including the waiters themselves, each blocked by the others' registration - so contended acquisition only succeeded in windows where every waiter happened to be unregistered. This is also why (1) could not be fixed by simply registering the sleepers: registration alone deadlocks acquisition until the acquire deadline. try_acquire_shared already masks correctly and preserves the counter bits in its CAS; the exclusive path now mirrors it. The fix: mask the acquisition CAS to ownership bits only (writer flag, active readers), and turn the early-retry sleep into a notification wait bounded by the same backoff, so a release wakes a waiter immediately while the bound still protects against lost or stolen wakeups exactly as NOTIFY_WAIT_CAP does for the post-retry wait. With both changes, 8 concurrent same-key CAS writers resolve in 17-29 ms (was 340-460 ms) and 32 resolve in 20-53 ms (was ~5 s), with per-racer cost now decreasing in N. Outcomes remain exactly one winner, N-1 precondition failures, zero errors at every width. cargo test -p rustfs-lock passes 113/113 at pristine-parity runtime, including test_concurrent_write_lock_contention, which previously only passed because sleepers were invisible to it. * test(lock): pin both halves of the waiter-starvation fix The fix commit touched only production files, so reverting either half left the suite green: test_concurrent_write_lock_contention only waits for five writers to finish and never asserts that acquisition happens before the backoff ladder runs out. Three tests, one per revert: * exclusive_acquisition_ignores_registered_waiters (state.rs) - a free lock with registered waiters must be acquirable, and the CAS must preserve the counters. Fails against the all-zero `expected`. * early_retry_registers_as_waiter (shard.rs) - a waiter in the early-retry backoff must appear in the writer waiter count within the ~750ms early-retry phase, since notify_writer/notify_readers are gated on those counters. Fails against a bare `sleep`, which registers nowhere. * contended_writers_drain_promptly_after_release (tests.rs) - 16 same-key writers, all registered behind one holder, must drain within 1s of the release rather than sit out their 5s acquire deadlines. Fails against the all-zero `expected` end to end. Wakeup latency is deliberately not asserted anywhere. NOTIFY_POOL is a process-global of 128 Notify slots shared by every lock, so a waiter in a concurrently-running test can consume another's notify_one and push it to the end of its rung: a 24-key latency probe measured ~150us in isolation and ~92ms - a full unexpired rung - alongside the existing 64-key missed-wakeup test. That is the stolen wakeup NOTIFY_WAIT_CAP already exists to bound, and it makes any in-suite latency budget flaky. cargo test -p rustfs-lock: 116/116. Signed-off-by: Miguel Amador <miguel@amador.one> --------- Signed-off-by: Miguel Amador <miguel@amador.one>
This commit is contained in:
@@ -18,7 +18,7 @@ mod fast_lock_tests {
|
||||
use crate::fast_lock::types::{LockConfig, LockMode, LockPriority, LockResult, ObjectKey, ObjectLockRequest};
|
||||
use crate::fast_lock::{DEFAULT_SHARD_COUNT, FastObjectLockManager};
|
||||
use std::sync::Arc;
|
||||
use std::time::Duration;
|
||||
use std::time::{Duration, Instant};
|
||||
use tokio::time::sleep;
|
||||
|
||||
/// Helper function to create a test lock manager
|
||||
@@ -470,6 +470,67 @@ mod fast_lock_tests {
|
||||
}
|
||||
}
|
||||
|
||||
// Regression for the waiter-preserving exclusive CAS, end to end
|
||||
// (rustfs#5657 same-key write contention).
|
||||
//
|
||||
// `try_acquire_exclusive` used to demand a fully-zero state word, which
|
||||
// includes the waiting counters. Once the slow path's retries register as
|
||||
// waiters — as they must, to hear a release — a lock with waiters becomes
|
||||
// acquirable by no one, each waiter blocked by the others' registration, so
|
||||
// every waiter here sits out its full acquire deadline instead of draining.
|
||||
//
|
||||
// The holder is held long enough for all waiters to be registered before
|
||||
// the single release. After it they only serialize on each other, holding
|
||||
// nothing, so they should drain in tens of milliseconds.
|
||||
#[tokio::test(flavor = "multi_thread")]
|
||||
async fn contended_writers_drain_promptly_after_release() {
|
||||
let manager = Arc::new(create_test_manager());
|
||||
let key = ObjectKey::new("bucket", "hot-object");
|
||||
const WAITERS: usize = 16;
|
||||
const HOLD: Duration = Duration::from_millis(300);
|
||||
// Generous next to the ~20ms the fixed path needs for these waiters, and
|
||||
// far below the 5s acquire deadline a zero-state CAS makes them all
|
||||
// sit out.
|
||||
const DRAIN_BUDGET: Duration = Duration::from_millis(1000);
|
||||
|
||||
let mut holder = manager
|
||||
.acquire_write_lock(key.clone(), "holder")
|
||||
.await
|
||||
.expect("holder should acquire immediately");
|
||||
|
||||
let mut handles = Vec::new();
|
||||
for i in 0..WAITERS {
|
||||
let manager = manager.clone();
|
||||
let key = key.clone();
|
||||
handles.push(tokio::spawn(async move {
|
||||
let request =
|
||||
ObjectLockRequest::new_write(key, format!("waiter-{i}")).with_acquire_timeout(Duration::from_secs(5));
|
||||
let mut guard = manager
|
||||
.acquire_lock(request)
|
||||
.await
|
||||
.expect("every waiter must acquire once the holder releases");
|
||||
assert!(guard.release());
|
||||
}));
|
||||
}
|
||||
|
||||
// Let every waiter fail its fast path and register in the retry ladder.
|
||||
sleep(HOLD).await;
|
||||
|
||||
let released_at = Instant::now();
|
||||
assert!(holder.release());
|
||||
|
||||
for handle in handles {
|
||||
handle.await.expect("waiter task should not panic");
|
||||
}
|
||||
|
||||
let drain = released_at.elapsed();
|
||||
assert!(
|
||||
drain < DRAIN_BUDGET,
|
||||
"{WAITERS} waiters took {drain:?} to drain after the release (budget {DRAIN_BUDGET:?}) - \
|
||||
registered waiters are blocking acquisition"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_lock_timeout() {
|
||||
let manager = create_test_manager();
|
||||
|
||||
Reference in New Issue
Block a user