[pbs-devel] [RFC proxmox-backup 4/4] garbage collection: read pruned snapshot index files from trash

[Fabian Grünbichler]

> Christian Ebner <c.ebner at proxmox.com> hat am 17.04.2025 12:38 CEST geschrieben:
> On 4/17/25 11:29, Fabian Grünbichler wrote:
> > On April 16, 2025 4:18 pm, Christian Ebner wrote:
> >> Snapshots pruned during phase 1 are now also assured to be included
> >> in the marking phase by reading the index files from trash and
> >> touching these chunks as well.
> >>
> >> Clear any trash before starting with phase 1, so that only snapshots
> >> pruned during GC are consided.
> >>
> >> Further, drop the retry logic used before to assure eventually newly
> >> written index files are included in the marking phase, if the
> >> previously last one was pruned. This is not necessary anymore as the
> >> previously last one will be read from trash.
> >>
> >> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
> >> ---
> >>   pbs-datastore/src/datastore.rs | 141 +++++++++++++++------------------
> >>   1 file changed, 65 insertions(+), 76 deletions(-)
> >>
> >> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> >> index 97b78f000..688e65247 100644
> >> --- a/pbs-datastore/src/datastore.rs
> >> +++ b/pbs-datastore/src/datastore.rs
> >> @@ -1137,7 +1137,13 @@ impl DataStore {
> >>           //
> >>           // By this it is assured that all index files are used, even if they would not have been
> >>           // seen by the regular logic and the user is informed by the garbage collection run about
> >> -        // the detected index files not following the iterators logic.
> >> +        // the detected index files not following the iterators logic. Further, include trashed
> >> +        // snapshots which have been pruned during garbage collections marking phase.
> >> +
> >> +        let trash = PathBuf::from(".trash/");
> >> +        let mut full_trash_path = self.base_path();
> >> +        full_trash_path.push(&trash);
> >> +        let _ = std::fs::remove_dir_all(full_trash_path);
> > 
> > I think this would need some locking, else we start recursively deleting
> > here while at the same time a prune operation is moving something into
> > the trash..
> 
> True, if there is a concurrent insert, then deletion will probably fail 
> because the sub-directory is then not empty anymore. Instead of locking, 
> I could do an atomic rename of the whole trash can instead, and cleanup 
> the renamed one? Should be not only more efficient but also easier to 
> implement.

that only works if you assume that nothing operates on (or rather, inside of)
the trash dir via an already opened FD.. which seems like a dangerous and
easy to miss assumption..