[pbs-devel] [PATCH proxmox 2/2] pbs api types: add option to set GC chunk cleanup wait period

[Christian Ebner]

On 3/3/25 13:58, Fabian Grünbichler wrote:
> On February 19, 2025 5:48 pm, Christian Ebner wrote:
>> Add the `gc-wait-period` option to the datastore tuning parameters.
>> This allows to specify the time after which the chunks are not
>> considered in use anymore if their atime has not been updated since
>> then. This option is only considered, if the `gc-atime-check` is
>> enabled, to avoid potential data loss.
>>
>> The default is to keep chunks within the 24h 5m timespan (given no
>> active writers). The 5m minimum was chosen to be in line with the
>> already used safety offset for garbage collection.
>>
>> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
>> ---
>> changes since version 1:
>> - not present in previous version
>>
>>   pbs-api-types/src/datastore.rs | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
>> index a7a9db7d..7542e969 100644
>> --- a/pbs-api-types/src/datastore.rs
>> +++ b/pbs-api-types/src/datastore.rs
>> @@ -223,6 +223,13 @@ pub enum DatastoreFSyncLevel {
>>       Filesystem,
>>   }
>>   
>> +pub const GC_WAIT_PERIOD_SCHEMA: Schema =
>> +    IntegerSchema::new("Wait period (in minutes) for garbage collection phase 2 chunk cleanup (default 24h 5m)")
>> +        .minimum(5)
>> +        .maximum(1445)
> 
> these seem a bit conservative - if we introduce the option, we could
> also allow to reduce the grace period to 0 or to require longer grace
> periods?

I did stick to the additional 5 minutes safety gap, as this was also 
introduced with the oldest writer change in commit 11861a48 
("src/backup/chunk_store.rs: fix GC"). Since that commit does not 
explicitly mention the reason (I assume to prevent a race between writer 
instantiation and garbage collection), I sticked with this additional 
lower limit.

Increasing the upper limit might be possible without such concerns, I do 
not really see however the use case for that.

> 
> wait period is also a bit of a misnomer IMHO, this is not something
> where we "wait" per se, but rather something to account for the
> timestamp information potentially not being accurate.. not sure what a
> better term would be, "grace period" doesn't really fit either..

I sticked with Thomas' naming suggestion here, but maybe something like 
`gc-clenup-safety-margin` is more fitting? Although it does not imply 
that this is due to the atime... Other suggestions?

> 
> it's a kind of "safety margin", maybe something related to that? naming
> can be quite hard..
> 
> maybe gc_atime_safety_margin ? that also implies that setting a
> non-default value has potential safety implications/risks your data,
> IMHO more than "reducing a wait period" does..
> 
>> +        .default(1445)
>> +        .schema();
>> +
>>   #[api(
>>       properties: {
>>           "chunk-order": {
>> @@ -235,6 +242,10 @@ pub enum DatastoreFSyncLevel {
>>               default: true,
>>               type: bool,
>>           },
>> +        "gc-wait-period": {
>> +            schema: GC_WAIT_PERIOD_SCHEMA,
>> +            optional: true,
>> +        },
>>       },
>>   )]
>>   #[derive(Serialize, Deserialize, Default)]
>> @@ -248,6 +259,8 @@ pub struct DatastoreTuning {
>>       pub sync_level: Option<DatastoreFSyncLevel>,
>>       #[serde(skip_serializing_if = "Option::is_none")]
>>       pub gc_atime_check: Option<bool>,
>> +    #[serde(skip_serializing_if = "Option::is_none")]
>> +    pub gc_wait_period: Option<usize>,
>>   }
>>   
>>   pub const DATASTORE_TUNING_STRING_SCHEMA: Schema = StringSchema::new("Datastore tuning options")
>> -- 
>> 2.39.5
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
>