jojochuang commented on PR #10995: URL: https://github.com/apache/ozone/pull/10995#issuecomment-5403016176
## Deadlock review Reviewed for deadlock scenarios with Ratis callback threading in mind. **The fix looks correct** — I don't see a remaining lock cycle after separating `termIndexLock` from the state-machine monitor. ### Root cause (confirmed) Before this patch, lifecycle (`pause`/`unpause`/`reinitialize`) and term/index updates both used the same monitor (`synchronized` instance methods on `OzoneManagerStateMachine`): - `pause()` holds SM monitor → `OzoneManagerDoubleBuffer.stop()` → `daemon.join()` waits for flush thread - Flush thread → `updateLastAppliedTermIndex()` → waits for the same SM monitor The new test `testPauseWhileDoubleBufferUpdatesLastAppliedIndex` correctly reproduces this. ### Why the fix is sound | Lock | Protects | |------|----------| | **SM monitor (`this`)** | `pause()`, `unpause()`, `reinitialize()`, `takeSnapshotImpl()` | | **`termIndexLock`** | `notifyTermIndexUpdated()`, `updateLastAppliedTermIndex()`, `restoreLastAppliedTermIndex()` | Lock order is **SM monitor → `termIndexLock`**. No path acquires `termIndexLock` first and then the SM monitor. Removing `synchronized` from the term/index methods is necessary — keeping those modifiers would still acquire the lifecycle monitor. ### Ratis threading (relevant callbacks) - **StateMachineUpdater**: `applyLog()` → `notifyTermIndexUpdated()` (metadata/config entries) and `applyTransaction()` (state-machine entries) - **InstallSnapshot RPC thread**: `notifyInstallSnapshotFromLeader()` → OM install, then Ratis `pause()` + `reloadStateMachine()` → `reinitialize()` - After the fix, `notifyTermIndexUpdated()` only contends on `termIndexLock`, not the SM monitor held by `pause()` — no cycle. ### Post-fix scenarios checked 1. **`notifyTermIndexUpdated` during `pause()`** — safe (different lock). 2. **`takeSnapshotImpl()` vs `pause()`** — intentional serialization (snapshot must finish before pause proceeds); not a cycle since flush no longer needs SM monitor. 3. **`reinitialize()` nested `unpause()`** — same-thread SM reentrancy; nested `termIndexLock` follows declared order. 4. **`OzoneManager` synchronized + SM locks** — no cycle found; flush callback doesn't take OM monitor. ### Remaining risks (not classic deadlocks) - **`takeSnapshot()` spin loop** reads `lastSkippedIndex` / applied index without `termIndexLock` — possible extra spins, not a lock cycle. - **Window between OM `pause()` and Ratis `pause()`**: Ratis `StateMachineUpdater` may still be `RUNNING` and apply entries while flush thread is stopped — pre-existing correctness concern, not introduced by this PR. - **`pause()` doesn't drain apply executor** — in-flight apply work can still enqueue after flush stops; pre-existing. - **Failed install after `pause()` without `unpause()`** — stuck PAUSED state, not circular wait. ### Verdict Approve from a deadlock perspective. The HDDS-16155 fix is targeted and the lock ordering is consistent. Optional follow-ups (non-blocking): stress test `notifyTermIndexUpdated` + `pause` concurrency; document that `pause()` intentionally doesn't stop the apply executor. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
