From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 5DBF31FF187 for ; Mon, 6 Oct 2025 15:18:37 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E327A4C47; Mon, 6 Oct 2025 15:18:40 +0200 (CEST) Date: Mon, 06 Oct 2025 15:18:04 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20251006104151.487202-1-c.ebner@proxmox.com> <20251006104151.487202-5-c.ebner@proxmox.com> In-Reply-To: <20251006104151.487202-5-c.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1759751384.9y4hkha6xh.astroid@yuna.none> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1759756658793 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.049 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup 4/7] chunk store: fix: replace evicted cache chunks instead of truncate X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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? > > Signed-off-by: Christian Ebner > --- > pbs-datastore/src/chunk_store.rs | 20 +++++++++++++++ > .../src/local_datastore_lru_cache.rs | 25 +++---------------- > 2 files changed, 23 insertions(+), 22 deletions(-) > > diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs > index 010785fbc..74fa79db1 100644 > --- a/pbs-datastore/src/chunk_store.rs > +++ b/pbs-datastore/src/chunk_store.rs > @@ -668,6 +668,26 @@ impl ChunkStore { > (chunk_path, digest_str) > } > > + /// Replace a chunk file with a zero size file in the chunk store. > + /// > + /// 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> { > + let (chunk_path, digest_str) = self.chunk_path(digest); > + let mut create_options = CreateOptions::new(); > + if nix::unistd::Uid::effective().is_root() { > + let uid = pbs_config::backup_user()?.uid; > + let gid = pbs_config::backup_group()?.gid; > + 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(()) > + } > + > pub fn relative_path(&self, path: &Path) -> PathBuf { > // unwrap: only `None` in unit tests > assert!(self.locker.is_some()); > diff --git a/pbs-datastore/src/local_datastore_lru_cache.rs b/pbs-datastore/src/local_datastore_lru_cache.rs > index 1d2e87cb9..6f950f4b3 100644 > --- a/pbs-datastore/src/local_datastore_lru_cache.rs > +++ b/pbs-datastore/src/local_datastore_lru_cache.rs > @@ -71,17 +71,8 @@ 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| { > - let (path, _digest_str) = self.store.chunk_path(&digest); > - // Truncate to free up space but keep the inode around, since that > - // is used as marker for chunks in use by garbage collection. > - if let Err(err) = nix::unistd::truncate(&path, 0) { > - if err != nix::errno::Errno::ENOENT { > - return Err(Error::from(err)); > - } > - } > - Ok(()) > - }) > + self.cache > + .insert(*digest, (), |digest| self.store.clear_chunk(&digest)) > } > > /// Remove a chunk from the local datastore cache. > @@ -104,17 +95,7 @@ impl LocalDatastoreLruCache { > ) -> Result, Error> { > if self > .cache > - .access(*digest, cacher, |digest| { > - let (path, _digest_str) = self.store.chunk_path(&digest); > - // Truncate to free up space but keep the inode around, since that > - // is used as marker for chunks in use by garbage collection. > - if let Err(err) = nix::unistd::truncate(&path, 0) { > - if err != nix::errno::Errno::ENOENT { > - return Err(Error::from(err)); > - } > - } > - Ok(()) > - }) > + .access(*digest, cacher, |digest| self.store.clear_chunk(&digest)) > .await? > .is_some() > { > -- > 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