[pbs-devel] [PATCH proxmox-backup v2 1/4] GC: s3: fix local marker cleanup for unreferenced, s3 only chunks

[Christian Ebner]

If a chunk object is located on the s3 object store only, not being
referenced by any index file and having no local marker file it was
marked for cleanup by pretending an atime equal to the unix epoch.

While this will mark the chunk for deletion from the backend and
include it in the delete list for the next s3 delete objects call, it
also will lead to the chunk marker and LRU cache entry being tried to
clean up locally, which however failed since there is no marker to be
cleaned up.

In order to treat this edge case, instead of pretending an atime
equal to the unix epoch, make the atime optional and skip over the
atime check for that case altogether, directly pushing the object and
its guard to the delete list, while updating the gc status
accordingly.

Fixes: https://forum.proxmox.com/threads/176567/
Originally-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
---
Changes since version 1:
- Skip over cond_sweep_chunk altogether if the marker is not present and
  chunk no longer required

 pbs-datastore/src/datastore.rs | 68 +++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 29 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 65299cca9..1b489b449 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1709,48 +1709,58 @@ impl DataStore {
                     // Check local markers (created or atime updated during phase1) and
                     // keep or delete chunk based on that.
                     let atime = match std::fs::metadata(&chunk_path) {
-                        Ok(stat) => stat.accessed()?,
+                        Ok(stat) => Some(stat.accessed()?),
                         Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
                             if self.inner.chunk_store.clear_chunk_expected_mark(&digest)? {
                                 unsafe {
                                     // chunk store lock held
                                     self.inner.chunk_store.replace_chunk_with_marker(&digest)?;
                                 }
-                                SystemTime::now()
+                                Some(SystemTime::now())
                             } else {
-                                // File not found, delete by setting atime to unix epoch
-                                SystemTime::UNIX_EPOCH
+                                // File not found, only delete from S3
+                                None
                             }
                         }
                         Err(err) => return Err(err.into()),
                     };
-                    let atime = atime.duration_since(SystemTime::UNIX_EPOCH)?.as_secs() as i64;
-
-                    unsafe {
-                        self.inner.chunk_store.cond_sweep_chunk(
-                            atime,
-                            min_atime,
-                            oldest_writer,
-                            content.size,
-                            bad,
-                            &mut gc_status,
-                            || {
-                                if let Some(cache) = self.cache() {
-                                    if !bad {
-                                        cache.remove(&digest)?;
-                                    } else {
-                                        std::fs::remove_file(chunk_path)?;
+                    if let Some(atime) = atime {
+                        let atime = atime.duration_since(SystemTime::UNIX_EPOCH)?.as_secs() as i64;
+
+                        unsafe {
+                            self.inner.chunk_store.cond_sweep_chunk(
+                                atime,
+                                min_atime,
+                                oldest_writer,
+                                content.size,
+                                bad,
+                                &mut gc_status,
+                                || {
+                                    if let Some(cache) = self.cache() {
+                                        if !bad {
+                                            cache.remove(&digest)?;
+                                        } else {
+                                            std::fs::remove_file(chunk_path)?;
+                                        }
                                     }
-                                }
 
-                                // set age based on first insertion
-                                if delete_list.is_empty() {
-                                    delete_list_age = epoch_i64();
-                                }
-                                delete_list.push((content.key, _chunk_guard));
-                                Ok(())
-                            },
-                        )?;
+                                    // set age based on first insertion
+                                    if delete_list.is_empty() {
+                                        delete_list_age = epoch_i64();
+                                    }
+                                    delete_list.push((content.key, _chunk_guard));
+                                    Ok(())
+                                },
+                            )?;
+                        }
+                    } else {
+                        gc_status.removed_chunks += 1;
+                        gc_status.removed_bytes += content.size;
+                        // set age based on first insertion
+                        if delete_list.is_empty() {
+                            delete_list_age = epoch_i64();
+                        }
+                        delete_list.push((content.key, _chunk_guard));
                     }
 
                     chunk_count += 1;
-- 
2.47.3