Skip to content

Bound cache directory bucket walks in all builds #13608

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
masaori335 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Bound cache directory bucket walks in all builds #13608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 44 additions & 62 deletions src/iocore/cache/CacheDir.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,6 @@
#include <unordered_map>
#include <utility>

#ifdef LOOP_CHECK_MODE
#define DIR_LOOP_THRESHOLD 1000
#endif

namespace
{

Expand Down Expand Up @@ -260,13 +256,9 @@ Directory::bucket_length(Dir *b, int s)
Dir *e = b;
int i = 0;
Dir *seg = this->get_segment(s);
#ifdef LOOP_CHECK_MODE
if (this->bucket_loop_fix(b, s))
return 1;
#endif
while (e) {
i++;
if (i > 100) {
if (i > this->max_bucket_depth()) {
return -1;
}
e = next_dir(e, seg);
Expand Down Expand Up @@ -310,18 +302,14 @@ inline void
dir_clean_bucket(Dir *b, int s, StripeSM *stripe)
{
Dir *e = b, *p = nullptr;
Dir *seg = stripe->directory.get_segment(s);
#ifdef LOOP_CHECK_MODE
int loop_count = 0;
#endif
Dir *seg = stripe->directory.get_segment(s);
int loop_count = 0;
do {
#ifdef LOOP_CHECK_MODE
loop_count++;
if (loop_count > DIR_LOOP_THRESHOLD) {
if (stripe->directory.bucket_loop_fix(b, s))
return;
// Past the longest legitimate chain, so this is provably a cycle.
if (++loop_count > stripe->directory.max_bucket_depth()) {
stripe->directory.bucket_loop_fix(b, s);
return;
}
#endif
if (!stripe->dir_valid(e) || !dir_offset(e)) {
if (dbg_ctl_dir_clean.on()) {
Dbg(dbg_ctl_dir_clean, "cleaning Stripe:%s: %p tag %X boffset %" PRId64 " b %p p %p bucket len %d", stripe->hash_text.get(),
Expand Down Expand Up @@ -374,19 +362,6 @@ Directory::clear_range(off_t start, off_t end, StripeSM *stripe)
this->cleanup(stripe);
}

void
check_bucket_not_contains(Dir *b, Dir *e, Dir *seg)
{
Dir *x = b;
do {
if (x == e) {
break;
}
x = next_dir(x, seg);
} while (x);
ink_assert(!x);
}

void
freelist_clean(int s, StripeSM *stripe)
{
Expand Down Expand Up @@ -450,19 +425,25 @@ int
Directory::probe(const CacheKey *key, StripeSM *stripe, Dir *result, Dir **last_collision)
{
ink_assert(stripe->mutex->thread_holding == this_ethread());
int s = key->slice32(0) % this->segments;
int b = key->slice32(1) % this->buckets;
Dir *seg = this->get_segment(s);
Dir *e = nullptr, *p = nullptr, *collision = *last_collision;
int s = key->slice32(0) % this->segments;
int b = key->slice32(1) % this->buckets;
Dir *seg = this->get_segment(s);
Dir *e = nullptr;
Dir *p = nullptr;
Dir *collision = *last_collision;
CHECK_DIR(d);
#ifdef LOOP_CHECK_MODE
if (this->bucket_loop_fix(dir_bucket(b, seg), s))
return 0;
#endif
Lagain:
e = dir_bucket(b, seg);
if (dir_offset(e)) {
int loop_count = 0;
do {
// Past the longest legitimate chain, so this is provably a cycle. probe() is a reader: it neither repairs nor
// walks on, and the report is throttled because the loop lives until a writer repairs it, so every lookup that
// hashes to this bucket lands here.
if (++loop_count > this->max_bucket_depth()) {
SiteThrottledWarning("directory loop on '%s' segment %d bucket %d, abandoning probe", stripe->hash_text.get(), s, b);
return 0;
}
if (dir_compare_tag(e, key)) {
ink_assert(dir_offset(e));
// Bug: 51680. Need to check collision before checking
Expand Down Expand Up @@ -567,7 +548,15 @@ Directory::insert(const CacheKey *key, StripeSM *stripe, Dir *to_part)
do {
prev = last;
last = next_dir(last, seg);
} while (last && (++l <= this->buckets * DIR_DEPTH));
// Past the longest legitimate chain, so this is provably a cycle. This is the only walk insert() makes, so it is
// where a writer meets one: repair and start over rather than linking into the cycle.
if (++l > this->max_bucket_depth()) {
if (this->bucket_loop_fix(b, s)) {
goto Lagain;
}
break;
}
Comment thread
masaori335 marked this conversation as resolved.
} while (last);

dir_set_next(e, 0);
dir_set_next(prev, dir_to_offset(e, seg));
Expand Down Expand Up @@ -598,10 +587,6 @@ Directory::overwrite(const CacheKey *key, StripeSM *stripe, Dir *dir, Dir *overw
Dir *b = dir_bucket(bi, seg);
unsigned int t = DIR_MASK_TAG(key->slice32(2));
int res = 1;
#ifdef LOOP_CHECK_MODE
int loop_count = 0;
bool loop_possible = true;
#endif
CHECK_DIR(d);

ink_assert(static_cast<unsigned int>(dir_approx_size(dir)) <=
Expand All @@ -610,16 +595,16 @@ Directory::overwrite(const CacheKey *key, StripeSM *stripe, Dir *dir, Dir *overw
// find entry to overwrite
e = b;
if (dir_offset(e)) {
int loop_count = 0;
do {
#ifdef LOOP_CHECK_MODE
loop_count++;
if (loop_count > DIR_LOOP_THRESHOLD && loop_possible) {
if (this->bucket_loop_fix(b, s)) {
loop_possible = false;
goto Lagain;
// Past the longest legitimate chain, so this is provably a cycle. Repair and restart; bail if
// the repair somehow disagrees, rather than resuming an unbounded walk.
if (++loop_count > this->max_bucket_depth()) {
if (!this->bucket_loop_fix(b, s)) {
return 0;
}
goto Lagain;
}
#endif
if (dir_tag(e) == t && dir_offset(e) == dir_offset(overwrite)) {
goto Lfill;
}
Expand Down Expand Up @@ -657,10 +642,11 @@ Directory::overwrite(const CacheKey *key, StripeSM *stripe, Dir *dir, Dir *overw

l = 0;
last = b;
// No repair here, unlike insert(): the walk above already proved this chain finite. The cap is a backstop.
do {
prev = last;
last = next_dir(last, seg);
} while (last && (++l <= this->buckets * DIR_DEPTH));
} while (last && (++l <= this->max_bucket_depth()));

dir_set_next(e, 0);
dir_set_next(prev, dir_to_offset(e, seg));
Expand All @@ -683,21 +669,17 @@ Directory::remove(const CacheKey *key, StripeSM *stripe, Dir *del)
int b = key->slice32(1) % this->buckets;
Dir *seg = this->get_segment(s);
Dir *e = nullptr, *p = nullptr;
#ifdef LOOP_CHECK_MODE
int loop_count = 0;
#endif
CHECK_DIR(vol);

e = dir_bucket(b, seg);
if (dir_offset(e)) {
int loop_count = 0;
do {
#ifdef LOOP_CHECK_MODE
loop_count++;
if (loop_count > DIR_LOOP_THRESHOLD) {
if (this->bucket_loop_fix(dir_bucket(b, seg), s))
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);

@bneradt bneradt Sep 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

return 0;
}
#endif
int64_t offset = dir_offset(e);
if (dir_compare_tag(e, key) && offset == dir_offset(del)) {
ts::Metrics::Gauge::decrement(cache_rsb.direntries_used);
Expand Down
16 changes: 14 additions & 2 deletions src/iocore/cache/P_CacheDir.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ struct InterimCacheVol;
struct CacheVC;
class CacheEvacuateDocVC;

// #define LOOP_CHECK_MODE 1

/*
Directory layout
*/
Expand Down Expand Up @@ -308,6 +306,14 @@ class Directory
*/
Dir *get_segment(int s) const;

/* Longest chain a bucket can legitimately hold.
*
* init_segment() frees rows 1..DIR_DEPTH-1 of every bucket onto the segment free list and never row 0, so one
* chain holds at most every free entry of the segment plus its own head. A walk past this has revisited an entry,
* which means the chain loops.
*/
int max_bucket_depth() const;

int probe(const CacheKey *, StripeSM *, Dir *, Dir **);
int insert(const CacheKey *key, StripeSM *stripe, Dir *to_part);
int overwrite(const CacheKey *key, StripeSM *stripe, Dir *to_part, Dir *overwrite, bool must_overwrite = true);
Expand Down Expand Up @@ -407,6 +413,12 @@ Directory::get_segment(int s) const
return reinterpret_cast<Dir *>((reinterpret_cast<char *>(this->dir)) + (s * this->buckets) * DIR_DEPTH * SIZEOF_DIR);
}

inline int
Directory::max_bucket_depth() const
{
return (DIR_DEPTH - 1) * this->buckets + 1;
}

inline void
Directory::unlink_from_freelist(Dir *e, int s)
{
Expand Down
6 changes: 6 additions & 0 deletions src/iocore/cache/Stripe.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,11 +402,17 @@ Stripe::dir_check()
ink_assert(nullptr == next_dir(e, seg));
break;
} else {
// Past the longest legitimate chain the walk is looping; stop before chain_tag overruns.
if (h >= directory.max_bucket_depth()) {
printf(" - Chain from bucket %d is longer than %d entries, abandoning walk\n", b, directory.max_bucket_depth());
break;
}
int e_idx = e - seg;
++h;
chain_tag[chain_idx++] = dir_tag(e);
if (chain_mark[e_idx] == mark) {
printf(" - Cycle of length %d detected for bucket %d\n", h, b);
break;
} else if (chain_mark[e_idx] >= 0) {
printf(" - Entry %d is in chain %d and %d", e_idx, chain_mark[e_idx], mark);
} else {
Expand Down
99 changes: 41 additions & 58 deletions src/iocore/cache/unit_tests/test_CacheDir.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,75 +166,58 @@ class CacheDirTest : public CacheInit

Dbg(dbg_ctl_cache_dir_test, "corrupt_bucket test");
for (int ntimes = 0; ntimes < 10; ntimes++) {
#ifdef LOOP_CHECK_MODE
// probe in bucket with loop
rand_CacheKey(&key);
s1 = key.slice32(0) % stripe->directory.segments;
b1 = key.slice32(1) % stripe->directory.buckets;
dir_corrupt_bucket(dir_bucket(b1, stripe->directory.get_segment(s1)), s1, stripe);
stripe->directory.insert(&key, stripe, &dir);
Dir *last_collision = 0;
stripe->directory.probe(&key, stripe, &dir, &last_collision);

rand_CacheKey(&key);
s1 = key.slice32(0) % stripe->directory.segments;
b1 = key.slice32(1) % stripe->directory.buckets;
dir_corrupt_bucket(dir_bucket(b1, stripe->directory.get_segment(s1)), s1, stripe);

last_collision = 0;
stripe->directory.probe(&key, stripe, &dir, &last_collision);
// Reset every iteration: Directory::check() fails from the first corruption onward, so without this only the
// first iteration would assert anything.
stripe->clear_dir();

// overwrite in bucket with loop
rand_CacheKey(&key);
s1 = key.slice32(0) % stripe->directory.segments;
b1 = key.slice32(1) % stripe->directory.buckets;
CacheKey key1;
key1.b[1] = 127;
dir1 = dir;
dir_set_offset(&dir1, 23);
stripe->directory.insert(&key1, stripe, &dir1);
stripe->directory.insert(&key, stripe, &dir);
key1.b[1] = 80;
stripe->directory.insert(&key1, stripe, &dir1);
dir_corrupt_bucket(dir_bucket(b1, stripe->directory.get_segment(s1)), s1, stripe);
stripe->directory.overwrite(&key, stripe, &dir, &dir, 1);

rand_CacheKey(&key);
s1 = key.slice32(0) % stripe->directory.segments;
b1 = key.slice32(1) % stripe->directory.buckets;
key.b[1] = 23;
stripe->directory.insert(&key, stripe, &dir1);
// Valid entries, so probe() below walks past them instead of deleting them as it goes.
stripe->directory.header->agg_pos = stripe->directory.header->write_pos += 1024;
dir_clear(&dir1);
dir_set_offset(&dir1, 1);
REQUIRE(stripe->dir_valid(&dir1));
for (int i = 0; i < 5; i++) {
stripe->directory.insert(&key, stripe, &dir1);
}
dir_corrupt_bucket(dir_bucket(b1, stripe->directory.get_segment(s1)), s1, stripe);
stripe->directory.overwrite(&key, stripe, &dir, &dir, 0);

rand_CacheKey(&key);
s1 = key.slice32(0) % stripe->directory.segments;
Dir *seg1 = stripe->directory.get_segment(s1);
// freelist_length in freelist with loop
dir_corrupt_bucket(dir_from_offset(stripe->directory.header->freelist[s], seg1), s1, stripe);
stripe->directory.freelist_length(s1);
// Detection: a cycle makes the chain longer than its segment can hold.
CHECK(!stripe->directory.check());
CHECK(stripe->directory.bucket_length(dir_bucket(b1, stripe->directory.get_segment(s1)), s1) == -1);

// A reader must terminate on a looped chain. Vary only slice32(2), the tag, so the probe lands on the same
// segment and bucket but matches no entry and therefore walks the whole cycle.
CacheKey miss_key = key;
miss_key.u32[2] = ~key.u32[2];
Dir probed;
Dir *probe_collision = nullptr;
dir_clear(&probed);
CHECK(stripe->directory.probe(&miss_key, stripe, &probed, &probe_collision) == 0);

// The reader leaves the loop in place for a writer to repair.
CHECK(!stripe->directory.check());

rand_CacheKey(&key);
s1 = key.slice32(0) % stripe->directory.segments;
b1 = key.slice32(1) % stripe->directory.buckets;
// bucket_length in bucket with loop
dir_corrupt_bucket(dir_bucket(b1, stripe->directory.get_segment(s1)), s1, stripe);
stripe->directory.bucket_length(dir_bucket(b1, stripe->directory.get_segment(s1)), s1);
// insert() only walks the chain once the bucket's own rows are full, which the five entries above ensure, and
// that walk must repair.
stripe->directory.insert(&key, stripe, &dir1);
CHECK(stripe->directory.check());
#else
// test corruption detection
rand_CacheKey(&key);
s1 = key.slice32(0) % stripe->directory.segments;
b1 = key.slice32(1) % stripe->directory.buckets;

stripe->directory.insert(&key, stripe, &dir1);
stripe->directory.insert(&key, stripe, &dir1);
stripe->directory.insert(&key, stripe, &dir1);
stripe->directory.insert(&key, stripe, &dir1);
stripe->directory.insert(&key, stripe, &dir1);
// overwrite() repairs from its own walk, so give it a freshly corrupted chain.
for (int i = 0; i < 5; i++) {
stripe->directory.insert(&key, stripe, &dir1);
}
dir_corrupt_bucket(dir_bucket(b1, stripe->directory.get_segment(s1)), s1, stripe);
CHECK(!stripe->directory.check());
#endif

// Target an offset absent from the chain so the search actually walks it rather than matching the head entry
// immediately.
Dir absent;
dir_clear(&absent);
dir_set_offset(&absent, 999);
stripe->directory.overwrite(&key, stripe, &dir1, &absent, false);
CHECK(stripe->directory.check());
}
stripe->clear_dir();

Expand Down