[pbs-devel] [PATCH proxmox-backup] datastore: avoid calculating protected attribute twice

[pbs-devel] [PATCH proxmox-backup] datastore: avoid calculating protected attribute twice

[Gabriel Goller]

The protected status of the snapshot is retrieved twice. This is slow
because it stat's the .protected file multiple times.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 src/api2/admin/datastore.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index f28fb97fa975..9c5ef7185b74 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -505,7 +505,7 @@ 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] datastore: avoid calculating protected attribute twice

[Christian Ebner]

nit: s/calculating/checking/ as this is not really a calculation in that 
sense. But this does not warrant a new version.

On 7/3/24 17:02, Gabriel Goller wrote:
> The protected status of the snapshot is retrieved twice. This is slow
> because it stat's the .protected file multiple times.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>   src/api2/admin/datastore.rs | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index f28fb97fa975..9c5ef7185b74 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -505,7 +505,7 @@ 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)) => {

This looks good to me, although there is now a possibly increased delay 
between the check and closure invocation, as the closure call can be 
significantly later, e.g. when there are a lot of snapshots in the group 
to list. This is however not problematic, as callers do not rely on this 
and the avoided lookup for the file path existence is beneficial.

Did test this by setting a snapshot to protected and verifying the 
listing is still as expected via:

```
proxmox-backup-client snapshot list <group> --output-format json-pretty
```

So please consider:

Tested-by: Christian Ebner <c.ebner@proxmox.com>
Reviewed-by: Christian Ebner <c.ebner@proxmox.com>


_______________________________________________
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] datastore: avoid calculating protected attribute twice

[Wolfgang Bumiller]

applied, thanks


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