[pbs-devel] [PATCH proxmox-backup 4/7] chunk store: fix: replace evicted cache chunks instead of truncate

[Christian Ebner]

On 10/6/25 5:35 PM, Christian Ebner wrote:
> On 10/6/25 3:18 PM, Fabian Grünbichler wrote:
>> On October 6, 2025 12:41 pm, Christian Ebner wrote:
>>> Evicted chunks have been truncated to size zero, keeping the chunk
>>> file in place as in-use marker for the garbage collection but freeing
>>> the chunk file contents. This can however lead to restores failing if
>>> they already opened the chunk file for reading, as their contents are
>>> now incomplete.
>>>
>>> Fix this by instead replacing the chunk file with a zero sized file,
>>> leaving the contents accessible for the already opened chunk readers.
>>>
>>> By moving the logic from the local datastore cache to a helper method
>>> on the chunk store, it is also assured that the operation is guarded
>>> by the chunk store mutex lock to avoid races with chunk re-insert.
>>
>> AFAICT this is still racy even after this patch, because in cache.access
>> after a cache miss (either no local file, or empty local file) we fetch
>> the chunk from S3, insert it into the chunk store (cache), but then
>> instead of returning the chunk from the already in-memory chunk data, we
>> load it again from the path - without holding the lock that prevents the
>> chunk from being evicted again.. while unlikely to be hit in practice,
>> this is still wasteful because we could save a round-trip and just
>> return the chunk we've already constructed from the S3 response and hit
>> two birds with one stone?
> 
> Good catch! Indeed we can serve the response from the already in-memory 
> data in that case.

Actually it is more subtle, as I cannot pass the response from the 
fetcher along to the access implementation of the 
LocalDatastoreLruCache. But I will rewrite the implementation for that,
making the fetcher basically a dummy and moving the insert logic to the 
access method instead.