* [pbs-devel] [PATCH proxmox-backup 1/3] GC: refactor chunk removal helper
2025-10-15 8:38 [pbs-devel] [PATCH proxmox-backup 0/3] GC refactor follow-ups Fabian Grünbichler
@ 2025-10-15 8:38 ` Fabian Grünbichler
2025-10-15 9:10 ` Christian Ebner
2025-10-15 8:38 ` [pbs-devel] [PATCH proxmox-backup 2/3] GC: rename helper to cond_sweep_chunk Fabian Grünbichler
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Fabian Grünbichler @ 2025-10-15 8:38 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>
---
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..1c7df9074 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] 11+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup 1/3] GC: refactor chunk removal helper
2025-10-15 8:38 ` [pbs-devel] [PATCH proxmox-backup 1/3] GC: refactor chunk removal helper Fabian Grünbichler
@ 2025-10-15 9:10 ` Christian Ebner
2025-10-15 9:46 ` Fabian Grünbichler
0 siblings, 1 reply; 11+ messages in thread
From: Christian Ebner @ 2025-10-15 9:10 UTC (permalink / raw)
To: Fabian Grünbichler, pbs-devel
one comment inline
On 10/15/25 10:38 AM, Fabian Grünbichler wrote:
> simplify the callback, and move the error handling to the helper..
>
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> 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..1c7df9074 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);
Unless I'm overseeing something, this will now no longer propagate the
error in case the removal of a non-bad chunk fails? Previously the error
was returned independent from the `bad` state.
> + }
> + }
> 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) };
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup 1/3] GC: refactor chunk removal helper
2025-10-15 9:10 ` Christian Ebner
@ 2025-10-15 9:46 ` Fabian Grünbichler
2025-10-15 9:56 ` Christian Ebner
0 siblings, 1 reply; 11+ messages in thread
From: Fabian Grünbichler @ 2025-10-15 9:46 UTC (permalink / raw)
To: Christian Ebner, pbs-devel
On October 15, 2025 11:10 am, Christian Ebner wrote:
> one comment inline
>
> On 10/15/25 10:38 AM, Fabian Grünbichler wrote:
>> simplify the callback, and move the error handling to the helper..
>>
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>> 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..1c7df9074 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);
>
> Unless I'm overseeing something, this will now no longer propagate the
> error in case the removal of a non-bad chunk fails? Previously the error
> was returned independent from the `bad` state.
yes, you are right!
although I now wonder - should we make failure to remove bad chunk files
non-fatal? or even all chunk files? at this point we've made all the
decisions already, and best-effort removal might be better than no
removal (e.g., a single chunk with a permission issue effectively blocks
GC now??).
>
>> + }
>> + }
>> 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) };
>
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup 1/3] GC: refactor chunk removal helper
2025-10-15 9:46 ` Fabian Grünbichler
@ 2025-10-15 9:56 ` Christian Ebner
2025-10-15 10:04 ` Fabian Grünbichler
0 siblings, 1 reply; 11+ messages in thread
From: Christian Ebner @ 2025-10-15 9:56 UTC (permalink / raw)
To: Fabian Grünbichler, pbs-devel
On 10/15/25 11:46 AM, Fabian Grünbichler wrote:
> On October 15, 2025 11:10 am, Christian Ebner wrote:
>> one comment inline
>>
>> On 10/15/25 10:38 AM, Fabian Grünbichler wrote:
>>> simplify the callback, and move the error handling to the helper..
>>>
>>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>>> ---
>>> 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..1c7df9074 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);
>>
>> Unless I'm overseeing something, this will now no longer propagate the
>> error in case the removal of a non-bad chunk fails? Previously the error
>> was returned independent from the `bad` state.
>
> yes, you are right!
>
> although I now wonder - should we make failure to remove bad chunk files
> non-fatal? or even all chunk files? at this point we've made all the
> decisions already, and best-effort removal might be better than no
> removal (e.g., a single chunk with a permission issue effectively blocks
> GC now??).
No strong opinion on this, but I would agree. Removal on best effort
would at least not lead to unintentional fill up of the chunk store.
OTOH, most likely if the permissions are wrong on one chunk or the
removal fails for that particular file, this affects also others for
example out of memory situations on ZFS. So one probably does not gain
much? Or it might even lead to spamming of the task log, which should be
avoided.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup 1/3] GC: refactor chunk removal helper
2025-10-15 9:56 ` Christian Ebner
@ 2025-10-15 10:04 ` Fabian Grünbichler
0 siblings, 0 replies; 11+ messages in thread
From: Fabian Grünbichler @ 2025-10-15 10:04 UTC (permalink / raw)
To: Christian Ebner, pbs-devel
On October 15, 2025 11:56 am, Christian Ebner wrote:
> On 10/15/25 11:46 AM, Fabian Grünbichler wrote:
>> On October 15, 2025 11:10 am, Christian Ebner wrote:
>>> one comment inline
>>>
>>> On 10/15/25 10:38 AM, Fabian Grünbichler wrote:
>>>> simplify the callback, and move the error handling to the helper..
>>>>
>>>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>>>> ---
>>>> 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..1c7df9074 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);
>>>
>>> Unless I'm overseeing something, this will now no longer propagate the
>>> error in case the removal of a non-bad chunk fails? Previously the error
>>> was returned independent from the `bad` state.
>>
>> yes, you are right!
>>
>> although I now wonder - should we make failure to remove bad chunk files
>> non-fatal? or even all chunk files? at this point we've made all the
>> decisions already, and best-effort removal might be better than no
>> removal (e.g., a single chunk with a permission issue effectively blocks
>> GC now??).
>
> No strong opinion on this, but I would agree. Removal on best effort
> would at least not lead to unintentional fill up of the chunk store.
>
> OTOH, most likely if the permissions are wrong on one chunk or the
> removal fails for that particular file, this affects also others for
> example out of memory situations on ZFS. So one probably does not gain
> much? Or it might even lead to spamming of the task log, which should be
> avoided.
yeah, spamming the task log would indeed be bad.
we could introduce a new gc_status field for those maybe, and just
summarize according to error category instead of logging one error per
line, but that is yet another bigger refactor, so I'll just send v2 with
the handling as it was!
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 2/3] GC: rename helper to cond_sweep_chunk
2025-10-15 8:38 [pbs-devel] [PATCH proxmox-backup 0/3] GC refactor follow-ups Fabian Grünbichler
2025-10-15 8:38 ` [pbs-devel] [PATCH proxmox-backup 1/3] GC: refactor chunk removal helper Fabian Grünbichler
@ 2025-10-15 8:38 ` Fabian Grünbichler
2025-10-15 9:12 ` Christian Ebner
2025-10-15 8:38 ` [pbs-devel] [PATCH proxmox-backup 3/3] GC: mark cond_sweep_chunk helper as unafe Fabian Grünbichler
2025-10-15 10:14 ` [pbs-devel] superseded: [PATCH proxmox-backup 0/3] GC refactor follow-ups Fabian Grünbichler
3 siblings, 1 reply; 11+ messages in thread
From: Fabian Grünbichler @ 2025-10-15 8:38 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>
---
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 1c7df9074..f81603971 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] 11+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup 2/3] GC: rename helper to cond_sweep_chunk
2025-10-15 8:38 ` [pbs-devel] [PATCH proxmox-backup 2/3] GC: rename helper to cond_sweep_chunk Fabian Grünbichler
@ 2025-10-15 9:12 ` Christian Ebner
0 siblings, 0 replies; 11+ messages in thread
From: Christian Ebner @ 2025-10-15 9:12 UTC (permalink / raw)
To: Fabian Grünbichler, pbs-devel
On 10/15/25 10:38 AM, Fabian Grünbichler wrote:
> 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>
> ---
> 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 1c7df9074..f81603971 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,
LGTM!
Reviewed-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] 11+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 3/3] GC: mark cond_sweep_chunk helper as unafe
2025-10-15 8:38 [pbs-devel] [PATCH proxmox-backup 0/3] GC refactor follow-ups Fabian Grünbichler
2025-10-15 8:38 ` [pbs-devel] [PATCH proxmox-backup 1/3] GC: refactor chunk removal helper Fabian Grünbichler
2025-10-15 8:38 ` [pbs-devel] [PATCH proxmox-backup 2/3] GC: rename helper to cond_sweep_chunk Fabian Grünbichler
@ 2025-10-15 8:38 ` Fabian Grünbichler
2025-10-15 9:13 ` Christian Ebner
2025-10-15 10:14 ` [pbs-devel] superseded: [PATCH proxmox-backup 0/3] GC refactor follow-ups Fabian Grünbichler
3 siblings, 1 reply; 11+ messages in thread
From: Fabian Grünbichler @ 2025-10-15 8:38 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>
---
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 f81603971..94ff53b86 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] 11+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup 3/3] GC: mark cond_sweep_chunk helper as unafe
2025-10-15 8:38 ` [pbs-devel] [PATCH proxmox-backup 3/3] GC: mark cond_sweep_chunk helper as unafe Fabian Grünbichler
@ 2025-10-15 9:13 ` Christian Ebner
0 siblings, 0 replies; 11+ messages in thread
From: Christian Ebner @ 2025-10-15 9:13 UTC (permalink / raw)
To: Fabian Grünbichler, pbs-devel
On 10/15/25 10:38 AM, Fabian Grünbichler wrote:
> 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>
> ---
> 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 f81603971..94ff53b86 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;
> }
LGTM!
Reviewed-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] 11+ messages in thread
* [pbs-devel] superseded: [PATCH proxmox-backup 0/3] GC refactor follow-ups
2025-10-15 8:38 [pbs-devel] [PATCH proxmox-backup 0/3] GC refactor follow-ups Fabian Grünbichler
` (2 preceding siblings ...)
2025-10-15 8:38 ` [pbs-devel] [PATCH proxmox-backup 3/3] GC: mark cond_sweep_chunk helper as unafe Fabian Grünbichler
@ 2025-10-15 10:14 ` Fabian Grünbichler
3 siblings, 0 replies; 11+ messages in thread
From: Fabian Grünbichler @ 2025-10-15 10:14 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
by https://lore.proxmox.com/pbs-devel/20251015101349.872639-2-f.gruenbichler@proxmox.com/T/#u
On October 15, 2025 10:38 am, 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
>
> pbs-datastore/src/chunk_store.rs | 57 +++++++++++++++++---------------
> pbs-datastore/src/datastore.rs | 34 ++++++++++---------
> 2 files changed, 49 insertions(+), 42 deletions(-)
>
> --
> 2.47.3
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 11+ messages in thread