* [pbs-devel] [PATCH proxmox-backup v2 1/4] GC: s3: fix local marker cleanup for unreferenced, s3 only chunks
2025-11-24 9:40 [pbs-devel] [PATCH proxmox-backup v2 0/4] fix local marker cleanup for unreferenced, s3 only chunks Christian Ebner
@ 2025-11-24 9:40 ` Christian Ebner
2025-11-24 9:40 ` [pbs-devel] [PATCH proxmox-backup v2 2/4] chunk store: fix and expand the clear_chunk_expected_mark() docstring Christian Ebner
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Christian Ebner @ 2025-11-24 9:40 UTC (permalink / raw)
To: pbs-devel
If a chunk object is located on the s3 object store only, not being
referenced by any index file and having no local marker file it was
marked for cleanup by pretending an atime equal to the unix epoch.
While this will mark the chunk for deletion from the backend and
include it in the delete list for the next s3 delete objects call, it
also will lead to the chunk marker and LRU cache entry being tried to
clean up locally, which however failed since there is no marker to be
cleaned up.
In order to treat this edge case, instead of pretending an atime
equal to the unix epoch, make the atime optional and skip over the
atime check for that case altogether, directly pushing the object and
its guard to the delete list, while updating the gc status
accordingly.
Fixes: https://forum.proxmox.com/threads/176567/
Originally-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
Changes since version 1:
- Skip over cond_sweep_chunk altogether if the marker is not present and
chunk no longer required
pbs-datastore/src/datastore.rs | 68 +++++++++++++++++++---------------
1 file changed, 39 insertions(+), 29 deletions(-)
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 65299cca9..1b489b449 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1709,48 +1709,58 @@ impl DataStore {
// Check local markers (created or atime updated during phase1) and
// keep or delete chunk based on that.
let atime = match std::fs::metadata(&chunk_path) {
- Ok(stat) => stat.accessed()?,
+ Ok(stat) => Some(stat.accessed()?),
Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
if self.inner.chunk_store.clear_chunk_expected_mark(&digest)? {
unsafe {
// chunk store lock held
self.inner.chunk_store.replace_chunk_with_marker(&digest)?;
}
- SystemTime::now()
+ Some(SystemTime::now())
} else {
- // File not found, delete by setting atime to unix epoch
- SystemTime::UNIX_EPOCH
+ // File not found, only delete from S3
+ None
}
}
Err(err) => return Err(err.into()),
};
- let atime = atime.duration_since(SystemTime::UNIX_EPOCH)?.as_secs() as i64;
-
- unsafe {
- self.inner.chunk_store.cond_sweep_chunk(
- atime,
- min_atime,
- oldest_writer,
- content.size,
- bad,
- &mut gc_status,
- || {
- if let Some(cache) = self.cache() {
- if !bad {
- cache.remove(&digest)?;
- } else {
- std::fs::remove_file(chunk_path)?;
+ if let Some(atime) = atime {
+ let atime = atime.duration_since(SystemTime::UNIX_EPOCH)?.as_secs() as i64;
+
+ unsafe {
+ self.inner.chunk_store.cond_sweep_chunk(
+ atime,
+ min_atime,
+ oldest_writer,
+ content.size,
+ bad,
+ &mut gc_status,
+ || {
+ if let Some(cache) = self.cache() {
+ if !bad {
+ cache.remove(&digest)?;
+ } else {
+ std::fs::remove_file(chunk_path)?;
+ }
}
- }
- // set age based on first insertion
- if delete_list.is_empty() {
- delete_list_age = epoch_i64();
- }
- delete_list.push((content.key, _chunk_guard));
- Ok(())
- },
- )?;
+ // set age based on first insertion
+ if delete_list.is_empty() {
+ delete_list_age = epoch_i64();
+ }
+ delete_list.push((content.key, _chunk_guard));
+ Ok(())
+ },
+ )?;
+ }
+ } else {
+ gc_status.removed_chunks += 1;
+ gc_status.removed_bytes += content.size;
+ // set age based on first insertion
+ if delete_list.is_empty() {
+ delete_list_age = epoch_i64();
+ }
+ delete_list.push((content.key, _chunk_guard));
}
chunk_count += 1;
--
2.47.3
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* [pbs-devel] [PATCH proxmox-backup v2 2/4] chunk store: fix and expand the clear_chunk_expected_mark() docstring
2025-11-24 9:40 [pbs-devel] [PATCH proxmox-backup v2 0/4] fix local marker cleanup for unreferenced, s3 only chunks Christian Ebner
2025-11-24 9:40 ` [pbs-devel] [PATCH proxmox-backup v2 1/4] GC: s3: " Christian Ebner
@ 2025-11-24 9:40 ` Christian Ebner
2025-11-24 9:40 ` [pbs-devel] [PATCH proxmox-backup v2 3/4] chunk store: clarify chunk marker helper creates marker if missing Christian Ebner
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Christian Ebner @ 2025-11-24 9:40 UTC (permalink / raw)
To: pbs-devel
The sentence was grammatically wrong and did not provide to much
context on what this is used for. Fix and extend the docstring.
Reported-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
Changes since version 1:
- not present
pbs-datastore/src/chunk_store.rs | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index f10415d1f..143848838 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -749,7 +749,12 @@ impl ChunkStore {
Ok(())
}
- /// Remove chunk as required mark by removing file the chunk store.
+ /// Remove the chunk-expected marker file from the chunk store.
+ ///
+ /// Used to remove the chunk-expected marker file during phase 2 of garbage collection. This
+ /// marker file is created during phase 1 of garbage collection in case the chunk or its zero
+ /// size marker file is not found in the chunk store, but still referenced by an index file,
+ /// flagging it as still required.
///
/// Returns true if the file was present and removed, false if the file did not exist.
pub(crate) fn clear_chunk_expected_mark(&self, digest: &[u8; 32]) -> Result<bool, Error> {
--
2.47.3
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* [pbs-devel] [PATCH proxmox-backup v2 3/4] chunk store: clarify chunk marker helper creates marker if missing
2025-11-24 9:40 [pbs-devel] [PATCH proxmox-backup v2 0/4] fix local marker cleanup for unreferenced, s3 only chunks Christian Ebner
2025-11-24 9:40 ` [pbs-devel] [PATCH proxmox-backup v2 1/4] GC: s3: " Christian Ebner
2025-11-24 9:40 ` [pbs-devel] [PATCH proxmox-backup v2 2/4] chunk store: fix and expand the clear_chunk_expected_mark() docstring Christian Ebner
@ 2025-11-24 9:40 ` Christian Ebner
2025-11-24 9:40 ` [pbs-devel] [PATCH proxmox-backup v2 4/4] datastore: refactor common delete list logic into closure Christian Ebner
2025-11-24 12:23 ` [pbs-devel] applied: [PATCH proxmox-backup v2 0/4] fix local marker cleanup for unreferenced, s3 only chunks Thomas Lamprecht
4 siblings, 0 replies; 7+ messages in thread
From: Christian Ebner @ 2025-11-24 9:40 UTC (permalink / raw)
To: pbs-devel
Rename replace_chunk_with_marker() to
replace_chunk_with_marker_or_create_marker() and adapt the docstring
so it is clear that this also creates the marker if the chunk file
is missing.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
Changes since version 1:
- not present
pbs-datastore/src/chunk_store.rs | 8 ++++++--
pbs-datastore/src/datastore.rs | 4 +++-
pbs-datastore/src/local_datastore_lru_cache.rs | 10 ++++++++--
3 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 143848838..f53460664 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -716,10 +716,14 @@ impl ChunkStore {
/// 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.
+ /// for garbage collection. Returns with success also if chunk file is not pre-existing,
+ /// also creating the marker file in that case.
///
/// Safety: chunk store mutex must be held!
- pub(crate) unsafe fn replace_chunk_with_marker(&self, digest: &[u8; 32]) -> Result<(), Error> {
+ pub(crate) unsafe fn replace_chunk_with_marker_or_create_marker(
+ &self,
+ digest: &[u8; 32],
+ ) -> Result<(), Error> {
let (chunk_path, digest_str) = self.chunk_path(digest);
Self::create_marker_file(&chunk_path)
.map_err(|err| format_err!("clear chunk failed for {digest_str} - {err}"))?;
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 1b489b449..d88785c6d 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1714,7 +1714,9 @@ impl DataStore {
if self.inner.chunk_store.clear_chunk_expected_mark(&digest)? {
unsafe {
// chunk store lock held
- self.inner.chunk_store.replace_chunk_with_marker(&digest)?;
+ self.inner
+ .chunk_store
+ .replace_chunk_with_marker_or_create_marker(&digest)?;
}
Some(SystemTime::now())
} else {
diff --git a/pbs-datastore/src/local_datastore_lru_cache.rs b/pbs-datastore/src/local_datastore_lru_cache.rs
index ae2215be1..f39b26ebb 100644
--- a/pbs-datastore/src/local_datastore_lru_cache.rs
+++ b/pbs-datastore/src/local_datastore_lru_cache.rs
@@ -42,7 +42,10 @@ impl LocalDatastoreLruCache {
}
self.cache.insert(*digest, (), |digest| {
// Safety: lock acquired above, this is executed inline!
- unsafe { self.store.replace_chunk_with_marker(&digest) }
+ unsafe {
+ self.store
+ .replace_chunk_with_marker_or_create_marker(&digest)
+ }
})
}
@@ -80,7 +83,10 @@ impl LocalDatastoreLruCache {
let _lock = self.store.mutex().lock().unwrap();
self.cache.insert(*digest, (), |digest| {
// Safety: lock acquired above, this is executed inline
- unsafe { self.store.replace_chunk_with_marker(&digest) }
+ unsafe {
+ self.store
+ .replace_chunk_with_marker_or_create_marker(&digest)
+ }
})?;
Ok(Some(chunk))
}
--
2.47.3
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* [pbs-devel] [PATCH proxmox-backup v2 4/4] datastore: refactor common delete list logic into closure
2025-11-24 9:40 [pbs-devel] [PATCH proxmox-backup v2 0/4] fix local marker cleanup for unreferenced, s3 only chunks Christian Ebner
` (2 preceding siblings ...)
2025-11-24 9:40 ` [pbs-devel] [PATCH proxmox-backup v2 3/4] chunk store: clarify chunk marker helper creates marker if missing Christian Ebner
@ 2025-11-24 9:40 ` Christian Ebner
2025-11-24 12:23 ` [pbs-devel] applied: [PATCH proxmox-backup v2 0/4] fix local marker cleanup for unreferenced, s3 only chunks Thomas Lamprecht
4 siblings, 0 replies; 7+ messages in thread
From: Christian Ebner @ 2025-11-24 9:40 UTC (permalink / raw)
To: pbs-devel
Reduce code duplication by defining a dedicated closure to update
the delete list age and push file lock guarded chunk object keys to
it.
No functional changes intended.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
Changes since version 1:
- not present
pbs-datastore/src/datastore.rs | 36 +++++++++++++++++++++++-----------
1 file changed, 25 insertions(+), 11 deletions(-)
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index d88785c6d..077b199e6 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1691,6 +1691,19 @@ impl DataStore {
delete_list.clear();
Ok(())
};
+
+ let add_to_delete_list =
+ |delete_list: &mut Vec<(S3ObjectKey, BackupLockGuard)>,
+ delete_list_age: &mut i64,
+ key: S3ObjectKey,
+ _chunk_guard: BackupLockGuard| {
+ // set age based on first insertion
+ if delete_list.is_empty() {
+ *delete_list_age = epoch_i64();
+ }
+ delete_list.push((key, _chunk_guard));
+ };
+
loop {
for content in list_bucket_result.contents {
let (chunk_path, digest, bad) =
@@ -1745,12 +1758,12 @@ impl DataStore {
std::fs::remove_file(chunk_path)?;
}
}
-
- // set age based on first insertion
- if delete_list.is_empty() {
- delete_list_age = epoch_i64();
- }
- delete_list.push((content.key, _chunk_guard));
+ add_to_delete_list(
+ &mut delete_list,
+ &mut delete_list_age,
+ content.key,
+ _chunk_guard,
+ );
Ok(())
},
)?;
@@ -1758,11 +1771,12 @@ impl DataStore {
} else {
gc_status.removed_chunks += 1;
gc_status.removed_bytes += content.size;
- // set age based on first insertion
- if delete_list.is_empty() {
- delete_list_age = epoch_i64();
- }
- delete_list.push((content.key, _chunk_guard));
+ add_to_delete_list(
+ &mut delete_list,
+ &mut delete_list_age,
+ content.key,
+ _chunk_guard,
+ );
}
chunk_count += 1;
--
2.47.3
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* [pbs-devel] applied: [PATCH proxmox-backup v2 0/4] fix local marker cleanup for unreferenced, s3 only chunks
2025-11-24 9:40 [pbs-devel] [PATCH proxmox-backup v2 0/4] fix local marker cleanup for unreferenced, s3 only chunks Christian Ebner
` (3 preceding siblings ...)
2025-11-24 9:40 ` [pbs-devel] [PATCH proxmox-backup v2 4/4] datastore: refactor common delete list logic into closure Christian Ebner
@ 2025-11-24 12:23 ` Thomas Lamprecht
2025-11-24 12:51 ` Christian Ebner
4 siblings, 1 reply; 7+ messages in thread
From: Thomas Lamprecht @ 2025-11-24 12:23 UTC (permalink / raw)
To: pbs-devel, Christian Ebner
On Mon, 24 Nov 2025 10:40:14 +0100, Christian Ebner wrote:
> These patches fix an issue with garbage collection on S3 backends for an
> edge case where the local chunk marker is missing and the chunk not
> being referenced anymore by any index file. In that case garbage
> collection failed when trying to remove the missing marker from the
> local datastore cache.
>
> Fix this by adapting the phase 2 logic such that the atime is now
> optional, skipping over the chunk sweep logic if not set and adding the
> chunk directly for deletion.
>
> [...]
Applied, thanks!
While I applied the last patch already as the intentions behind it are
worthwhile to already have, it's IMO not ideal and the function here is already
quite crowded. Maybe creating a local wrapper struc/type around the delete Vec
and age variables and implementing this logic on that struct would be nicer?
In any case, really not important for now, in fact, I probably won't even
consider applying such patches this week.
[1/4] GC: s3: fix local marker cleanup for unreferenced, s3 only chunks
commit: e22be89d6d3f8113fc28a47e08600c759917f43d
[2/4] chunk store: fix and expand the clear_chunk_expected_mark() docstring
commit: d8499811f3cb338f02ccce1beeabd81c57fc129c
[3/4] chunk store: clarify chunk marker helper creates marker if missing
commit: bf279c52b5b8dc82093ad26cdf35ddf8b1b68fe1
[4/4] datastore: refactor common delete list logic into closure
commit: ed2ae6d3fbbeb19a19a57e9ed7d6ed7e59c22ba0
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [pbs-devel] applied: [PATCH proxmox-backup v2 0/4] fix local marker cleanup for unreferenced, s3 only chunks
2025-11-24 12:23 ` [pbs-devel] applied: [PATCH proxmox-backup v2 0/4] fix local marker cleanup for unreferenced, s3 only chunks Thomas Lamprecht
@ 2025-11-24 12:51 ` Christian Ebner
0 siblings, 0 replies; 7+ messages in thread
From: Christian Ebner @ 2025-11-24 12:51 UTC (permalink / raw)
To: Thomas Lamprecht, pbs-devel
On 11/24/25 1:27 PM, Thomas Lamprecht wrote:
> On Mon, 24 Nov 2025 10:40:14 +0100, Christian Ebner wrote:
>> These patches fix an issue with garbage collection on S3 backends for an
>> edge case where the local chunk marker is missing and the chunk not
>> being referenced anymore by any index file. In that case garbage
>> collection failed when trying to remove the missing marker from the
>> local datastore cache.
>>
>> Fix this by adapting the phase 2 logic such that the atime is now
>> optional, skipping over the chunk sweep logic if not set and adding the
>> chunk directly for deletion.
>>
>> [...]
>
> Applied, thanks!
>
> While I applied the last patch already as the intentions behind it are
> worthwhile to already have, it's IMO not ideal and the function here is already
> quite crowded. Maybe creating a local wrapper struc/type around the delete Vec
> and age variables and implementing this logic on that struct would be nicer?
Yes, bundling all the logic regarding the delete list, including
creation and age checks, ecc. into a dedicated struct with its helpers
should help de-clutter this a bit. Will have a go at that, thanks!
> In any case, really not important for now, in fact, I probably won't even
> consider applying such patches this week.
>
> [1/4] GC: s3: fix local marker cleanup for unreferenced, s3 only chunks
> commit: e22be89d6d3f8113fc28a47e08600c759917f43d
> [2/4] chunk store: fix and expand the clear_chunk_expected_mark() docstring
> commit: d8499811f3cb338f02ccce1beeabd81c57fc129c
> [3/4] chunk store: clarify chunk marker helper creates marker if missing
> commit: bf279c52b5b8dc82093ad26cdf35ddf8b1b68fe1
> [4/4] datastore: refactor common delete list logic into closure
> commit: ed2ae6d3fbbeb19a19a57e9ed7d6ed7e59c22ba0
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 7+ messages in thread