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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
