[pbs-devel] [PATCH proxmox-backup stable-3 v2 0/3] Backport group listing optimizations including bugfix

[pbs-devel] [PATCH proxmox-backup stable-3 v2 0/3] Backport group listing optimizations including bugfix

[Christian Ebner]

These patches were part of the patch series to introduce the
streaming content api for PBS [0], optimizing the backup group
listing.

They did however indirectly fix also a bug in the group listing
api call, which lead to the `last-backup` time to be returned
incorrectly in some occasions. This was found and disected in
issue 6956 [1], identifying an incorrect is_finished() check while
folding the snapshot iterator to determine the last finished snapshot
of a group.

Therefore, instead of just fixing the bug, backport the relevant
patches which also provides the group listing improvements.

While the first 2 patches are preparatory, the last patch actually
fixes the issue.

[0] https://lore.proxmox.com/pbs-devel/20251008134344.3512958-1-d.csapak@proxmox.com/T/#t
[1] https://bugzilla.proxmox.com/show_bug.cgi?id=6956

Dominik Csapak (3):
  api: admin: datastore: refactor BackupGroup to GroupListItem
    conversion
  api: admin: datastore: factor out 'get_group_owner'
  api: admin: datastore: optimize `groups` api call

 src/api2/admin/datastore.rs | 134 +++++++++++++++++++++---------------
 1 file changed, 77 insertions(+), 57 deletions(-)

-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup stable-3 v2 1/3] api: admin: datastore: refactor BackupGroup to GroupListItem conversion

[Christian Ebner]

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

We will reuse this later.

No functionial change intended.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Link: https://lore.proxmox.com/20251008134344.3512958-5-d.csapak@proxmox.com
(cherry picked from commit 45693bfa7a13e780a29e12f4be00f4efc742ebe0)
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 1:
- include cherry-picked commit digest and missing signed-off trailer

 src/api2/admin/datastore.rs | 107 +++++++++++++++++++++---------------
 1 file changed, 62 insertions(+), 45 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index cc7e17a29..1f097a555 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -195,57 +195,74 @@ pub fn list_groups(
         .try_fold(Vec::new(), |mut group_info, group| {
             let group = group?;
 
-            let owner = match datastore.get_owner(&ns, group.as_ref()) {
-                Ok(auth_id) => auth_id,
-                Err(err) => {
-                    eprintln!(
-                        "Failed to get owner of group '{}' in {} - {}",
-                        group.group(),
-                        print_store_and_ns(&store, &ns),
-                        err
-                    );
-                    return Ok(group_info);
-                }
-            };
-            if !list_all && check_backup_owner(&owner, &auth_id).is_err() {
-                return Ok(group_info);
+            let item =
+                backup_group_to_group_list_item(datastore.clone(), group, &ns, &auth_id, list_all);
+
+            if let Some(item) = item {
+                group_info.push(item);
             }
 
-            let snapshots = match group.list_backups() {
-                Ok(snapshots) => snapshots,
-                Err(_) => return Ok(group_info),
-            };
+            Ok(group_info)
+        })
+}
 
-            let backup_count: u64 = snapshots.len() as u64;
-            if backup_count == 0 {
-                return Ok(group_info);
-            }
+fn backup_group_to_group_list_item(
+    datastore: Arc<DataStore>,
+    group: pbs_datastore::BackupGroup,
+    ns: &BackupNamespace,
+    auth_id: &Authid,
+    list_all: bool,
+) -> Option<GroupListItem> {
+    let owner = match datastore.get_owner(ns, group.as_ref()) {
+        Ok(auth_id) => auth_id,
+        Err(err) => {
+            eprintln!(
+                "Failed to get owner of group '{}' in {} - {}",
+                group.group(),
+                print_store_and_ns(datastore.name(), ns),
+                err
+            );
+            return None;
+        }
+    };
+    if !list_all && check_backup_owner(&owner, auth_id).is_err() {
+        return None;
+    }
 
-            let last_backup = snapshots
-                .iter()
-                .fold(&snapshots[0], |a, b| {
-                    if a.is_finished() && a.backup_dir.backup_time() > b.backup_dir.backup_time() {
-                        a
-                    } else {
-                        b
-                    }
-                })
-                .to_owned();
-
-            let notes_path = datastore.group_notes_path(&ns, group.as_ref());
-            let comment = file_read_firstline(notes_path).ok();
-
-            group_info.push(GroupListItem {
-                backup: group.into(),
-                last_backup: last_backup.backup_dir.backup_time(),
-                owner: Some(owner),
-                backup_count,
-                files: last_backup.files,
-                comment,
-            });
+    let snapshots = match group.list_backups() {
+        Ok(snapshots) => snapshots,
+        Err(_) => return None,
+    };
 
-            Ok(group_info)
+    let backup_count: u64 = snapshots.len() as u64;
+    if backup_count == 0 {
+        return None;
+    }
+
+    let last_backup = snapshots
+        .iter()
+        .fold(&snapshots[0], |a, b| {
+            if a.is_finished() && a.backup_dir.backup_time() > b.backup_dir.backup_time() {
+                a
+            } else {
+                b
+            }
         })
+        .to_owned();
+
+    let notes_path = datastore.group_notes_path(ns, group.as_ref());
+    let comment = file_read_firstline(notes_path).ok();
+
+    let item = GroupListItem {
+        backup: group.into(),
+        last_backup: last_backup.backup_dir.backup_time(),
+        owner: Some(owner),
+        backup_count,
+        files: last_backup.files,
+        comment,
+    };
+
+    Some(item)
 }
 
 #[api(
-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup stable-3 v2 2/3] api: admin: datastore: factor out 'get_group_owner'

