From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 39D271FF13A for ; Wed, 22 Jul 2026 16:22:31 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 0BCF4214C3; Wed, 22 Jul 2026 16:22:31 +0200 (CEST) Message-ID: Date: Wed, 22 Jul 2026 16:21:55 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Christian Ebner Subject: Re: [PATCH v1 proxmox-backup 1/3] datastore: fix local path for .i.bad chunk files during S3 GC To: Robert Obkircher , pbs-devel@lists.proxmox.com References: <20260713132338.170565-1-r.obkircher@proxmox.com> <20260713132338.170565-2-r.obkircher@proxmox.com> Content-Language: en-US, de-DE In-Reply-To: <20260713132338.170565-2-r.obkircher@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784730087668 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.181 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: DGYCVHB4SIAWCJVCMXTLDIQ3NCA7M2LI X-Message-ID-Hash: DGYCVHB4SIAWCJVCMXTLDIQ3NCA7M2LI X-MailFrom: c.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 7/13/26 3:24 PM, Robert Obkircher wrote: > Do not push an additional path segment with the extension, and rename > the confusing variable. The additional segment caused a rather cryptic > "TASK ERROR: Not a directory (os error 20)" error during Phase 2 if > the .i.bad file existed locally. > > If it doesn't exist locally, the S3 object was and continues to be > deleted, unless it is the very first one and the corresponding chunk > expected marker exists. This behavior might not be desirable if the > files are missing because the datastore was recreated with the "reuse > existing datastore" option. This is not desired and should be fixed as well. The chunk expected marker already is set during phase 1 of GC based on the encountered digest from the index file (if there is still one referencing it, otherwise the bad object can be dropped safely). During phase 2 when the marker is replaced by replace_chunk_with_marker_or_create_marker() this should then also get the respective extension to create the local marker file for this object. If the good chunk object exists as well or is recreated by backup uploads, this will then lead to the bad marker and object being cleaned up by future GC runs as intended. > > Fixes: 27e3f5b7b ("GC: fix: don't drop bad extension for S3 object to chunk path helper") > Signed-off-by: Robert Obkircher Consider this patch: Reviewed-by: Christian Ebner > --- > pbs-datastore/src/datastore.rs | 14 +++++--------- > 1 file changed, 5 insertions(+), 9 deletions(-) > > diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs > index e2d1ae67c..3079fe833 100644 > --- a/pbs-datastore/src/datastore.rs > +++ b/pbs-datastore/src/datastore.rs > @@ -2678,8 +2678,8 @@ impl DataStore { > // Check object is actually a chunk > let path = Path::new::(object_key); > // file_name() should always be Some, as objects will have a filename > - let digest = path.file_name()?; > - let bytes = digest.as_bytes(); > + let file_name = path.file_name()?; > + let bytes = file_name.as_bytes(); > let bad_ext_len = ".0.bad".len(); > let bad_chunk = if bytes.len() == 64 + bad_ext_len { > true > @@ -2693,19 +2693,15 @@ impl DataStore { > } > > // Safe since contains valid ascii hexdigits only as checked above. > - let digest_str = digest.to_string_lossy(); > + let digest_str = file_name.to_string_lossy(); > let hexdigit_prefix = unsafe { digest_str.get_unchecked(0..4) }; > let mut chunk_path = self.base_path(); > chunk_path.push(".chunks"); > chunk_path.push(hexdigit_prefix); > - chunk_path.push(digest); > - if bad_chunk { > - let extension = unsafe { digest_str.get_unchecked(64..64 + bad_ext_len) }; > - chunk_path.push(extension); > - } > + chunk_path.push(file_name); > > let mut digest_bytes = [0u8; 32]; > - let digest = digest.as_bytes(); > + let digest = file_name.as_bytes(); nit: pre-existing and only mentioning this for completeness as it gets replaced in patch 3 anyways, this could be dropped in favor of using `bytes` from above directly. > // safe to unwrap as already checked above > hex::decode_to_slice(&digest[..64], &mut digest_bytes).unwrap();