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

[pbs-devel] [PATCH proxmox-backup stable-3 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.39.5



_______________________________________________
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 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
---
 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.39.5



_______________________________________________
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 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
---
 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.39.5



_______________________________________________
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 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>
---
 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.39.5



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

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

[Thomas Lamprecht]

Am 27.10.25 um 10:33 schrieb 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.

They all miss your S-o-b trailer, and ideally add the original commit
they got backported from, if you use `git -xs cherry-pick <ref>` all
this happens automatically.

> 
> 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(-)
> 



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

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

[Christian Ebner]

On 10/27/25 11:21 AM, Thomas Lamprecht wrote:
> Am 27.10.25 um 10:33 schrieb 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.
> 
> They all miss your S-o-b trailer, and ideally add the original commit
> they got backported from, if you use `git -xs cherry-pick <ref>` all
> this happens automatically.

Thanks for the hint! Will resend the series as v2  with that fixed shortly.



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

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

[Thomas Lamprecht]

Am 27.10.25 um 11:24 schrieb Christian Ebner:
> Thanks for the hint! Will resend the series as v2  with that fixed shortly.

I juggled the argument order though, sorry. The "-x" and the "-s" are options for
"cherry-pick" and come after that, not before, i.e. this is what I meant to
write:

# git cherry-pick -xs <ref>


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

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

[Christian Ebner]

superseded-by version 2:
https://lore.proxmox.com/pbs-devel/20251027103123.101013-1-c.ebner@proxmox.com/T/


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