You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "phongn (via GitHub)" <gi...@apache.org> on 2026/07/14 16:13:36 UTC
[PR] Honor the RamCache copy flag in LRU and S3-FIFO [trafficserver]
phongn opened a new pull request, #13380:
URL: https://github.com/apache/trafficserver/pull/13380
## Summary
`put(..., copy = true)` is the caller's contract that buffers may not be shared between the cache and the caller in either direction. `CacheVC` requests it whenever RAM cache compression is configured (`http_copy_hdr`, `CacheVC.cc`), because it unmarshals HTTP headers in place in its own buffer *after* the put, and again on every RAM-cache hit.
The LRU and S3-FIFO RAM caches ignored the flag and shared buffers both ways. With `proxy.config.cache.ram_cache.compress` set and `algorithm = 1` (the default) or `2`:
1. put stores the caller's buffer, and CacheVC's subsequent in-place unmarshal silently mutates the cached bytes;
2. the next HTTP RAM hit hands that buffer back and unmarshals it again — `HTTPInfo::unmarshal` fails on the already-unmarshalled header (`ink_assert` in debug builds; broken HTTP RAM hits in release).
The `http_copy_hdr` gate keys off the compress config only, never the algorithm, so this is reachable purely by configuration today. This correctness fix stands alone (compression support for LRU is proposed separately) and is a backport candidate.
## Changes
- `RamCacheLRU` and `RamCacheS3FIFO` now honor `copy` the way CLFUS always has: copy on put, copy on get, and a `copy = true` put refreshes a resident entry that may still be sharing a caller's buffer from before a configuration change.
- `copy = false` behavior is unchanged: zero-copy sharing, pinned by a test.
- New Catch2 test (`test_RamCacheCopy`) pins the contract for all three policies: caller mutation after put, caller mutation of the returned buffer, the resident-refresh transition, and copy=false sharing. Before the fix, the three copy cases fail for LRU and S3-FIFO; CLFUS passes.
## Notes for reviewers
- The copied-in buffer is an exact-size xmalloc allocation, so `block_size()` recovers the data length and neither entry struct needs a new length field.
- S3-FIFO's ghost-readmit and stale-auxkey paths go through the single insert site, so they inherit the copy handling.
- The test keeps constructed caches reachable rather than destroying them: the policies have no destructors (entries are pool-allocated), so destroying a cache object would strand its entries under leak checkers.
--
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] Honor the RamCache copy flag in LRU and S3-FIFO [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on PR #13380:
URL: https://github.com/apache/trafficserver/pull/13380#issuecomment-5109845464
Read through this and ran the tests locally. The bug reproduces as described: `http_copy_hdr` at `CacheVC.cc:403` is computed from `cache_config_ram_cache_compress` and the doc properties with no reference to the algorithm, and `RecordsConfig.cc:863` defaults `ram_cache.algorithm` to 1, so the default policy was the one ignoring the flag.
I checked a couple of other cases:
Tested on a 5000 byte object to exercise the size rounding in `iobuffer_size_to_index()`. The tests still pass. Worth using a non-power-of-two payload anyway, so the tests do not depend on 8192 landing exactly on a size index.
Checked the `ram_cache_bytes` gauge across the resident refresh: N objects put with `copy=false`, re-put with `copy=true`, then the cache overflowed to force eviction. The gauge rises on the puts and does not go negative through eviction, for all three policies. That path is the only place the fix does arithmetic rather than just swapping the buffer, so it is worth a permanent test.
I also recommend an S3-FIFO ghost admit test. The copy handling is inherited -- the ghost is removed and the fresh entry goes through the same insert site -- but the readmit lands in `SEG_MAIN` and accounts against `_m_bytes`, and with `copy` set the charged size is `len` rather than `data->block_size()`. That combination is not covered.
Happy to approve once it comes out of draft.
--
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] Honor the RamCache copy flag in LRU and S3-FIFO [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen merged PR #13380:
URL: https://github.com/apache/trafficserver/pull/13380
--
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] Honor the RamCache copy flag in LRU and S3-FIFO [trafficserver]
Posted by "phongn (via GitHub)" <gi...@apache.org>.
phongn commented on code in PR #13380:
URL: https://github.com/apache/trafficserver/pull/13380#discussion_r3972122705
##########
src/iocore/cache/RamCacheLRU.cc:
##########
@@ -213,6 +218,26 @@ RamCacheLRU::put(CryptoHash *key, IOBufferData *data, [[maybe_unused]] uint32_t
if (e->auxkey == auxkey) {
lru.remove(e);
lru.enqueue(e);
+ if (copy) {
Review Comment:
Done in f3286882a7 — LRU and S3-FIFO both guard on `copy && !e->copy` now, so the refresh is reserved for the shared-to-private transition. Thanks; this race also corrects my framing from the last round. I'd described the refresh branch as unreachable, but that's true only of the shared-to-private *transition*. The branch itself fires on exactly the concurrent-miss case you describe, which is why the guard has real value.
I also applied it to CLFUS, since its pre-existing resident path has the same redundant copy — it replaces `e->data` unconditionally on every re-put, both arms. Under the default seen filter it's actually the most exposed of the three: CLFUS gates its filter on new entries only and S3-FIFO has none, so both hit the refresh on the second racing put, while LRU filters *before* the resident walk and toggles per slot, so it needs a fourth.
The CLFUS guard is narrower, though: `copy && e->flag_bits.copy && !e->flag_bits.compressed`. The plain form is unsafe there. A compressed entry holds `compressed_len` bytes behind `flag_bits.copy`, and the resident path clears `flag_bits.compressed` after the swap — skipping the swap while still clearing the flag would have `get()` memcpy `e->len` bytes out of a `compressed_len`-byte buffer (`compressed_len < e->len` is the compressor's admission condition). So a compressed entry still takes the swap, which is what decompresses it. The narrow guard is exact rather than approximate: every path that produces an uncompressed private entry sets `size == len` (Linsert; the compressor's no-gain branch; `get()`'s in-place decompress is `!copy`-only), so the `delta` it skips is 0 and the flag writes it skips are no-ops. Only the allocation and memcpy go away.
One honest caveat: the `!compressed` term isn't covered by a unit test. It needs a compressed entry, which requires the background compressor on `ET_TASK`, and that isn't reachable from the Catch2 harness. It rests on the reasoning above. In the race itself the entry is freshly inserted and the compressor hasn't run, so the term is there for correctness rather than for the common path.
The guard is invisible to the gauge — a private copy is already charged `len`, so `delta` is 0 either way — so the new test pins it by re-putting *different* bytes under the same key and auxkey and checking the cache kept the first, for all three policies. Real callers never do that (same key and auxkey name one on-disk doc), but it's the only observable. It also confirms the shared-to-private refresh still happens, so the guard is on the entry's state and not on how many puts it has seen. Disabling any one of the three guards fails that test for its policy.
--
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] Honor the RamCache copy flag in LRU and S3-FIFO [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on code in PR #13380:
URL: https://github.com/apache/trafficserver/pull/13380#discussion_r4067001367
##########
src/iocore/cache/RamCacheLRU.cc:
##########
@@ -230,16 +235,60 @@ RamCacheLRU::put(CryptoHash *key, IOBufferData *data, [[maybe_unused]] uint32_t
}
}
+ RamCacheLRUEntry *e = bucket[i].head;
Review Comment:
The rebase left two resident-entry loops. This one redeclares `e` (`error: redeclaration of 'RamCacheLRUEntry* e'`), and with that fixed it's still unreachable: the loop above from #13669 returns 1 on a resident hit, so the `copy && !e->copy` refresh never runs and 4 of the 8 `test_RamCacheCopy` cases fail. Move the refresh into the first loop and drop this one. With that change the RAM cache tests pass locally.
--
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] Honor the RamCache copy flag in LRU and S3-FIFO [trafficserver]
Posted by "phongn (via GitHub)" <gi...@apache.org>.
phongn commented on PR #13380:
URL: https://github.com/apache/trafficserver/pull/13380#issuecomment-4971666378
Tracking issue: #13381
--
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] Honor the RamCache copy flag in LRU and S3-FIFO [trafficserver]
Posted by "phongn (via GitHub)" <gi...@apache.org>.
phongn commented on PR #13380:
URL: https://github.com/apache/trafficserver/pull/13380#issuecomment-5414228310
Thanks for the careful read — all three are in, and the first turned out to be load-bearing rather than hygiene.
**Non-power-of-two payload.** Switched to 5000 bytes, with a helper deriving the rounding difference instead of hardcoding it. You were right that the tests shouldn't depend on 8192 landing on a size index: at 8192 every size computation this fix adds is degenerate. `size = copy ? len : data->block_size()` returns the same value on both arms, and `delta` is 0 in both refresh paths. Confirmed by mutation — changing LRU's refresh to charge `data->block_size()` instead of `len` now fails (`66560 == 41024`), and at 8192 that same mutation passes silently.
**`ram_cache_bytes` across the refresh.** Added for all three policies, asserting the exact rounding given back per object rather than a range. It's uniform because CLFUS turns out to share the same refresh arithmetic (`delta = size - e->size`), so all three give back exactly `block_size(5000) - 5000` per object. It also checks that the per-volume gauge tracks the global one, and that neither goes negative through the eviction flood.
Chasing that turned up something the gauge alone cannot see: the gauge and the internal counter that admission and eviction actually run off (`bytes`, `_s_bytes`/`_m_bytes`) are updated independently, so deleting the counter update while keeping the gauge passed every assertion. Covered both ways now. For S3-FIFO `size()` derives from those counters, so it is checked against the gauge at a point where no ghosts exist yet. For LRU `size()` recomputes from the entry list with a different per-entry formula and structurally cannot witness the counter, so that one is behavioral: fill the cache, refresh every entry, then insert more objects than there was room for beforehand — they fit only if the refresh really returned the bytes, and nothing resident is evicted if it did.
**S3-FIFO ghost admit.** Added. I wanted to be sure it wasn't quietly degenerating into a plain insert, so I instrumented the branch temporarily and confirmed it fires exactly once with `copy=1 len=5000 old_size=8192 seg=2` (`SEG_GHOST`) — so it does cross from a `block_size()`-charged entry to a `len`-charged one, which was your point. It also floods a second time afterwards and checks the object survives, as evidence the readmit landed in main rather than small.
Two things fell out of this that are worth flagging:
- LRU was the only policy storing no data length, recovering it from `e->data->block_size()` on get. That is correct only because `copy_data_in()` happens to allocate with `new_xmalloc_IOBufferData`, an invariant maintained in a different file with nothing checking it. It now stores `len` like CLFUS and S3-FIFO, and the refresh delta is derived from the two allocations, so neither the length handed back nor the accounting depends on how `copy_data_in()` allocates.
- The description was wrong about the resident-refresh path. I had justified it as handling a configuration change, but `ram_cache.compress` is `RECU_RESTART_TS`, so `cache_config_ram_cache_compress` cannot change within a process, and `http_copy_hdr`'s other inputs (`doc_type`, `hlen`) are fixed for a given doc — so the copy flag cannot flip for an entry that is already resident. Both `fixup()` callsites are evacuations that relocate a doc byte-identically, so they do not flip it either. I've kept the path, since the contract is per-put, but rewrote the description to call it defensive, and noted the consequence: `delta` is never positive, which is why the refresh needs no eviction pass. That reasoning is now a comment at both sites.
--
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] Honor the RamCache copy flag in LRU and S3-FIFO [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on code in PR #13380:
URL: https://github.com/apache/trafficserver/pull/13380#discussion_r3915349507
##########
src/iocore/cache/RamCacheLRU.cc:
##########
@@ -213,6 +218,26 @@ RamCacheLRU::put(CryptoHash *key, IOBufferData *data, [[maybe_unused]] uint32_t
if (e->auxkey == auxkey) {
lru.remove(e);
lru.enqueue(e);
+ if (copy) {
Review Comment:
Please guard this with `copy && !e->copy`.
When two requests both get a RAM-cache miss, both read the same object from disk, and then both try to fill the RAM cache, whichever finishes first inserts a private copy. The other request finds that entry resident and currently allocates and copies the entire object again. Both requests allocate a RAM-cache buffer, but only the first allocation is necessary.
If `e->copy` is already true, the stored buffer is private and `get()` never exposes it directly, so replacing it is unnecessary. The guard preserves the shared-to-private transition while avoiding the redundant allocation and memcpy. The same applies to the corresponding resident-refresh block in S3-FIFO.
--
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] Honor the RamCache copy flag in LRU and S3-FIFO [trafficserver]
Posted by "phongn (via GitHub)" <gi...@apache.org>.
phongn commented on PR #13380:
URL: https://github.com/apache/trafficserver/pull/13380#issuecomment-5608597981
Two corrections to things I said earlier in this PR, both from a closer read after your guard comment. I've rewritten the description accordingly.
**`ram_cache.compress` is not fixed for the life of the process.** I'd claimed the shared-to-private refresh was unreachable because the record is `RECU_RESTART_TS`. That marking is advisory: `cache_config_ram_cache_compress` is linked via `RecEstablishStaticConfigInt32`, and `RecExecConfigUpdateCbs` fires every pending callback regardless of update type -- the type is only returned so `traffic_ctl` can report that a restart is needed. So a reload changes the value live. That makes the refresh reachable (a reload landing between two racing misses). One thing that surfaced from that and that I've deliberately left out of this PR: LRU runs its seen filter before the resident-entry lookup, so it can decline exactly that refresh (CLFUS gates its filter on new entries only). That's a pre-existing LRU behavior rather than part of the copy contract, so it will come as its own PR. It also means a 1→0 reload leaves LRU/S3-FIFO holding marshaled private copies that `handleRead` no longer
routes through the unmarshal -- CLFUS has always had that exposure, this PR extends it, and the right fix (decide from the buffer's state, not the config) is in CacheVC. Called out in the description as a follow-up.
**The failure mode was overstated.** The description said the double unmarshal asserts. `HTTPInfo::unmarshal` returns early on an already-unmarshaled heap ("must be a ram cache"), so under default config the shared-buffer mutation is masked. The observable failure is with `enable_checksum` on, where the recheck in `handleReadDone` walks the mutated bytes and marks the doc corrupt on every RAM hit after the first. The contract violation is real either way and the fix stands; the severity framing did not.
Also in the two new commits: the CLFUS resident re-put now refreshes `e->len` alongside `e->data` (defensive, matching the other two), `copy_data_in` allocates through `IOBufferData::alloc` instead of setting `_mem_type` by hand, and the test keys now spread across hash buckets -- they all landed in bucket 0 before. Two pre-existing items surfaced that I've left for separate PRs: `doc->len` isn't bounded against the read size before the put, and CLFUS's decompress-on-hit path charges `compressed_len` for an entry that now holds `len` bytes.
--
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