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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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..

>          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?

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..

>                      Ok(Some(chunk))
>                  }
>                  // File was empty, might have been evicted since
> -- 
> 2.47.3
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 


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