* [pbs-devel] [PATCH proxmox-backup v2 1/3] GC: refactor chunk removal helper
2025-10-15 10:13 [pbs-devel] [PATCH proxmox-backup v2 0/3] GC refactor follow-ups Fabian Grünbichler
@ 2025-10-15 10:13 ` Fabian Grünbichler
2025-10-15 10:13 ` [pbs-devel] [PATCH proxmox-backup v2 2/3] GC: rename helper to cond_sweep_chunk Fabian Grünbichler
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Fabian Grünbichler @ 2025-10-15 10:13 UTC (permalink / raw)
To: pbs-devel
simplify the callback, and move the error handling to the helper..
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
Notes:
v2: keep error propagation identical
pbs-datastore/src/chunk_store.rs | 27 ++++++++++++---------------
pbs-datastore/src/datastore.rs | 2 +-
2 files changed, 13 insertions(+), 16 deletions(-)
diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 6e50327cb..f8e5457b7 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -415,19 +415,13 @@ impl ChunkStore {
stat.st_size as u64,
bad,
status,
- |status| {
- if let Err(err) =
- unlinkat(Some(dirfd), filename, UnlinkatFlags::NoRemoveDir)
- {
- if bad {
- status.still_bad += 1;
- }
- bail!(
+ || {
+ unlinkat(Some(dirfd), filename, UnlinkatFlags::NoRemoveDir).map_err(|err| {
+ format_err!(
"unlinking chunk {filename:?} failed on store '{}' - {err}",
self.name,
- );
- }
- Ok(())
+ )
+ })
},
)?;
}
@@ -441,9 +435,7 @@ impl ChunkStore {
/// status accordingly.
///
/// If the chunk should be removed, the [`remove_callback`] is executed.
- pub(super) fn check_atime_and_update_gc_status<
- T: FnOnce(&mut GarbageCollectionStatus) -> Result<(), Error>,
- >(
+ pub(super) fn check_atime_and_update_gc_status<T: FnOnce() -> Result<(), Error>>(
atime: i64,
min_atime: i64,
oldest_writer: i64,
@@ -453,7 +445,12 @@ impl ChunkStore {
remove_callback: T,
) -> Result<(), Error> {
if atime < min_atime {
- remove_callback(gc_status)?;
+ if let Err(err) = remove_callback() {
+ if bad {
+ gc_status.still_bad += 1;
+ }
+ return Err(err);
+ }
if bad {
gc_status.removed_bad += 1;
} else {
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index a6b17e3c3..21998a157 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1671,7 +1671,7 @@ impl DataStore {
content.size,
bad,
&mut gc_status,
- |_status| {
+ || {
if let Some(cache) = self.cache() {
// ignore errors, phase 3 will retry cleanup anyways
let _ = unsafe { cache.remove(&digest) };
--
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] 6+ messages in thread* [pbs-devel] [PATCH proxmox-backup v2 2/3] GC: rename helper to cond_sweep_chunk
2025-10-15 10:13 [pbs-devel] [PATCH proxmox-backup v2 0/3] GC refactor follow-ups Fabian Grünbichler
2025-10-15 10:13 ` [pbs-devel] [PATCH proxmox-backup v2 1/3] GC: refactor chunk removal helper Fabian Grünbichler
@ 2025-10-15 10:13 ` Fabian Grünbichler
2025-10-15 10:13 ` [pbs-devel] [PATCH proxmox-backup v2 3/3] GC: mark cond_sweep_chunk helper as unafe Fabian Grünbichler
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Fabian Grünbichler @ 2025-10-15 10:13 UTC (permalink / raw)
To: pbs-devel
and make it take self, to make it more clear that the chunk store should be
locked at this point.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
---
pbs-datastore/src/chunk_store.rs | 5 +++--
pbs-datastore/src/datastore.rs | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index f8e5457b7..065eb4a08 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -408,7 +408,7 @@ impl ChunkStore {
chunk_count += 1;
- Self::check_atime_and_update_gc_status(
+ self.cond_sweep_chunk(
stat.st_atime,
min_atime,
oldest_writer,
@@ -435,7 +435,8 @@ impl ChunkStore {
/// status accordingly.
///
/// If the chunk should be removed, the [`remove_callback`] is executed.
- pub(super) fn check_atime_and_update_gc_status<T: FnOnce() -> Result<(), Error>>(
+ pub(super) fn cond_sweep_chunk<T: FnOnce() -> Result<(), Error>>(
+ &self,
atime: i64,
min_atime: i64,
oldest_writer: i64,
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 21998a157..f1237af32 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1664,7 +1664,7 @@ impl DataStore {
.extension()
.is_some_and(|ext| ext == "bad");
- ChunkStore::check_atime_and_update_gc_status(
+ self.inner.chunk_store.cond_sweep_chunk(
atime,
min_atime,
oldest_writer,
--
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] 6+ messages in thread* [pbs-devel] [PATCH proxmox-backup v2 3/3] GC: mark cond_sweep_chunk helper as unafe
2025-10-15 10:13 [pbs-devel] [PATCH proxmox-backup v2 0/3] GC refactor follow-ups Fabian Grünbichler
2025-10-15 10:13 ` [pbs-devel] [PATCH proxmox-backup v2 1/3] GC: refactor chunk removal helper Fabian Grünbichler
2025-10-15 10:13 ` [pbs-devel] [PATCH proxmox-backup v2 2/3] GC: rename helper to cond_sweep_chunk Fabian Grünbichler
@ 2025-10-15 10:13 ` Fabian Grünbichler
2025-10-15 10:50 ` [pbs-devel] [PATCH proxmox-backup v2 0/3] GC refactor follow-ups Christian Ebner
2025-10-15 12:57 ` [pbs-devel] applied-series: " Fabian Grünbichler
4 siblings, 0 replies; 6+ messages in thread
From: Fabian Grünbichler @ 2025-10-15 10:13 UTC (permalink / raw)
To: pbs-devel
it's only allowed to be called in GC context with all the prerequisites like
locking, ensuring proper atime calculation, etc.
ideally, this should be internal to the chunk store, but with S3 the boundaries
between datastore, chunk cache and chunk store became rather blurry, and that
probably requires more refactoring of the interactions..
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
---
best viewed with -w
pbs-datastore/src/chunk_store.rs | 39 +++++++++++++++++++-------------
pbs-datastore/src/datastore.rs | 34 +++++++++++++++-------------
2 files changed, 41 insertions(+), 32 deletions(-)
diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 065eb4a08..de34b3323 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -408,22 +408,26 @@ impl ChunkStore {
chunk_count += 1;
- self.cond_sweep_chunk(
- stat.st_atime,
- min_atime,
- oldest_writer,
- stat.st_size as u64,
- bad,
- status,
- || {
- unlinkat(Some(dirfd), filename, UnlinkatFlags::NoRemoveDir).map_err(|err| {
- format_err!(
- "unlinking chunk {filename:?} failed on store '{}' - {err}",
- self.name,
+ unsafe {
+ self.cond_sweep_chunk(
+ stat.st_atime,
+ min_atime,
+ oldest_writer,
+ stat.st_size as u64,
+ bad,
+ status,
+ || {
+ unlinkat(Some(dirfd), filename, UnlinkatFlags::NoRemoveDir).map_err(
+ |err| {
+ format_err!(
+ "unlinking chunk {filename:?} failed on store '{}' - {err}",
+ self.name,
+ )
+ },
)
- })
- },
- )?;
+ },
+ )?;
+ }
}
drop(lock);
}
@@ -435,7 +439,10 @@ impl ChunkStore {
/// status accordingly.
///
/// If the chunk should be removed, the [`remove_callback`] is executed.
- pub(super) fn cond_sweep_chunk<T: FnOnce() -> Result<(), Error>>(
+ ///
+ /// Unsafe: requires locking and GC checks to be called
+ /// FIXME: make this internal with further refactoring
+ pub(super) unsafe fn cond_sweep_chunk<T: FnOnce() -> Result<(), Error>>(
&self,
atime: i64,
min_atime: i64,
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index f1237af32..038306166 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1664,22 +1664,24 @@ impl DataStore {
.extension()
.is_some_and(|ext| ext == "bad");
- self.inner.chunk_store.cond_sweep_chunk(
- atime,
- min_atime,
- oldest_writer,
- content.size,
- bad,
- &mut gc_status,
- || {
- if let Some(cache) = self.cache() {
- // ignore errors, phase 3 will retry cleanup anyways
- let _ = unsafe { cache.remove(&digest) };
- }
- delete_list.push(content.key);
- Ok(())
- },
- )?;
+ 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() {
+ // ignore errors, phase 3 will retry cleanup anyways
+ let _ = cache.remove(&digest);
+ }
+ delete_list.push(content.key);
+ Ok(())
+ },
+ )?;
+ }
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] 6+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v2 0/3] GC refactor follow-ups
2025-10-15 10:13 [pbs-devel] [PATCH proxmox-backup v2 0/3] GC refactor follow-ups Fabian Grünbichler
` (2 preceding siblings ...)
2025-10-15 10:13 ` [pbs-devel] [PATCH proxmox-backup v2 3/3] GC: mark cond_sweep_chunk helper as unafe Fabian Grünbichler
@ 2025-10-15 10:50 ` Christian Ebner
2025-10-15 12:57 ` [pbs-devel] applied-series: " Fabian Grünbichler
4 siblings, 0 replies; 6+ messages in thread
From: Christian Ebner @ 2025-10-15 10:50 UTC (permalink / raw)
To: Fabian Grünbichler, pbs-devel
On 10/15/25 12:13 PM, Fabian Grünbichler wrote:
> some further follow-ups for the recent GC changes by Chris
Thanks! All patches look good to me now.
Tested GC on regular and S3 datastores work as expected
Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
Tested-by: Christian Ebner <c.ebner@proxmox.com>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* [pbs-devel] applied-series: [PATCH proxmox-backup v2 0/3] GC refactor follow-ups
2025-10-15 10:13 [pbs-devel] [PATCH proxmox-backup v2 0/3] GC refactor follow-ups Fabian Grünbichler
` (3 preceding siblings ...)
2025-10-15 10:50 ` [pbs-devel] [PATCH proxmox-backup v2 0/3] GC refactor follow-ups Christian Ebner
@ 2025-10-15 12:57 ` Fabian Grünbichler
4 siblings, 0 replies; 6+ messages in thread
From: Fabian Grünbichler @ 2025-10-15 12:57 UTC (permalink / raw)
To: pbs-devel, Fabian Grünbichler
On Wed, 15 Oct 2025 12:13:39 +0200, Fabian Grünbichler wrote:
> some further follow-ups for the recent GC changes by Chris
>
> Fabian Grünbichler (3):
> GC: refactor chunk removal helper
> GC: rename helper to cond_sweep_chunk
> GC: mark cond_sweep_chunk helper as unafe
>
> [...]
Applied with Chris' T/R-b, thanks!
[1/3] GC: refactor chunk removal helper
commit: c7d6fc9ca8b0c7496da1cac5b79ce96294ec44bc
[2/3] GC: rename helper to cond_sweep_chunk
commit: 8de7e8f03523454f83063dd325ff34e9257c720e
[3/3] GC: mark cond_sweep_chunk helper as unafe
commit: 7862ae5181389bc44b564f3d7e976b4c7238189a
Best regards,
--
Fabian Grünbichler <f.gruenbichler@proxmox.com>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 6+ messages in thread