[Christian Ebner]

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

and change the `eprintln` to a `log::warn`

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Link: https://lore.proxmox.com/20251008134344.3512958-6-d.csapak@proxmox.com
(cherry picked from commit 9af3b391b6c465371b895e3d7b81b3fcc640feb6)
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 1:
- include cherry-picked commit digest and missing signed-off trailer

 src/api2/admin/datastore.rs | 47 +++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 1f097a555..9535c3417 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -213,18 +213,8 @@ fn backup_group_to_group_list_item(
     auth_id: &Authid,
     list_all: bool,
 ) -> Option<GroupListItem> {
-    let owner = match datastore.get_owner(ns, group.as_ref()) {
-        Ok(auth_id) => auth_id,
-        Err(err) => {
-            eprintln!(
-                "Failed to get owner of group '{}' in {} - {}",
-                group.group(),
-                print_store_and_ns(datastore.name(), ns),
-                err
-            );
-            return None;
-        }
-    };
+    let owner = get_group_owner(datastore.name(), ns, &group)?;
+
     if !list_all && check_backup_owner(&owner, auth_id).is_err() {
         return None;
     }
@@ -492,6 +482,25 @@ pub async fn list_snapshots(
     .map_err(|err| format_err!("failed to await blocking task: {err}"))?
 }
 
+fn get_group_owner(
+    store: &str,
+    ns: &BackupNamespace,
+    group: &pbs_datastore::BackupGroup,
+) -> Option<Authid> {
+    match group.get_owner() {
+        Ok(auth_id) => Some(auth_id),
+        Err(err) => {
+            log::warn!(
+                "Failed to get owner of group '{}' in {} - {}",
+                group.group(),
+                print_store_and_ns(store, ns),
+                err
+            );
+            None
+        }
+    }
+}
+
 /// This must not run in a main worker thread as it potentially does tons of I/O.
 unsafe fn list_snapshots_blocking(
     store: String,
@@ -605,17 +614,9 @@ unsafe fn list_snapshots_blocking(
     };
 
     groups.iter().try_fold(Vec::new(), |mut snapshots, group| {
-        let owner = match group.get_owner() {
-            Ok(auth_id) => auth_id,
-            Err(err) => {
-                eprintln!(
-                    "Failed to get owner of group '{}' in {} - {}",
-                    group.group(),
-                    print_store_and_ns(&store, &ns),
-                    err
-                );
-                return Ok(snapshots);
-            }
+        let owner = match get_group_owner(&store, &ns, group) {
+            Some(auth_id) => auth_id,
+            None => return Ok(snapshots),
         };
 
         if !list_all && check_backup_owner(&owner, &auth_id).is_err() {
-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup stable-3 v2 3/3] api: admin: datastore: optimize `groups` api call

[Christian Ebner]

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

Currently we always touch all files for each snapshot in a group when
listing them, even though we don't need all that info.

We're only interested in getting either the last finished snapshot
information, or the last unfinished one (which must the only one in
normal use, we can't have multiple unfinished snapshots usually)

Instead of getting all the information upfront, use the snapshot
iterator of the group to get only the id, sort them by time, and
use the first we're interested in, getting the snapshot specific info
only for those we want to check.

In my (admittedly extreme) setup with ~600 groups with ~1000 snapshots
each, this changes the time this api call needs from ~40s to <1s on a
relatively fast disk.

While at it, lift the restriction of only returning groups with
snapshots in them, now returning also empty ones.

To keep api compatibility, use a timestamp of 0 for those, as no valid
backup could have been made at that time anyway.

This API call is currently used by the web UI to add the group notes
to the backup snapshot tree in the datastore content view, so while it
will help in a few cases, it will not make the listing of all
snapshots itself faster.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Link: https://lore.proxmox.com/20251008134344.3512958-7-d.csapak@proxmox.com
 [TL: add context for where the UI uses this API call]
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
(cherry picked from commit e59d33f4cede466d8b175bab2f292d6e4279c28c)
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 1:
- include cherry-picked commit digest and missing signed-off trailer

 src/api2/admin/datastore.rs | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 9535c3417..2ca451fe4 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -219,36 +219,38 @@ fn backup_group_to_group_list_item(
         return None;
     }
 
-    let snapshots = match group.list_backups() {
-        Ok(snapshots) => snapshots,
+    let mut snapshots: Vec<_> = match group.iter_snapshots() {
+        Ok(snapshots) => snapshots.collect::<Result<Vec<_>, Error>>().ok()?,
         Err(_) => return None,
     };
 
     let backup_count: u64 = snapshots.len() as u64;
-    if backup_count == 0 {
-        return None;
-    }
+    let last = if backup_count == 1 {
+        // we may have only one unfinished snapshot
+        snapshots.pop().and_then(|dir| BackupInfo::new(dir).ok())
+    } else {
+        // we either have no snapshots, or at least one finished one, since we cannot have
+        // multiple unfinished ones
+        snapshots.sort_by_key(|b| std::cmp::Reverse(b.backup_time()));
+        snapshots
+            .iter()
+            .filter_map(|backup| BackupInfo::new(backup.clone()).ok())
+            .find(|info| info.is_finished())
+    };
 
-    let last_backup = snapshots
-        .iter()
-        .fold(&snapshots[0], |a, b| {
-            if a.is_finished() && a.backup_dir.backup_time() > b.backup_dir.backup_time() {
-                a
-            } else {
-                b
-            }
-        })
-        .to_owned();
+    let (last_backup, files) = last
+        .map(|info| (info.backup_dir.backup_time(), info.files))
+        .unwrap_or((0, Vec::new()));
 
     let notes_path = datastore.group_notes_path(ns, group.as_ref());
     let comment = file_read_firstline(notes_path).ok();
 
     let item = GroupListItem {
         backup: group.into(),
-        last_backup: last_backup.backup_dir.backup_time(),
+        last_backup,
         owner: Some(owner),
         backup_count,
-        files: last_backup.files,
+        files,
         comment,
     };
 
-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] applied: [PATCH proxmox-backup stable-3 v2 0/3] Backport group listing optimizations including bugfix

[Thomas Lamprecht]

On Mon, 27 Oct 2025 11:31:20 +0100, Christian Ebner wrote:
> These patches were part of the patch series to introduce the
> streaming content api for PBS [0], optimizing the backup group
> listing.
> 
> They did however indirectly fix also a bug in the group listing
> api call, which lead to the `last-backup` time to be returned
> incorrectly in some occasions. This was found and disected in
> issue 6956 [1], identifying an incorrect is_finished() check while
> folding the snapshot iterator to determine the last finished snapshot
> of a group.
> 
> [...]

Applied, thanks!

[1/3] api: admin: datastore: refactor BackupGroup to GroupListItem conversion
      commit: ca08eba0b3e0caab05b3bf466e55a47307422883
[2/3] api: admin: datastore: factor out 'get_group_owner'
      commit: 7c7a05cf6beeac24e3ea553c5ee380a97762e6dd
[3/3] api: admin: datastore: optimize `groups` api call
      commit: d90d7ff230c48b30c94933d465b3aa0ed40ce32f


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel