[pbs-devel] [PATCH proxmox-backup v3 0/4] improve catalog handling

[pbs-devel] [PATCH proxmox-backup v3 0/4] improve catalog handling

[Dietmar Maurer]

Resend all patches to make it clear how it is supposed to be used.

The basic diffence to Dominiks patches is that the cache files are
created on demand.

Dietmar Maurer (3):
  cleanup: factor out tape catalog path helpers
  tape: lock media_catalog file to to get a consistent view with
    load_catalog
  tape: media_catalog: add snapshot list cache for catalog

Dominik Csapak (1):
  api2: tape: media: use MediaCatalog::snapshot_list for content listing

 src/api2/tape/media.rs          |  45 +++++++------
 src/tape/media_catalog.rs       |  65 +++++++++++--------
 src/tape/media_catalog_cache.rs | 108 ++++++++++++++++++++++++++++++++
 src/tape/mod.rs                 |   3 +
 4 files changed, 171 insertions(+), 50 deletions(-)
 create mode 100644 src/tape/media_catalog_cache.rs

-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 1/4] cleanup: factor out tape catalog path helpers

[Dietmar Maurer]

---
 src/tape/media_catalog.rs | 43 +++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 22 deletions(-)

diff --git a/src/tape/media_catalog.rs b/src/tape/media_catalog.rs
index 086c8a7d..62c6acb3 100644
--- a/src/tape/media_catalog.rs
+++ b/src/tape/media_catalog.rs
@@ -2,7 +2,7 @@ use std::convert::TryFrom;
 use std::fs::File;
 use std::io::{Write, Read, BufReader, Seek, SeekFrom};
 use std::os::unix::io::AsRawFd;
-use std::path::Path;
+use std::path::{PathBuf, Path};
 use std::collections::{HashSet, HashMap};
 
 use anyhow::{bail, format_err, Error};
@@ -95,20 +95,29 @@ impl MediaCatalog {
         Ok(catalogs)
     }
 
-    /// Test if a catalog exists
-    pub fn exists(base_path: &Path, uuid: &Uuid) -> bool {
+    pub fn catalog_path(base_path: &Path, uuid: &Uuid) -> PathBuf {
         let mut path = base_path.to_owned();
         path.push(uuid.to_string());
         path.set_extension("log");
-        path.exists()
+        path
+    }
+
+    fn tmp_catalog_path(base_path: &Path, uuid: &Uuid) -> PathBuf {
+        let mut path = base_path.to_owned();
+        path.push(uuid.to_string());
+        path.set_extension("tmp");
+        path
+    }
+
+    /// Test if a catalog exists
+    pub fn exists(base_path: &Path, uuid: &Uuid) -> bool {
+        Self::catalog_path(base_path, uuid).exists()
     }
 
     /// Destroy the media catalog (remove all files)
     pub fn destroy(base_path: &Path, uuid: &Uuid) -> Result<(), Error> {
 
-        let mut path = base_path.to_owned();
-        path.push(uuid.to_string());
-        path.set_extension("log");
+        let path = Self::catalog_path(base_path, uuid);
 
         match std::fs::remove_file(path) {
             Ok(()) => Ok(()),
@@ -125,9 +134,7 @@ impl MediaCatalog {
 
         let uuid = &media_id.label.uuid;
 
-        let mut path = base_path.to_owned();
-        path.push(uuid.to_string());
-        path.set_extension("log");
+        let path = Self::catalog_path(base_path, uuid);
 
         let file = match std::fs::OpenOptions::new().read(true).open(&path) {
             Ok(file) => file,
@@ -198,9 +205,7 @@ impl MediaCatalog {
 
         let uuid = &media_id.label.uuid;
 
-        let mut path = base_path.to_owned();
-        path.push(uuid.to_string());
-        path.set_extension("log");
+        let path = Self::catalog_path(base_path, uuid);
 
         let me = proxmox::try_block!({
 
@@ -251,9 +256,7 @@ impl MediaCatalog {
 
         Self::create_basedir(base_path)?;
 
-        let mut tmp_path = base_path.to_owned();
-        tmp_path.push(uuid.to_string());
-        tmp_path.set_extension("tmp");
+        let tmp_path = Self::tmp_catalog_path(base_path, uuid);
 
         let file = std::fs::OpenOptions::new()
             .read(true)
@@ -285,9 +288,7 @@ impl MediaCatalog {
 
         let uuid = &media_id.label.uuid;
 
-        let mut tmp_path = base_path.to_owned();
-        tmp_path.push(uuid.to_string());
-        tmp_path.set_extension("tmp");
+        let tmp_path = Self::tmp_catalog_path(base_path, uuid);
 
         let me = proxmox::try_block!({
 
@@ -333,9 +334,7 @@ impl MediaCatalog {
         commit: bool,
     ) -> Result<(), Error> {
 
-        let mut tmp_path = base_path.to_owned();
-        tmp_path.push(uuid.to_string());
-        tmp_path.set_extension("tmp");
+        let tmp_path = Self::tmp_catalog_path(base_path, uuid);
 
         if commit {
             let mut catalog_path = tmp_path.clone();
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 2/4] tape: lock media_catalog file to to get a consistent view with load_catalog

[Dietmar Maurer]

---
 src/tape/media_catalog.rs | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/src/tape/media_catalog.rs b/src/tape/media_catalog.rs
index 62c6acb3..1bd136f9 100644
--- a/src/tape/media_catalog.rs
+++ b/src/tape/media_catalog.rs
@@ -231,7 +231,12 @@ impl MediaCatalog {
                 pending: Vec::new(),
             };
 
-            let (found_magic_number, _) = me.load_catalog(&mut file, media_id.media_set_label.as_ref())?;
+            // Note: lock file, to get a consistent view with load_catalog
+            nix::fcntl::flock(file.as_raw_fd(), nix::fcntl::FlockArg::LockExclusive)?;
+            let result = me.load_catalog(&mut file, media_id.media_set_label.as_ref());
+            nix::fcntl::flock(file.as_raw_fd(), nix::fcntl::FlockArg::Unlock)?;
+
+            let (found_magic_number, _) = result?;
 
             if !found_magic_number {
                 me.pending.extend(&Self::PROXMOX_BACKUP_MEDIA_CATALOG_MAGIC_1_1);
@@ -372,9 +377,18 @@ impl MediaCatalog {
 
         match self.file {
             Some(ref mut file) => {
-                file.write_all(&self.pending)?;
-                file.flush()?;
-                file.sync_data()?;
+                let pending = &self.pending;
+                // Note: lock file, to get a consistent view with load_catalog
+                nix::fcntl::flock(file.as_raw_fd(), nix::fcntl::FlockArg::LockExclusive)?;
+                let result: Result<(), Error> = proxmox::try_block!({
+                    file.write_all(pending)?;
+                    file.flush()?;
+                    file.sync_data()?;
+                    Ok(())
+                });
+                nix::fcntl::flock(file.as_raw_fd(), nix::fcntl::FlockArg::Unlock)?;
+
+                result?;
             }
             None => bail!("media catalog not writable (opened read only)"),
         }
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 3/4] tape: media_catalog: add snapshot list cache for catalog

[Dietmar Maurer]

For some parts of the ui, we only need the snapshot list from the catalog,
and reading the whole catalog (can be multiple hundred MiB) is not
really necessary.

Instead, we write the list of snapshots into a seperate .index file. This file
is generated on demand and is much smaller and thus faster to read.
---
 src/tape/media_catalog_cache.rs | 108 ++++++++++++++++++++++++++++++++
 src/tape/mod.rs                 |   3 +
 2 files changed, 111 insertions(+)
 create mode 100644 src/tape/media_catalog_cache.rs

diff --git a/src/tape/media_catalog_cache.rs b/src/tape/media_catalog_cache.rs
new file mode 100644
index 00000000..bf298e65
--- /dev/null
+++ b/src/tape/media_catalog_cache.rs
@@ -0,0 +1,108 @@
+use std::path::Path;
+use std::io::{BufRead, BufReader};
+
+use anyhow::{format_err, bail, Error};
+
+use proxmox::tools::fs::CreateOptions;
+
+use crate::tape::{MediaCatalog, MediaId};
+
+/// Returns a list of (store, snapshot) for a given MediaId
+///
+/// To speedup things for large catalogs, we cache the list of
+/// snapshots into a separate file.
+pub fn media_catalog_snapshot_list(
+    base_path: &Path,
+    media_id: &MediaId,
+) -> Result<Vec<(String, String)>, Error> {
+
+    let uuid = &media_id.label.uuid;
+
+    let mut cache_path = base_path.to_owned();
+    cache_path.push(uuid.to_string());
+    let mut catalog_path = cache_path.clone();
+    cache_path.set_extension("index");
+    catalog_path.set_extension("log");
+
+    let stat = match nix::sys::stat::stat(&catalog_path) {
+        Ok(stat) => stat,
+        Err(err) => bail!("unable to stat media catalog {:?} - {}", catalog_path, err),
+    };
+
+    let cache_id = format!("{:016X}-{:016X}-{:016X}", stat.st_ino, stat.st_size as u64, stat.st_mtime as u64);
+
+    match std::fs::OpenOptions::new().read(true).open(&cache_path) {
+        Ok(file) => {
+            let mut list = Vec::new();
+            let file = BufReader::new(file);
+            let mut lines = file.lines();
+            match lines.next() {
+                Some(Ok(id)) => {
+                    if id != cache_id { // cache is outdated - rewrite
+                        return write_snapshot_cache(base_path, media_id, &cache_path, &cache_id);
+                    }
+                }
+                _ => bail!("unable to read catalog cache firstline {:?}", cache_path),
+            }
+
+            for line in lines {
+                let mut line = line?;
+
+                let idx = line
+                    .find(':')
+                    .ok_or_else(|| format_err!("invalid line format (no store found)"))?;
+
+                let snapshot = line.split_off(idx + 1);
+                line.truncate(idx);
+                list.push((line, snapshot));
+            }
+
+            Ok(list)
+        }
+        Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
+            write_snapshot_cache(base_path, media_id, &cache_path, &cache_id)
+        }
+        Err(err) => bail!("unable to open catalog cache - {}", err),
+    }
+}
+
+fn write_snapshot_cache(
+    base_path: &Path,
+    media_id: &MediaId,
+    cache_path: &Path,
+    cache_id: &str,
+) ->  Result<Vec<(String, String)>, Error> {
+
+    // open normal catalog and write cache
+    let catalog = MediaCatalog::open(base_path, media_id, false, false)?;
+
+    let mut data = String::new();
+    data.push_str(cache_id);
+    data.push('\n');
+
+    let mut list = Vec::new();
+    for (store, content) in catalog.content() {
+        for snapshot in content.snapshot_index.keys() {
+            list.push((store.to_string(), snapshot.to_string()));
+            data.push_str(store);
+            data.push(':');
+            data.push_str(snapshot);
+            data.push('\n');
+        }
+    }
+
+    let backup_user = crate::backup::backup_user()?;
+    let mode = nix::sys::stat::Mode::from_bits_truncate(0o0640);
+    let options = CreateOptions::new()
+        .perm(mode)
+        .owner(backup_user.uid)
+        .group(backup_user.gid);
+
+    proxmox::tools::fs::replace_file(
+        cache_path,
+        data.as_bytes(),
+        options,
+    )?;
+
+    Ok(list)
+}
diff --git a/src/tape/mod.rs b/src/tape/mod.rs
index 8190e141..93c24719 100644
--- a/src/tape/mod.rs
+++ b/src/tape/mod.rs
@@ -42,6 +42,9 @@ pub use media_pool::*;
 mod media_catalog;
 pub use media_catalog::*;
 
+mod media_catalog_cache;
+pub use media_catalog_cache::*;
+
 mod pool_writer;
 pub use pool_writer::*;
 
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 4/4] api2: tape: media: use MediaCatalog::snapshot_list for content listing

[Dietmar Maurer]

From: Dominik Csapak <d.csapak@proxmox.com>

this should make the api call much faster, since it is not reading
the whole catalog anymore

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
 src/api2/tape/media.rs | 45 ++++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/src/api2/tape/media.rs b/src/api2/tape/media.rs
index 8351b2be..f8e1699d 100644
--- a/src/api2/tape/media.rs
+++ b/src/api2/tape/media.rs
@@ -42,6 +42,7 @@ use crate::{
         Inventory,
         MediaPool,
         MediaCatalog,
+        media_catalog_snapshot_list,
         changer::update_online_status,
     },
 };
@@ -502,32 +503,28 @@ pub fn list_content(
             .generate_media_set_name(&set.uuid, template)
             .unwrap_or_else(|_| set.uuid.to_string());
 
-        let catalog = MediaCatalog::open(status_path, &media_id, false, false)?;
+        for (store, snapshot) in media_catalog_snapshot_list(status_path, &media_id)? {
+            let backup_dir: BackupDir = snapshot.parse()?;
 
-        for (store, content) in catalog.content() {
-            for snapshot in content.snapshot_index.keys() {
-                let backup_dir: BackupDir = snapshot.parse()?;
-
-                if let Some(ref backup_type) = filter.backup_type {
-                    if backup_dir.group().backup_type() != backup_type { continue; }
-                }
-                if let Some(ref backup_id) = filter.backup_id {
-                    if backup_dir.group().backup_id() != backup_id { continue; }
-                }
-
-                list.push(MediaContentEntry {
-                    uuid: media_id.label.uuid.clone(),
-                    label_text: media_id.label.label_text.to_string(),
-                    pool: set.pool.clone(),
-                    media_set_name: media_set_name.clone(),
-                    media_set_uuid: set.uuid.clone(),
-                    media_set_ctime: set.ctime,
-                    seq_nr: set.seq_nr,
-                    snapshot: snapshot.to_owned(),
-                    store: store.to_owned(),
-                    backup_time: backup_dir.backup_time(),
-                });
+            if let Some(ref backup_type) = filter.backup_type {
+                if backup_dir.group().backup_type() != backup_type { continue; }
+            }
+            if let Some(ref backup_id) = filter.backup_id {
+                if backup_dir.group().backup_id() != backup_id { continue; }
             }
+
+            list.push(MediaContentEntry {
+                uuid: media_id.label.uuid.clone(),
+                label_text: media_id.label.label_text.to_string(),
+                pool: set.pool.clone(),
+                media_set_name: media_set_name.clone(),
+                media_set_uuid: set.uuid.clone(),
+                media_set_ctime: set.ctime,
+                seq_nr: set.seq_nr,
+                snapshot: snapshot.to_owned(),
+                store: store.to_owned(),
+                backup_time: backup_dir.backup_time(),
+            });
         }
     }
 
-- 
2.30.2

[pbs-devel] applied: [PATCH proxmox-backup v3 0/4] improve catalog handling

[Dietmar Maurer]

applied

On 7/28/21 11:11 AM, Dietmar Maurer wrote:
> Resend all patches to make it clear how it is supposed to be used.
>
> The basic diffence to Dominiks patches is that the cache files are
> created on demand.
>
> Dietmar Maurer (3):
>    cleanup: factor out tape catalog path helpers
>    tape: lock media_catalog file to to get a consistent view with
>      load_catalog
>    tape: media_catalog: add snapshot list cache for catalog
>
> Dominik Csapak (1):
>    api2: tape: media: use MediaCatalog::snapshot_list for content listing
>
>   src/api2/tape/media.rs          |  45 +++++++------
>   src/tape/media_catalog.rs       |  65 +++++++++++--------
>   src/tape/media_catalog_cache.rs | 108 ++++++++++++++++++++++++++++++++
>   src/tape/mod.rs                 |   3 +
>   4 files changed, 171 insertions(+), 50 deletions(-)
>   create mode 100644 src/tape/media_catalog_cache.rs
>