[pbs-devel] [RFC proxmox-backup 3/4] datastore: move snapshots to trash folder on destroy

[Christian Ebner]

On 4/18/25 13:06, Thomas Lamprecht wrote:
> Am 17.04.25 um 11:29 schrieb Fabian Grünbichler:
>> On April 16, 2025 4:18 pm, Christian Ebner wrote:
>>> Instead of directly deleting the snapshot directory and it's contents
>>> on a prune, move the snapshot directory into the `.trash` subfolder
>>> of the datastore.
>>>
>>> This allows to mark chunks which were used by these index files if
>>> the snapshot was pruned during an ongoing garbage collection.
>>> Garbage collection will clean up these files before starting with the
>>> marking phase 1 and read all index files after completing that phase,
>>> touching these chunks as well.
>>
>> some other variants to maybe consider:
>>
>> marking the snapshot itself as trash (in the manifest, or by adding a
>> trash marker file inside the dir) - this would mean that there is no
>> iterator race issue when undoing a prune, no double-pruning collisions,
>> .. - but it also means we need to adapt all call sites that should skip
>> trashed snapshots (most existing ones), which is more churn.
> 
> Shouldn't we use the central iterators implementations to query indexes?

Yes, correct me if I'm wrong, have not checked all call sites yet but 
index files are mostly accessed by going trough the manifest, either via 
BackupManifest::files or at least verifying it via 
BackupManifest::verfiy_file, as that's also were encryption and 
verification state are stored.

So adding a label to store a trashed state there would work out just 
fine, filtering these snapshots for listing, sync job, ecc. is then fine 
as well. Also, fetching the previous backup snapshot for fast 
incremental mode will work, although require additional filtering.

Although, I'm a bit concerned about performance for the content listing 
if we keep and iterate all of the pruned snapshots. After all they will 
persist until next GC, which could lead to a lot of accumulated snapshots.

One further issue I see with that approach is again sync jobs, which now 
do not see the trashed snapshot on the target and try to re-sync it? Or 
would we include that information for the sync jobs to skip over? Would 
be a bit strange however if the snapshot is not trashed on the source side.

Also, thinking about UI to recover from trash: Might it be good to still 
show the snapshots while listing, but marked with an icon, just like for 
e.g. encryption state? Or create a dedicated window/tab to only show 
trashed items.

All in all storing the trash information on the manifest might not be 
the better option. Give above issues, I'm leaning more towards a 
separate folder structure for this.

What's your opinion on these concerns?