Bound MaintainPoolSize loops by the LRU list and abort on corrupt pool accounting - #15150
Open
yxshwanth wants to merge 3 commits into
Open
Bound MaintainPoolSize loops by the LRU list and abort on corrupt pool accounting#15150yxshwanth wants to merge 3 commits into
yxshwanth wants to merge 3 commits into
Conversation
Both loops in LRUCacheShard::MaintainPoolSize() terminated only on a size_t usage counter while walking a circular list. The invariant tying those counters to the list is checked only by assert, which is compiled out in release builds. A counter that disagreed with the list therefore sent the walk past its own region and around the list, subtracting charges that were never counted into that pool. The counter underflowed, and since low_pri_pool_ratio defaults to 0 the capacity is 0, so the loop condition stayed true permanently. The loop holds mutex_ throughout, so every DB sharing the cache wedges behind it. Bound each loop by the list it walks. If a loop runs out of entries while its counter still claims the pool is over capacity, that counter no longer describes the shard, so report the accounting state and abort rather than hang or continue on corrupt state. Aborting raises SIGABRT and leaves a core dump, unlike port::Crash() which sends SIGTERM. Neither bound is reachable while the counters agree with the list, so consistent shards behave exactly as before. Fixes facebook#15128
Puts a shard's low-pri counter out of sync with its LRU list and checks that MaintainPoolSize() reports the accounting state and aborts, rather than spinning with mutex_ held. Note the test only exercises the guard indirectly in debug builds: without the list bounds the walk trips assert(lru_bottom_pri_->InLowPriPool()) long before the counter wraps. The spin, and so the guard that prevents it, is specific to release builds.
ValidateLRUList checked list order and per-entry flags but never the usage counters, which are what MaintainPoolSize() loops on and what wedges a shard when they drift from the list. Adds a randomized stress over the shard's public API that recomputes both pool usages from the list and compares them against the counters after every operation, along with marker ordering and per-region flags. The test asserts on the average LRU list length. An earlier version of it passed while averaging 0.3 entries per list, which exercised almost none of the pool logic; the assertion keeps it from silently going vacuous again. Set LRU_STRESS_SEEDS to raise the iteration count beyond the default.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #15128, where an LRU block cache shared across ~128 DBs wedged every one of them because a thread was spinning in
LRUCacheShard::MaintainPoolSize()holding the shard mutex.Both loops there terminated only on a
size_tusage counter while walking a circular list. The invariant tying those counters to the list is checked only byassert, which is compiled out in release. So a counter that disagreed with the list sent the walk past its own region and around the list, subtracting charges never counted into that pool. The counter underflowed, and sincelow_pri_pool_ratiodefaults to 0 the capacity is 0, leavingusage > capacitytrue forever withmutex_held.This bounds each loop by the list it walks. The low-pri loop takes two bounds: the pool it drains is the entries in
(lru_bottom_pri_, lru_low_pri_], and separately it stops at the end of the list, which matters once the markers are out of order as they were in the report. Without the second one the walk runs off the end and reads the sentinel's uninitializedtotal_charge. If a loop runs out of entries while its counter still claims the pool is over capacity, that counter no longer describes the shard, so it prints the accounting state and aborts. @xingbowang asked for a crash with a core dump rather than silent recovery, which is what this does.port::Crash()is not usable for it because SIGTERM does not dump core, so it callsstd::abort().Rather than detect a loop that has run too long, it detects the state that makes the loop unable to end. That fires on the first bad iteration and has nothing to tune.
I could not find what makes the counters diverge in the first place. 40M randomized operations against a shard, checking both counters against the list after each one, produced no divergence, and I found no unsynchronized path:
LRUHandleappears only undercache/, every publicLRUCacheShardmethod takesmutex_, andNotifyEvictedfrees each handle once after unlinking. So this guards the amplification, not the trigger. It turns a permanent process-wide hang into a crash with the evidence attached.Test plan
make checkpasses, other than failures that reproduce on unmodified HEAD (see below).A standalone program against a release
librocksdb.a, differing only in the loop bounds, corruptslow_pri_pool_usage_and callsMaintainPoolSize(). Unbounded it runs at 99.4% CPU until killed, with all 1699 stack samples inMaintainPoolSize(). Bounded:Before trusting an abort in production code I ran 40M randomized operations with it in place, across every ratio combination, both metadata charge policies, and strict and non-strict. No spurious abort. None of the bounds are reachable while the counters agree with the list, so consistent shards are unaffected.
Two smaller things worth flagging:
ValidateLRUListchecked list order and per-entry flags but never the usage counters, which are what these loops run on. The third commit adds a stress test that checks them after every operation. It also asserts on average LRU list length, because an earlier version passed while averaging 0.3 entries per list and exercised almost none of the pool logic.The death test only exercises the guard indirectly in debug builds: without the bounds the walk trips
assert(lru_bottom_pri_->InLowPriPool())long before the counter wraps. The spin is specific to release builds, and RocksDB's test infrastructure cannot build in release at all, sinceSyncPointcompiles out under NDEBUG anddb_test_util.ccfails. That is why the release evidence above needed a standalone harness, and it may be worth a separate look: the checks for this class of bug run only in builds where the bug cannot occur.On macOS,
make checkalso fails twoDBSSTTestcases, threeEventListenerTestcases, androcksdb_dump_test, all of which reproduce on unmodified HEAD.$TMPDIRends in/there, soTEST_TMPDIRcontains a doubled slash andSstFileManagertracks each file under two spellings; the same tests pass with aTEST_TMPDIRthat has no trailing slash. Unrelated to this change, happy to file separately.