[pbs-devel] [PATCH proxmox-backup 0/4] GC cleanups

[pbs-devel] [PATCH proxmox-backup 0/4] GC cleanups

[Fabian Grünbichler]

Fabian Grünbichler (4):
  gc: shorten progress messages
  gc: log index files found outside of expected scheme
  gc: remove duplicate variable
  gc: don't limit index listing to same filesystem

 src/backup/chunk_store.rs |  2 +-
 src/backup/datastore.rs   | 34 +++++++++++++++++++++++++++-------
 2 files changed, 28 insertions(+), 8 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 1/4] gc: shorten progress messages

[Fabian Grünbichler]

we have messages starting the phases anyway, and limit the number of
progress updates so that context remains available at all times.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/backup/chunk_store.rs | 2 +-
 src/backup/datastore.rs   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/backup/chunk_store.rs b/src/backup/chunk_store.rs
index 59719bad..34d6f655 100644
--- a/src/backup/chunk_store.rs
+++ b/src/backup/chunk_store.rs
@@ -301,7 +301,7 @@ impl ChunkStore {
                 last_percentage = percentage;
                 crate::task_log!(
                     worker,
-                    "percentage done: phase2 {}% (processed {} chunks)",
+                    "processed {}% ({} chunks)",
                     percentage,
                     chunk_count,
                 );
diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index 19efc23f..b199f11f 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -505,7 +505,7 @@ impl DataStore {
             if percentage > last_percentage {
                 crate::task_log!(
                     worker,
-                    "percentage done: phase1 {}% ({} of {} index files)",
+                    "marked {}% ({} of {} index files)",
                     percentage,
                     done,
                     image_count,
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/4] gc: log index files found outside of expected scheme

[Fabian Grünbichler]

for safety reason, GC finds and marks all index files below the
datastore base path. as a result of regular operations, only index files
within the expected scheme of <TYPE>/<ID>/<TIMESTAMP> should exist.

add a small check + warning if the index list contains index files out
side of this expected scheme, so that an admin with shell access can
investigate.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    we could log the full path to syslog/journal as well

 src/backup/datastore.rs | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index b199f11f..2700d6e9 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -3,6 +3,7 @@ use std::io::{self, Write};
 use std::path::{Path, PathBuf};
 use std::sync::{Arc, Mutex};
 use std::convert::TryFrom;
+use std::str::FromStr;
 use std::time::Duration;
 use std::fs::File;
 
@@ -474,12 +475,24 @@ impl DataStore {
         let mut done = 0;
         let mut last_percentage: usize = 0;
 
+        let mut strange_paths_count: u64 = 0;
+
         for img in image_list {
 
             worker.check_abort()?;
             tools::fail_on_shutdown()?;
 
+            if let Some(backup_dir_path) = img.parent() {
+                let backup_dir_path = backup_dir_path.strip_prefix(self.base_path())?;
+                if let Some(backup_dir_str) = backup_dir_path.to_str() {
+                    if BackupDir::from_str(backup_dir_str).is_err() {
+                        strange_paths_count += 1;
+                    }
+                }
+            }
+
             let path = self.chunk_store.relative_path(&img);
+
             match std::fs::File::open(&path) {
                 Ok(file) => {
                     if let Ok(archive_type) = archive_type(&img) {
@@ -514,6 +527,15 @@ impl DataStore {
             }
         }
 
+        if strange_paths_count > 0 {
+            crate::task_log!(
+                worker,
+                "found (and marked) {} index files outside of expected directory scheme",
+                strange_paths_count,
+            );
+        }
+
+
         Ok(())
     }
 
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 3/4] gc: remove duplicate variable

[Fabian Grünbichler]

list_images already returns absolute paths, we don't need to prepend
anything.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/backup/datastore.rs | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index 2700d6e9..8ea0a753 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -491,26 +491,24 @@ impl DataStore {
                 }
             }
 
-            let path = self.chunk_store.relative_path(&img);
-
-            match std::fs::File::open(&path) {
+            match std::fs::File::open(&img) {
                 Ok(file) => {
                     if let Ok(archive_type) = archive_type(&img) {
                         if archive_type == ArchiveType::FixedIndex {
                             let index = FixedIndexReader::new(file).map_err(|e| {
-                                format_err!("can't read index '{}' - {}", path.to_string_lossy(), e)
+                                format_err!("can't read index '{}' - {}", img.to_string_lossy(), e)
                             })?;
                             self.index_mark_used_chunks(index, &img, status, worker)?;
                         } else if archive_type == ArchiveType::DynamicIndex {
                             let index = DynamicIndexReader::new(file).map_err(|e| {
-                                format_err!("can't read index '{}' - {}", path.to_string_lossy(), e)
+                                format_err!("can't read index '{}' - {}", img.to_string_lossy(), e)
                             })?;
                             self.index_mark_used_chunks(index, &img, status, worker)?;
                         }
                     }
                 }
                 Err(err) if err.kind() == io::ErrorKind::NotFound => (), // ignore vanished files
-                Err(err) => bail!("can't open index {} - {}", path.to_string_lossy(), err),
+                Err(err) => bail!("can't open index {} - {}", img.to_string_lossy(), err),
             }
             done += 1;
 
-- 
2.20.1

[pbs-devel] [RFC proxmox-backup 4/4] gc: don't limit index listing to same filesystem

[Fabian Grünbichler]

WalkDir does not follow symlinks by default anyway, and this behaviour
is not documented anywhere. e.g., if a sysadmin mounts 'extra storage'
for some backup group or type (not knowing that only metadata is stored
in those directories), GC will ignore all the indices contained within
and happily garbage collect their chunks..

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    RFC since I am not 100% sure, but the current behaviour seems like a timebomb
    waiting to bite someone's behind..

 src/backup/datastore.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index 8ea0a753..a4c437e1 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -380,7 +380,7 @@ impl DataStore {
 
         use walkdir::WalkDir;
 
-        let walker = WalkDir::new(&base).same_file_system(true).into_iter();
+        let walker = WalkDir::new(&base).into_iter();
 
         // make sure we skip .chunks (and other hidden files to keep it simple)
         fn is_hidden(entry: &walkdir::DirEntry) -> bool {
-- 
2.20.1

[pbs-devel] applied: [PATCH proxmox-backup 0/4] GC cleanups

[Dietmar Maurer]

applied, thanks!

> On 11/30/2020 4:22 PM Fabian Grünbichler <f.gruenbichler@proxmox.com> wrote:
> 
>  
> Fabian Grünbichler (4):
>   gc: shorten progress messages
>   gc: log index files found outside of expected scheme
>   gc: remove duplicate variable
>   gc: don't limit index listing to same filesystem
> 
>  src/backup/chunk_store.rs |  2 +-
>  src/backup/datastore.rs   | 34 +++++++++++++++++++++++++++-------
>  2 files changed, 28 insertions(+), 8 deletions(-)
> 
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel