[pbs-devel] [PATCH proxmox-backup] api: list-snapshots improvements

[pbs-devel] [PATCH proxmox-backup] api: list-snapshots improvements

[Gabriel Goller]

`full_path` did not use the cached time string. By using it we avoid a
call to the super-slow libc strftime.

Fix a comment typo.

`list_snapshots_blocking` doesn't need to be unsafe.

The `protected` attribute of a backup has already been checked, avoid
retrieving it another time.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 pbs-datastore/src/backup_info.rs | 4 +++-
 pbs-datastore/src/datastore.rs   | 2 +-
 src/api2/admin/datastore.rs      | 6 +++---
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index bdfaeabc1d12..414ec878d01a 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -415,7 +415,9 @@ impl BackupDir {
 
     /// Returns the absolute path for backup_dir, using the cached formatted time string.
     pub fn full_path(&self) -> PathBuf {
-        self.store.snapshot_path(&self.ns, &self.dir)
+        let mut path = self.store.base_path();
+        path.push(self.relative_path());
+        path
     }
 
     pub fn protected_file(&self) -> PathBuf {
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 5e2208bca40e..11c6c2499ddb 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -844,7 +844,7 @@ impl DataStore {
         ListGroupsType::new(Arc::clone(self), ns, ty)
     }
 
-    /// Get a streaming iter over top-level backup groups of a datatstore of a particular type,
+    /// Get a streaming iter over top-level backup groups of a datastore of a particular type,
     /// filtered by `Ok` results
     ///
     /// The iterated item's result is already unwrapped, if it contained an error it will be
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 82221c503d16..9c5ef7185b74 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -451,7 +451,7 @@ pub async fn list_snapshots(
 ) -> Result<Vec<SnapshotListItem>, Error> {
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
-    tokio::task::spawn_blocking(move || unsafe {
+    tokio::task::spawn_blocking(move || {
         list_snapshots_blocking(store, ns, backup_type, backup_id, auth_id)
     })
     .await
@@ -459,7 +459,7 @@ pub async fn list_snapshots(
 }
 
 /// This must not run in a main worker thread as it potentially does tons of I/O.
-unsafe fn list_snapshots_blocking(
+fn list_snapshots_blocking(
     store: String,
     ns: Option<BackupNamespace>,
     backup_type: Option<BackupType>,
@@ -505,7 +505,7 @@ unsafe fn list_snapshots_blocking(
             group: group.into(),
             time: info.backup_dir.backup_time(),
         };
-        let protected = info.backup_dir.is_protected();
+        let protected = info.protected;
 
         match get_all_snapshot_files(&info) {
             Ok((manifest, files)) => {
-- 
2.43.0



_______________________________________________
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] api: list-snapshots improvements

[Christian Ebner]

Hi, some initial high level comments, did not test this just yet.

You touch several completely unrelated thing in this patch. I feel that this should be split into at least 3-4 patches:

- fixing the typo
- removing the unsafe block (although this feels like Wolfgang did this on purpose to make a caller aware of possible consequences, see commit message for a577d7d8).
- avoid double retrial of protected flag, (can we also assure that this is not done on purpose, to avoid possible races?)
- refactor of `full_path`

Also, can you see a noticeable difference in listing time for groups with a larger number of backups?
Please give some ballpark figures if so.


_______________________________________________
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] api: list-snapshots improvements

[Gabriel Goller]

On 03.07.2024 15:46, Christian Ebner wrote:
>Hi, some initial high level comments, did not test this just yet.
>
>You touch several completely unrelated thing in this patch. I feel that this should be split into at least 3-4 patches:

Sure, I can do that. I just didn't want to pollute the mailing list with
a bunch of one-line fixes :)

>- fixing the typo
>- removing the unsafe block (although this feels like Wolfgang did this on purpose to make a caller aware of possible consequences, see commit message for a577d7d8).
>- avoid double retrial of protected flag, (can we also assure that this is not done on purpose, to avoid possible races?)
>- refactor of `full_path`
>
>Also, can you see a noticeable difference in listing time for groups with a larger number of backups?
>Please give some ballpark figures if so.

Hmm yeah, I can write some benchmarks quickly, shouldn't be a lot
though, some 100-200 ms maybe.


_______________________________________________
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] api: list-snapshots improvements

[Christian Ebner]

> On 03.07.2024 15:55 CEST Gabriel Goller <g.goller@proxmox.com> wrote:
> 
>  
> On 03.07.2024 15:46, Christian Ebner wrote:
> >Hi, some initial high level comments, did not test this just yet.
> >
> >You touch several completely unrelated thing in this patch. I feel that this should be split into at least 3-4 patches:
> 
> Sure, I can do that. I just didn't want to pollute the mailing list with
> a bunch of one-line fixes :)

But each of these warrants its own patch. Furhter, that allows to selectively apply them.

> 
> >- fixing the typo
> >- removing the unsafe block (although this feels like Wolfgang did this on purpose to make a caller aware of possible consequences, see commit message for a577d7d8).
> >- avoid double retrial of protected flag, (can we also assure that this is not done on purpose, to avoid possible races?)
> >- refactor of `full_path`
> >
> >Also, can you see a noticeable difference in listing time for groups with a larger number of backups?
> >Please give some ballpark figures if so.
> 
> Hmm yeah, I can write some benchmarks quickly, shouldn't be a lot
> though, some 100-200 ms maybe.

Absolute values have no meaning in this regard, as this depends on the number of snapshots in the group. You should give a relative estimate.
Also, my intention was not for you to have to write extra performance tests just for this ;)


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