Skip to content

caching: scan the cache in steps, mtime fallback, more tests (follow-up to #223) - #226

Merged
ThomasWaldmann merged 3 commits into
borgbackup:mainfrom
ThomasWaldmann:cache-eviction-followup
Sep 20, 2026
Merged

ThomasWaldmann merged 3 commits into
borgbackup:mainfrom
ThomasWaldmann:cache-eviction-followup

Conversation

@ThomasWaldmann

Copy link
Copy Markdown
Member

Follow-up to #223 (#183), addressing findings of a critical review of that PR. 3 commits:

1. scan the cache in steps while the store is in use

A store that has put more than size / 4 bytes into a size limited namespace scans it again to see what other clients did. That scan listed the whole namespace within one store() / load() call, holding the store's lock - with a lot of cached items, all threads were blocked for seconds, and there was nothing in the logs about it.

Now each item that is put into the cache continues the scan for about 5 ms (CACHE_SCAN_STEP_TIME), the store uses the cache as usual in between. No threads, no lock changes - it relies on the same thing Store.list() already relies on (an open backend listing interleaved with other operations).

  • What the store changes while a scan is in progress is tracked (CacheScan.changed) and not overridden by what the scan has (or has not) seen: it might have listed an item before the store evicted it, or have listed a directory before the store added an item there. See CacheIndex.finish_scan.
  • The listing also yields the directories, so a step stays short if there are many directories, but few items.
  • If a scan found nothing that must be sorted into the index (as usual if the cache is not shared), the index is not rebuilt.
  • Store.open() and Store.close() still scan in one go. close() drops a scan in progress and starts over, so it sees the latest changes of other clients.
  • A finished scan logs (debug): item count, items added / removed by other clients, time, steps.

Longest single store() call while rescanning, levels [2], local APFS (same total run time):

cache main this PR
100k -> 200k items 4508 ms 114 ms
65k directories, 1000 items 1505 ms 15 ms

p99 is 5.5 ms (= the step time). What remains is finishing a scan (comparing the listing with the index) and one directory listing - a flat directory with 100k names still is one 390 ms step, because posixfs.list does sorted(path.iterdir()) (an os.scandir based list would be about 3x faster in general, not part of this PR).

4 processes hammering one shared cache for 8 s (68k verified loads): 0 wrong reads, 0 cache errors, 0 warnings, peak within size * (1 + N / 4), under size after the last close - also with a step time of 0 (one listing entry per step, scans interleaved with everything).

2. use the mtime if the cache backend has no atime

For items the store has not used itself, max_age and the LRU order depend on ItemInfo.atime. Without atime support (s3), max_age removed all these items and size evicted them in no particular order. ItemInfo.mtime (when the item was put into the cache) is a much better guess than 0. Docs: list which backends have atime / mtime (sftp has atime, too).

3. add missing tests, simplify the eviction loops

Found by mutating the code of #223:

  • no test noticed if a scan did not keep the more recent last access of a known item (own vs. listed) - new test, also covering a changed size.
  • no test noticed if a cache miss did not drop the index entry - new test (item deleted by another client from primary and cache).
  • the eviction loops removed the index entry although _cache_delete does that anyway - removed.
  • the test for failing evictions also passed without any eviction - it now checks that evictions really failed and that each one was counted.
  • new: size limit in mirror mode, failing scan while the store is in use, close() with a scan in progress, clients using a shared cache at the same time (threads, each with its own Store).

All 13 mutants of the new / changed code are caught by the tests now.

Docs: while multiple clients put items into a shared cache, its size usually is above the limit, not just temporarily (measured medians for clients that continuously insert: +14 % for 2, +40 % for 4, +62 % for 8 clients). It is within size again when the last of them has closed the store.

Not in this PR

No CHANGES.rst entry yet.

🤖 Generated with Claude Code

ThomasWaldmann and others added 3 commits September 20, 2026 01:29
A store that has put more than size / 4 bytes into a size limited cache
namespace scans it again to see what other clients did. That scan listed the
whole namespace within one store or load call, while holding the store's lock:
with a lot of cached items, that blocked all threads for seconds (measured:
4.5 s for 200k items in 65k directories) and there was no way to notice it.

Now each item that is put into the cache continues the scan for about 5 ms
(CACHE_SCAN_STEP_TIME) and the store uses the cache as usual in between. The
listing also yields the directories, so a step is short even if there are many
directories, but few items. What the store changes while a scan is in progress
is tracked and not overridden by what the scan has (or has not) seen, see
CacheIndex.finish_scan. If a scan did not find anything to sort into the index
(as usual for a cache that is not shared), the index is not rebuilt.

Store.open() and Store.close() still scan in one go.

A finished scan logs (debug level) the item count, what other clients have
added and removed, the time it took and the number of steps.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
For items the store has not used itself, max_age and the LRU order depend on
ItemInfo.atime. For a cache backend without atime support, that made max_age
remove all these items and size evict them in no particular order. The mtime
(the time when the item was put into the cache) is a much better guess than 0.

docs: list which backends have atime / mtime (sftp has atime, too).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Found by mutating the code: no test noticed if a scan did not keep the more
recent last access of a known item, nor if a cache miss did not drop the index
entry (that matters if the item is gone from the primary, too). The eviction
loops removed the index entry although _cache_delete does that anyway.

Also new: a size limit in mirror mode, clients using a shared cache at the same
time (threads, each one with its own Store). The test for failing evictions now
checks that evictions really have failed, it also passed without any eviction.

docs: while multiple clients put items into a shared cache, its size usually
is above the limit, not just temporarily (measured medians for clients that
continuously insert: +14 % for 2, +40 % for 4, +62 % for 8 clients).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@ThomasWaldmann
ThomasWaldmann merged commit 6fa3582 into borgbackup:main Sep 20, 2026
9 checks passed
@ThomasWaldmann
ThomasWaldmann deleted the cache-eviction-followup branch September 20, 2026 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant