Re: [PATCH v1 proxmox-backup 1/3] datastore: fix local path for .i.bad chunk files during S3 GC

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

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 <r.obkircher@proxmox.com>

Consider this patch:

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

> ---
>   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::<str>(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();