* [pbs-devel] [PATCH proxmox-backup stable-3 v2 1/3] api: admin: datastore: refactor BackupGroup to GroupListItem conversion
2025-10-27 10:31 [pbs-devel] [PATCH proxmox-backup stable-3 v2 0/3] Backport group listing optimizations including bugfix Christian Ebner
@ 2025-10-27 10:31 ` Christian Ebner
2025-10-27 10:31 ` [pbs-devel] [PATCH proxmox-backup stable-3 v2 2/3] api: admin: datastore: factor out 'get_group_owner' Christian Ebner
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Christian Ebner @ 2025-10-27 10:31 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [pbs-devel] [PATCH proxmox-backup stable-3 v2 2/3] api: admin: datastore: factor out 'get_group_owner'
2025-10-27 10:31 [pbs-devel] [PATCH proxmox-backup stable-3 v2 0/3] Backport group listing optimizations including bugfix Christian Ebner
2025-10-27 10:31 ` [pbs-devel] [PATCH proxmox-backup stable-3 v2 1/3] api: admin: datastore: refactor BackupGroup to GroupListItem conversion Christian Ebner
@ 2025-10-27 10:31 ` Christian Ebner
2025-10-27 10:31 ` [pbs-devel] [PATCH proxmox-backup stable-3 v2 3/3] api: admin: datastore: optimize `groups` api call Christian Ebner
2025-10-27 17:49 ` [pbs-devel] applied: [PATCH proxmox-backup stable-3 v2 0/3] Backport group listing optimizations including bugfix Thomas Lamprecht
3 siblings, 0 replies; 5+ messages in thread
From: Christian Ebner @ 2025-10-27 10:31 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [pbs-devel] [PATCH proxmox-backup stable-3 v2 3/3] api: admin: datastore: optimize `groups` api call
2025-10-27 10:31 [pbs-devel] [PATCH proxmox-backup stable-3 v2 0/3] Backport group listing optimizations including bugfix Christian Ebner
2025-10-27 10:31 ` [pbs-devel] [PATCH proxmox-backup stable-3 v2 1/3] api: admin: datastore: refactor BackupGroup to GroupListItem conversion Christian Ebner
2025-10-27 10:31 ` [pbs-devel] [PATCH proxmox-backup stable-3 v2 2/3] api: admin: datastore: factor out 'get_group_owner' Christian Ebner
@ 2025-10-27 10:31 ` Christian Ebner
2025-10-27 17:49 ` [pbs-devel] applied: [PATCH proxmox-backup stable-3 v2 0/3] Backport group listing optimizations including bugfix Thomas Lamprecht
3 siblings, 0 replies; 5+ messages in thread
From: Christian Ebner @ 2025-10-27 10:31 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [pbs-devel] applied: [PATCH proxmox-backup stable-3 v2 0/3] Backport group listing optimizations including bugfix
2025-10-27 10:31 [pbs-devel] [PATCH proxmox-backup stable-3 v2 0/3] Backport group listing optimizations including bugfix Christian Ebner
` (2 preceding siblings ...)
2025-10-27 10:31 ` [pbs-devel] [PATCH proxmox-backup stable-3 v2 3/3] api: admin: datastore: optimize `groups` api call Christian Ebner
@ 2025-10-27 17:49 ` Thomas Lamprecht
3 siblings, 0 replies; 5+ messages in thread
From: Thomas Lamprecht @ 2025-10-27 17:49 UTC (permalink / raw)
To: pbs-devel, Christian Ebner
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
^ permalink raw reply [flat|nested] 5+ messages in thread