masaori335 commented on code in PR #13608:
URL: https://github.com/apache/trafficserver/pull/13608#discussion_r3900863516
##########
src/iocore/cache/CacheDir.cc:
##########
@@ -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;
+ }
Review Comment:
The loop concern doesn't hold, and the suggested fix would cost more than
the case it addresses.
bucket_loop_fix() returns 0 only when dir_bucket_loop_check() (Floyd's,
CacheDir.cc:179) proves the chain acyclic — it's exact, not a heuristic that
can "fail". And even setting that aside, the fall-through does dir_set_next(e,
0); dir_set_next(prev, dir_to_offset(e, seg)), which repoints prev at a
chain-terminating entry. That severs a cycle at prev; it cannot link into one.
So the break path is: a chain longer than (DIR_DEPTH - 1) * buckets + 1 that
is genuinely acyclic. That needs corrupted next offsets pulling other buckets'
row-0 heads into the chain, since the longest acyclic path in a segment is
DIR_DEPTH * buckets — a window of buckets - 1 entries above the bound. The
consequence is that the tail past prev is orphaned: off the chain, off the
freelist, reclaimed at the next init_segment(). No hang, no cycle, no wrong
data.
Forcing init_segment() there would discard up to DIR_DEPTH * buckets valid
entries to reclaim at most buckets - 1 orphans, on a segment we just proved has
no loop. Not a trade I want to make on a path that is already corruption-only.
For context, the link-after-cap behavior is not new — the previous code was
while (last && (++l <= this->buckets * DIR_DEPTH)), same fall-through, without
the repair attempt this PR adds.
--
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]
