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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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.

having a trash dir per group instead of a global one for the whole
datastore (less likely to incur extra costs in case somebody has a weird
setup where namespaces/.. are symlinked or bindmounted or similar
shenanigans). would need to postpone group removal to GC in case all
snapshots are pruned.

> 
> Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  pbs-datastore/src/backup_info.rs | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
> index d4732fdd9..cd0d521c9 100644
> --- a/pbs-datastore/src/backup_info.rs
> +++ b/pbs-datastore/src/backup_info.rs
> @@ -588,11 +588,19 @@ impl BackupDir {
>              bail!("cannot remove protected snapshot"); // use special error type?
>          }
>  
> +        let relative_path = self.relative_path();
> +        let mut trash_path = self.store.base_path();
> +        trash_path.push(".trash/");
> +        trash_path.push(relative_path);
> +        if let Some(parent) = trash_path.parent() {
> +            std::fs::create_dir_all(&parent)
> +                .with_context(|| format!("creating trash folders for {trash_path:?} failed"))?;
> +        }
> +
>          let full_path = self.full_path();
>          log::info!("removing backup snapshot {:?}", full_path);
> -        std::fs::remove_dir_all(&full_path).map_err(|err| {
> -            format_err!("removing backup snapshot {:?} failed - {}", full_path, err,)
> -        })?;
> +        std::fs::rename(&full_path, trash_path)
> +            .with_context(|| format!("moving backup snapshot {full_path:?} to trash failed"))?;
>  
>          // remove no longer needed lock files
>          let _ = std::fs::remove_file(self.manifest_lock_path()); // ignore errors
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> 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