Bound cache directory bucket walks in all builds - #13608
Conversation
The loop protection in probe/overwrite/remove/dir_clean_bucket sat behind LOOP_CHECK_MODE, which is commented out and not settable from CMake, so production builds walked bucket chains unbounded. A cycle in dir_next spins an event thread while it holds the stripe mutex. Directory::max_bucket_depth() is the bound, (DIR_DEPTH - 1) * buckets + 1: init_segment() frees rows 1..DIR_DEPTH-1 of every bucket onto the free list and never row 0, so one chain holds at most every free entry of the segment plus its own head. Exceeding it proves a cycle, so no exact check is needed at the trigger. bucket_length() capped at a hardcoded 100 instead, below the longest legitimate chain, so a chain over that would make check_segment() report corruption and Stripe::_shm_directory_is_valid() reject a healthy shared memory attach. Readers report and move on, writers repair. probe() throttles its report because it does not repair, so the loop lives until a writer clears it and every lookup that hashes to the bucket lands on the same trigger. insert()'s tail walk is the only chain walk it makes, so that is where a writer meets a cycle, and stopping at the cap linked the new entry into the cycle. Stripe::dir_check() had no cap at all and wrote chain_tag[] once per step, so a cycle ran off the end of a 65536-entry stack array. It now stops at the bound, and at the first revisited entry rather than printing a cycle line per step. check_bucket_not_contains(), an unbounded walk with no caller since the 2009 import, is deleted.
There was a problem hiding this comment.
Pull request overview
This PR makes cache directory bucket-chain walks bounded in all builds to prevent event-thread hangs (and potential stack overruns) when directory corruption introduces cycles in dir_next chains.
Changes:
- Introduces
Directory::max_bucket_depth()as a derived, segment-aware upper bound and uses it to cap/trigger handling for bucket chain walks. - Removes the dead
LOOP_CHECK_MODEgating and replaces the prior hardcoded bucket-length cap with the derived bound. - Updates
Stripe::dir_check()and cache dir unit tests to validate bounded traversal and repair behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/iocore/cache/CacheDir.cc | Replaces unbounded/compile-time-gated bucket walks with max_bucket_depth()-bounded logic and adjusts loop handling across probe/insert/overwrite/remove/clean paths. |
| src/iocore/cache/P_CacheDir.h | Adds Directory::max_bucket_depth() API and documentation explaining the derivation from directory layout invariants. |
| src/iocore/cache/Stripe.cc | Bounds Stripe::dir_check() chain walking to prevent runaway traversal/array overrun when chains loop. |
| src/iocore/cache/unit_tests/test_CacheDir.cc | Reworks corruption tests to reliably exercise bounded reader behavior and writer repair paths in all builds. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bneradt
left a comment
bneradt
left a comment
There was a problem hiding this comment.
Reviewed the bounded walks, segment reset paths, and updated unit tests. One persistence issue noted inline. All reported PR checks pass; local tests were not run because this worktree has no configured build.
| return 0; | ||
| // Past the longest legitimate chain, so this is provably a cycle. | ||
| if (++loop_count > this->max_bucket_depth()) { | ||
| this->bucket_loop_fix(dir_bucket(b, seg), s); |
There was a problem hiding this comment.
[P2] Mark the directory dirty when repairing a cycle
If header->dirty is 0 when this path runs, bucket_loop_fix() clears the segment through init_segment(), but neither helper sets the dirty flag, and this return bypasses delete_entry() (which normally sets it). The new overwrite(..., must_overwrite=true) repair path has the same problem: after resetting the segment it retries and returns 0 before Lfill. CacheSync::mainEvent() skips directories whose dirty flag is clear, so without a subsequent mutation the repair is never periodically persisted; an already-persisted corrupt chain can return after a crash. Please mark successful repairs dirty, preferably in the shared repair helper, and cover these paths starting with header->dirty = 0.
The loop protection in probe/overwrite/remove/dir_clean_bucket sat behind LOOP_CHECK_MODE, which is commented out and not settable from CMake, so production builds walked bucket chains unbounded. A cycle in dir_next spins an event thread while it holds the stripe mutex.
I have never seen this infinite loop in production. This is defensive change and cleanup of the
LOOP_CHECK_MODE.Directory::max_bucket_depth() is the bound,
(DIR_DEPTH - 1) * buckets + 1: init_segment() frees rows1..DIR_DEPTH-1of every bucket onto the free list and never row 0, so one chain holds at most every free entry of the segment plus its own head. Exceeding it proves a cycle, so no exact check is needed at the trigger. bucket_length() capped at a hardcoded 100 instead, below the longest legitimate chain, so a chain over that would make check_segment() report corruption and Stripe::_shm_directory_is_valid() reject a healthy shared memory attach.Readers report and move on, writers repair. probe() throttles its report because it does not repair, so the loop lives until a writer clears it and every lookup that hashes to the bucket lands on the same trigger. insert()'s tail walk is the only chain walk it makes, so that is where a writer meets a cycle, and stopping at the cap linked the new entry into the cycle.
Stripe::dir_check() had no cap at all and wrote chain_tag[] once per step, so a cycle ran off the end of a 65536-entry stack array. It now stops at the bound, and at the first revisited entry rather than printing a cycle line per step. check_bucket_not_contains(), an unbounded walk with no caller since the 2009 import, is deleted.