You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "masaori335 (via GitHub)" <gi...@apache.org> on 2026/10/15 05:40:27 UTC
[PR] cache: make Directory::probe() read-only [trafficserver]
masaori335 opened a new pull request, #13840:
URL: https://github.com/apache/trafficserver/pull/13840
# Motivation
I have been experimenting with replacing the stripe mutex with a reader-writer lock, hazard pointers, or RCU,
so that cache lookups can run concurrently. Every attempt hit the same obstacle: `Directory::probe()` looks
like a lookup, but it deletes every stale entry it walks past. That makes every lookup a directory writer, so
it needs exclusive access.
Other readers can see those writes. A lookup marks the directory dirty. When the stale entry is a bucket head,
`delete_entry()` moves the next entry into the head slot, which can invalidate another reader's
`last_collision` cursor.
# Approach
Make `probe()` a pure reader. Stale entries are left to the writers that already collect them:
`Directory::cleanup()` at every cache cycle wrap, and `freelist_clean()` when a segment's free list runs out.
`freelist_clean()` reclaims stale entries before it purges any live ones.
# Trade-off
Stale entries now hold directory slots until the next wrap. `probe()` only ever reclaimed the stale entries of
keys that were looked up again, so the extra retention is bounded by those. On the production caches we
checked, directory occupancy peaks well below half of capacity, so the extra retention fits easily. Where it
doesn't, `freelist_clean()` reclaims the stale entries first.
# Visible changes
- **`proxy.process.cache.direntries.used` (global and per-volume) counts stale entries until they are reclaimed.**
It now climbs higher through a cache cycle and drops at each wrap.
- **The `cache_dir_remove_invalid` USDT probe is removed.** `probe()` was its only emission site.
# Testing
- New `test_CacheDir` cases:
- `probe()` leaves a stale entry in place, and `clean_segment()` reclaims it.
- With a stale entry ahead of a live one for the same key, `probe()` returns the live entry, and resuming
from it returns no further match rather than restarting from the bucket head.
- The `ctest` cache suite passes.
# Follow-up
- Make the directory read path const-enforced, so a reader that mutates an entry fails to compile (stacked on
this PR).
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] cache: make Directory::probe() read-only [trafficserver]
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 merged PR #13840:
URL: https://github.com/apache/trafficserver/pull/13840
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] cache: make Directory::probe() read-only [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13840:
URL: https://github.com/apache/trafficserver/pull/13840#discussion_r4279497940
##########
src/iocore/cache/CacheDir.cc:
##########
@@ -497,19 +495,13 @@ Directory::probe(const CacheKey *key, StripeSM *stripe, Dir *result, Dir **last_
*last_collision = e;
ink_assert(dir_offset(e) * CACHE_BLOCK_SIZE < stripe->len);
return 1;
- } else { // delete the invalid entry
- ts::Metrics::Gauge::decrement(cache_rsb.direntries_used);
- ts::Metrics::Gauge::decrement(stripe->cache_vol->vol_rsb.direntries_used);
- ATS_PROBE7(cache_dir_remove_invalid, stripe->fd, s, dir_to_offset(e, seg), dir_offset(e), dir_approx_size(e),
- key->slice64(0), key->slice64(1));
- e = this->delete_entry(e, p, s);
- continue;
}
+ // Stale entry, left in place: reclaiming it here would make every lookup a writer. A writer collects it
+ // instead -- Directory::cleanup() at each cache cycle wrap, or freelist_clean() on freelist exhaustion.
Review Comment:
Retaining stale entries makes writer matching ambiguous when an offset is reused. If a stale entry precedes a live entry with the same tag and offset but opposite phases, `probe()` now skips the stale entry and returns the live one. However, `remove()` and `overwrite()` match only tag and offset (CacheDir.cc:703 and :627), so they select the stale entry instead. `CacheVC::removeEvent()` then reports successful removal while the live object remains cached; overwrite can likewise update the wrong entry. Make writer matching or writer-side cleanup distinguish these entries, and add removal/overwrite regressions with stale and live entries sharing an offset. If matching is made phase-aware, also update callers that construct synthetic targets, such as the `del_dir` in StripeSM.cc:757–762.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] cache: make Directory::probe() read-only [trafficserver]
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 commented on PR #13840:
URL: https://github.com/apache/trafficserver/pull/13840#issuecomment-6206127356
@brbzull0 Thank you for check.
- del_dir: agreed — the byte offset dates back to the initial import, so remove() never matched there. Fixed separately in #13847 with its own regression test, since it changes the write-error path on its own; that PR also removes head entries under first_key, which the offset fix alone would still have missed for single-fragment objects.
- direntries.used: added a line under Metrics in upgrading.en.rst (4ed7f6b4f3). Note the gauge already counted stale fragment entries until the wrap before this PR — each write gets a random earliest_key, so probe() only ever reclaimed re-requested heads — so this widens an existing gap rather than opening a new one.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] cache: make Directory::probe() read-only [trafficserver]
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 commented on code in PR #13840:
URL: https://github.com/apache/trafficserver/pull/13840#discussion_r4279922877
##########
src/iocore/cache/CacheDir.cc:
##########
@@ -497,19 +495,13 @@ Directory::probe(const CacheKey *key, StripeSM *stripe, Dir *result, Dir **last_
*last_collision = e;
ink_assert(dir_offset(e) * CACHE_BLOCK_SIZE < stripe->len);
return 1;
- } else { // delete the invalid entry
- ts::Metrics::Gauge::decrement(cache_rsb.direntries_used);
- ts::Metrics::Gauge::decrement(stripe->cache_vol->vol_rsb.direntries_used);
- ATS_PROBE7(cache_dir_remove_invalid, stripe->fd, s, dir_to_offset(e, seg), dir_offset(e), dir_approx_size(e),
- key->slice64(0), key->slice64(1));
- e = this->delete_entry(e, p, s);
- continue;
}
+ // Stale entry, left in place: reclaiming it here would make every lookup a writer. A writer collects it
+ // instead -- Directory::cleanup() at each cache cycle wrap, or freelist_clean() on freelist exhaustion.
Review Comment:
Valid — the old probe() deletion is what kept this from happening, since removeEvent() probes first. Fixed in 19faf693db: remove() and overwrite() now match tag, offset and phase, which records the cycle an entry was written in. A stale and a live entry at the same offset always differ in phase (an entry two cycles old is swept by the wrap cleanup() first). The synthetic del_dir in the write-error path now carries the current phase. Added regressions for both writers with a stale and a live entry sharing tag and offset.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org