Re: [pbs-devel] [RFC v2 proxmox-backup 14/21] datastore: GC: clean-up trashed snapshots, groups and namespaces

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

On May 8, 2025 3:05 pm, Christian Ebner wrote:
> Cleanup trashed items during phase 1 of garbage collection. If
> encountered, index files located within trashed snapshots are touched
> as well, deferring chunk cleanup to the next run
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  pbs-datastore/src/datastore.rs | 84 +++++++++++++++++++++++++++++++++-
>  1 file changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index ca05e1bea..d88af4c68 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -574,6 +574,18 @@ impl DataStore {
>          !path.exists()
>      }
>  
> +    /// Checks if the namespace trash marker file exists,
> +    /// does not imply that the namespace itself exists.
> +    pub fn namespace_is_trashed(&self, namespace: &BackupNamespace) -> bool {
> +        if namespace.is_root() {
> +            return false;
> +        }
> +        let mut path = self.base_path();
> +        path.push(namespace.path());
> +        path.push(TRASH_MARKER_FILENAME);
> +        path.exists()
> +    }
> +
>      /// Remove the namespace and all it's parent components from the trash by removing the trash or
>      /// trash-pending marker file for each namespace level from deepest to shallowest. Missing files
>      /// are ignored.
> @@ -1322,7 +1334,7 @@ impl DataStore {
>              .context("creating namespace iterator failed")?
>          {
>              let namespace = namespace.context("iterating namespaces failed")?;
> -            for group in arc_self.iter_backup_groups(namespace)? {
> +            for group in arc_self.iter_backup_groups(namespace.clone())? {
>                  let group = group.context("iterating backup groups failed")?;
>  
>                  // Avoid race between listing/marking of snapshots by GC and pruning the last
> @@ -1403,10 +1415,80 @@ impl DataStore {
>                              }
>                              processed_index_files += 1;
>                          }
> +
> +                        // Only try to lock a trashed snapshots and continue if that is not
> +                        // possible, as then most likely this is in the process of being untrashed.
> +                        // Check trash state before and after locking to avoid otherwise possible
> +                        // races.
> +                        if snapshot.backup_dir.is_trashed() {
> +                            if let Ok(_lock) = snapshot.backup_dir.lock() {
> +                                if snapshot.backup_dir.is_trashed() {
> +                                    let path = snapshot.backup_dir.full_path();
> +                                    log::info!("removing trashed backup snapshot {path:?}");
> +                                    std::fs::remove_dir_all(&path).with_context(|| {
> +                                        format!("removing trashed backup snapshot {path:?} failed")
> +                                    })?;
> +                                }
> +                            } else {
> +                                let path = snapshot.backup_dir.full_path();
> +                                warn!("failed to lock trashed backup snapshot can {path:?}");
> +                            }
> +                        }
>                      }
>  
>                      break;
>                  }
> +                if group.is_trashed() {
> +                    if let Ok(_lock) = group.lock() {
> +                        if group.is_trashed() {

shouldn't this use some helper to reduce code duplication?

> +                            let trash_path = group.full_group_path().join(".trashed");
> +                            std::fs::remove_file(&trash_path).map_err(|err| {
> +                                format_err!(
> +                                    "removing the trash file '{trash_path:?}' failed - {err}"
> +                                )
> +                            })?;
> +
> +                            let owner_path = group.full_group_path().join("owner");
> +                            std::fs::remove_file(&owner_path).map_err(|err| {
> +                                format_err!(
> +                                    "removing the owner file '{owner_path:?}' failed - {err}"
> +                                )
> +                            })?;
> +
> +                            let path = group.full_group_path();
> +
> +                            std::fs::remove_dir(&path).map_err(|err| {
> +                                format_err!("removing group directory {path:?} failed - {err}")
> +                            })?;
> +
> +                            // Remove any now empty backup type directory

is this needed here? if we remove the whole namespace below, it would be
done anyway..

> +                            let base_file = std::fs::File::open(self.base_path())?;
> +                            let base_fd = base_file.as_raw_fd();
> +                            for ty in BackupType::iter() {
> +                                let mut ty_dir = namespace.path();
> +                                ty_dir.push(ty.to_string());
> +                                match unlinkat(Some(base_fd), &ty_dir, UnlinkatFlags::RemoveDir) {
> +                                    Ok(_) => (),
> +                                    Err(nix::errno::Errno::ENOENT) |
> +                                    Err(nix::errno::Errno::ENOTEMPTY) => (),
> +                                    Err(err) => info!("failed to remove backup type directory for {namespace} - {err}"),
> +                                }
> +                            }
> +                        } else {
> +                            let path = group.full_group_path();
> +                            warn!("failed to lock trashed backup group {path:?}");
> +                        }
> +                    }
> +                }
> +            }
> +            if self.namespace_is_trashed(&namespace) {
> +                // Remove the namespace, but only if it was empty (as the GC already cleared child
> +                // items and no new ones have been created since).
> +                match arc_self.destroy_namespace_recursive(&namespace, false) {
> +                    Ok(true) => info!("removed trashed namespace {namespace}"),
> +                    Ok(false) => info!("failed to remove trashed namespace {namespace}, not empty"),
> +                    Err(err) => warn!("removing trashed namespace failed: {err:#}"),
> +                }
>              }
>          }
>  
> -- 
> 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