From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id E48741FF15C for ; Fri, 14 Nov 2025 14:18:48 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2429C13415; Fri, 14 Nov 2025 14:19:35 +0100 (CET) From: Christian Ebner To: pbs-devel@lists.proxmox.com Date: Fri, 14 Nov 2025 14:18:55 +0100 Message-ID: <20251114131901.441650-16-c.ebner@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20251114131901.441650-1-c.ebner@proxmox.com> References: <20251114131901.441650-1-c.ebner@proxmox.com> MIME-Version: 1.0 X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1763126340587 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.048 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: [pbs-devel] [PATCH proxmox-backup v6 15/21] GC: guard missing marker file insertion for s3 backed stores 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" The chunk marker file should only ever be missing if the local datastore cache has been recreated (e.g. after setup on a new PBS instance while reusing the s3 bucket contents) or by manual user interaction. Garbage collection does re-create the marker in these cases. Guard this marker file creation by the per-chunk file lock to not run into races with chunk insertion operations. Since this requires to stat the chunk marker file and locking is expensive, first check if the marker exists without holding a lock, only then lock and retry. Since the first chunk file existence check is already performed anyways move the logic to be within the non-existing branch thereof. By making this happen after touching potential bad chunks, this will allow to check these beforehand. Signed-off-by: Christian Ebner --- pbs-datastore/src/datastore.rs | 46 ++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs index d71106ea8..9c0ce9859 100644 --- a/pbs-datastore/src/datastore.rs +++ b/pbs-datastore/src/datastore.rs @@ -1323,36 +1323,40 @@ impl DataStore { } if !self.inner.chunk_store.cond_touch_chunk(digest, false)? { - let hex = hex::encode(digest); - warn!( - "warning: unable to access non-existent chunk {hex}, required by {file_name:?}" - ); - + let (chunk_path, _digest_str) = self.chunk_path(digest); // touch any corresponding .bad files to keep them around, meaning if a chunk is // rewritten correctly they will be removed automatically, as well as if no index // file requires the chunk anymore (won't get to this loop then) for i in 0..=9 { let bad_ext = format!("{i}.bad"); - let mut bad_path = PathBuf::new(); - bad_path.push(self.chunk_path(digest).0); + let mut bad_path = chunk_path.clone(); bad_path.set_extension(bad_ext); self.inner.chunk_store.cond_touch_path(&bad_path, false)?; } - } - if let Some(ref _s3_client) = s3_client { - // Update atime on local cache marker files. - if !self.inner.chunk_store.cond_touch_chunk(digest, false)? { - let (chunk_path, _digest) = self.chunk_path(digest); - // Insert empty file as marker to tell GC phase2 that this is - // a chunk still in-use, so to keep in the S3 object store. - std::fs::File::options() - .write(true) - .create_new(true) - .open(&chunk_path) - .with_context(|| { - format!("failed to create marker for chunk {}", hex::encode(digest)) - })?; + if let Some(ref _s3_client) = s3_client { + // Do not retry here, this is very unlikely to happen as chunk markers will + // most likely only be missing if the local cache store was recreated. + let _guard = self + .inner + .chunk_store + .lock_chunk(digest, CHUNK_LOCK_TIMEOUT)?; + if !self.inner.chunk_store.cond_touch_chunk(digest, false)? { + // Insert empty file as marker to tell GC phase2 that this is + // a chunk still in-use, so to keep in the S3 object store. + std::fs::File::options() + .write(true) + .create_new(true) + .open(&chunk_path) + .with_context(|| { + format!("failed to create marker for chunk {}", hex::encode(digest)) + })?; + } + } else { + let hex = hex::encode(digest); + warn!( + "warning: unable to access non-existent chunk {hex}, required by {file_name:?}" + ); } } } -- 2.47.3 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel