Re: [pbs-devel] [RFC proxmox-backup] GC: fix deadlock for cache eviction and garbage collection

[Christian Ebner <c.ebner@proxmox.com>]

On 11/5/25 9:07 AM, Fabian Grünbichler wrote:
> On November 4, 2025 6:52 pm, Christian Ebner wrote:
>> When inserting a chunk via the local datastore cache, first the
>> chunk is inserted into the chunk store and then into the in-memory
>> AsyncLruCache. If the cache capacity is reached, the AsycLruCache
>> will execute a callback on the evicted cache node, which in case of
>> the local datastore cache performs a clear chunk call. For this
>> codepath, the AsyncLruCache is guarded by locking a mutex to get
>> exclusive access on the cache, and then the chunk store mutex guard
>> is acquired for safe clearing of the chunk.
>>
>> Garbage collection however tries the opposite if a chunk is no longer
>> present and should be cleaned up. It first guards the chunk store
>> mutex, only to then try and remove the chunk from the local chunk
>> store and the AsyncLruCache, the latter trying to guarantee
>> exclusive access by guarding its own mutex.
>>
>> This therefore can result in a deadlock, further locking the whole
>> chunk store.
>>
>> Fix this by locking the chunk store before even trying to clear the
>> chunk from the in-memory LRU cache, which is performed by the cache
>> eviction.
>>
>> Reported-by: https://forum.proxmox.com/threads/174878/
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>>   pbs-datastore/src/chunk_store.rs               |  6 +++---
>>   pbs-datastore/src/local_datastore_lru_cache.rs | 14 ++++++++++----
>>   2 files changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
>> index ba7618e40..f9f13ec87 100644
>> --- a/pbs-datastore/src/chunk_store.rs
>> +++ b/pbs-datastore/src/chunk_store.rs
>> @@ -667,7 +667,9 @@ impl ChunkStore {
>>       ///
>>       /// Used to evict chunks from the local datastore cache, while keeping them as in-use markers
>>       /// for garbage collection. Returns with success also if chunk file is not pre-existing.
>> -    pub fn clear_chunk(&self, digest: &[u8; 32]) -> Result<(), Error> {
>> +    ///
>> +    /// Caller must hold the chunk store mutex lock.
>> +    pub unsafe fn clear_chunk(&self, digest: &[u8; 32]) -> Result<(), Error> {
> 
> since this is only used by the local cache, should we maybe take the
> opportunity and move the fn there and make it private?
> 
> *all access* must go over the cache, including clearing chunks, so
> exposing it on its own (even as unsafe) seems like a footgun..

Hmm, not ideal either.. After all this was just moved [0] from exactly 
there to be a method of the chunk store to safely guard it there (would 
have been better to expose it as pub(crate) however). I do think that it 
makes sense that every interaction with the chunk store should use an 
interface of ChunkStore. The LocalDatastoreLRUCache just sits on top of 
that.

I rather think there needs to be a better way of how to handle the Mutex 
locking.

Maybe it is better to solve this on the other end then? Meaning 
reworking the locking on the garbage collection side. With [1] applied,
this should not require to lock the chunk store anymore unless we remove 
an item from the cache, which could however be handled inside the chunk 
store instead of directly in the LocalDatastoreLRUCache::remove(), which 
is another chunk store interaction which should better be implemented as 
interface to the chunk store itself rather than the cache.

> 
>>           let (chunk_path, digest_str) = self.chunk_path(digest);
>>           let mut create_options = CreateOptions::new();
>>           if nix::unistd::Uid::effective().is_root() {
>> @@ -676,8 +678,6 @@ impl ChunkStore {
>>               create_options = create_options.owner(uid).group(gid);
>>           }
>>   
>> -        let _lock = self.mutex.lock();
>> -
>>           proxmox_sys::fs::replace_file(&chunk_path, &[], create_options, false)
>>               .map_err(|err| format_err!("clear chunk failed for {digest_str} - {err}"))?;
>>           Ok(())
>> diff --git a/pbs-datastore/src/local_datastore_lru_cache.rs b/pbs-datastore/src/local_datastore_lru_cache.rs
>> index fe3b51a55..2cab6b83d 100644
>> --- a/pbs-datastore/src/local_datastore_lru_cache.rs
>> +++ b/pbs-datastore/src/local_datastore_lru_cache.rs
>> @@ -35,8 +35,11 @@ impl LocalDatastoreLruCache {
>>       /// Fails if the chunk cannot be inserted successfully.
>>       pub fn insert(&self, digest: &[u8; 32], chunk: &DataBlob) -> Result<(), Error> {
>>           self.store.insert_chunk(chunk, digest)?;
>> -        self.cache
>> -            .insert(*digest, (), |digest| self.store.clear_chunk(&digest))
>> +        let _lock = self.store.mutex().lock();
>> +        self.cache.insert(*digest, (), |digest| unsafe {
>> +            // unsafe condition satisfied, holding chunk store mutex guard
>> +            self.store.clear_chunk(&digest)
>> +        })
>>       }
>>   
>>       /// Remove a chunk from the local datastore cache.
>> @@ -71,8 +74,11 @@ impl LocalDatastoreLruCache {
>>               Ok(mut file) => match DataBlob::load_from_reader(&mut file) {
>>                   // File was still cached with contents, load response from file
>>                   Ok(chunk) => {
>> -                    self.cache
>> -                        .insert(*digest, (), |digest| self.store.clear_chunk(&digest))?;
>> +                    let _lock = self.store.mutex().lock();
>> +                    self.cache.insert(*digest, (), |digest| unsafe {
>> +                        // unsafe condition satisfied, holding chunk store mutex guard
>> +                        self.store.clear_chunk(&digest)
>> +                    })?;
> 
> I was initially a bit confused here, because at first glance it looks
> like the callback is stored and later called when evicting *this*
> digest (which of course would violate the invariant described in the
> comment), as opposed to it being called directly inline, in case the
> insertion evicts something now (which is the case, and is fine!)..
> 
> could be more readable if the closure takes something like
> `eviced_digest` maybe?
True, renaming this will make it clear that this is the evicted cache node.

> 
> or even better - the eviction callback could become part of the cache
> itself, instead of being passed on every insertion, since it is a global
> property of the cache.. although that would hide the unsafe in this
> case, which would then infect every insert call without being visible..
> 
> this is kinda annoying..

Yes, this is really frustrating and incompatible invocations easily missed.

[0] 
https://git.proxmox.com/?p=proxmox-backup.git;a=commit;h=4ed3710556defb71245055c35e0e577ec895b6bb
[1] 
https://lore.proxmox.com/pbs-devel/20251103113120.239455-16-c.ebner@proxmox.com/T/#u


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel