[pbs-devel] [PATCH proxmox-backup v2] fix #6566: backup: api: conditionally drop group and snapshot locks

[pbs-devel] [PATCH proxmox-backup v2] fix #6566: backup: api: conditionally drop group and snapshot locks

[Christian Ebner]

To guarantee consistency by possible concurrent operations, the
backup protocol locks the backup group, the previous backup
snapshot (if any) and holds a lock for the newly created backup
snapshot. All of these are currently stored in the backup worker
task, only released on its destruction.

The backup API however signals a successful backup via the return
status of the `finish` call, while still holding the locks.
Therefore, an immediate subsequent backup of the client to the same
group can fail because the locks cannot be acquired until the previous
backup task is completely destroyed, which can however outlive the
`finish` return for some time. This manifests in e.g. a push sync job
failing.

To fix this, store the lock guards inside the RPC environments shared
state instead, allowing to selectively drop the locks on successful
backup finish. On error, hold the locks until the cleanup was
successful.

Immediate verification of new snapshots already downgraded the lock
by dropping the exclusive lock and getting a shared lock. Since the
dropping is now already handled by the finish call, only gathering
the shared lock is required. While there is now a larger time window
for concurrent prunes, the underlying possible race between
verification and prune remains in place.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6566
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
Changes since version 1:
- rebased onto current master

 src/api2/backup/environment.rs | 38 +++++++++++++++++++++++++++++-----
 src/api2/backup/mod.rs         | 18 +++++++++-------
 2 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index ace305d7e..fa1444ab1 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -96,6 +96,27 @@ struct SharedBackupState {
     known_chunks: KnownChunksMap,
     backup_size: u64, // sums up size of all files
     backup_stat: UploadStatistic,
+    backup_lock_guards: BackupLockGuards,
+}
+
+pub struct BackupLockGuards {
+    previous_snapshot: Option<BackupLockGuard>,
+    group: Option<BackupLockGuard>,
+    snapshot: Option<BackupLockGuard>,
+}
+
+impl BackupLockGuards {
+    pub(crate) fn new(
+        previous_snapshot: Option<BackupLockGuard>,
+        group: BackupLockGuard,
+        snapshot: BackupLockGuard,
+    ) -> Self {
+        Self {
+            previous_snapshot,
+            group: Some(group),
+            snapshot: Some(snapshot),
+        }
+    }
 }
 
 impl SharedBackupState {
@@ -140,6 +161,7 @@ impl BackupEnvironment {
         datastore: Arc<DataStore>,
         backup_dir: BackupDir,
         no_cache: bool,
+        backup_lock_guards: BackupLockGuards,
     ) -> Result<Self, Error> {
         let state = SharedBackupState {
             finished: BackupState::Active,
@@ -150,6 +172,7 @@ impl BackupEnvironment {
             known_chunks: HashMap::new(),
             backup_size: 0,
             backup_stat: UploadStatistic::new(),
+            backup_lock_guards,
         };
 
         let backend = datastore.backend()?;
@@ -719,6 +742,8 @@ impl BackupEnvironment {
                 );
             }
         }
+        // drop previous snapshot lock
+        state.backup_lock_guards.previous_snapshot.take();
 
         let stats = serde_json::to_value(state.backup_stat)?;
 
@@ -744,13 +769,17 @@ impl BackupEnvironment {
         // marks the backup as successful
         state.finished = BackupState::Finished;
 
+        // drop snapshot and group lock only here so any error above will lead to
+        // the locks still being held in the env for the backup cleanup.
+        state.backup_lock_guards.snapshot.take();
+        state.backup_lock_guards.group.take();
+
         Ok(())
     }
 
     /// If verify-new is set on the datastore, this will run a new verify task
-    /// for the backup. If not, this will return and also drop the passed lock
-    /// immediately.
-    pub fn verify_after_complete(&self, excl_snap_lock: BackupLockGuard) -> Result<(), Error> {
+    /// for the backup. If not, this will return.
+    pub fn verify_after_complete(&self) -> Result<(), Error> {
         self.ensure_finished()?;
 
         if !self.datastore.verify_new() {
@@ -758,8 +787,7 @@ impl BackupEnvironment {
             return Ok(());
         }
 
-        // Downgrade to shared lock, the backup itself is finished
-        drop(excl_snap_lock);
+        // Get shared lock, the backup itself is finished
         let snap_lock = self.backup_dir.lock_shared().with_context(|| {
             format!(
                 "while trying to verify snapshot '{:?}' after completion",
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index 8a076a2b0..246b0946d 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -144,7 +144,7 @@ fn upgrade_to_backup_protocol(
         };
 
         // lock backup group to only allow one backup per group at a time
-        let (owner, _group_guard) = datastore.create_locked_backup_group(
+        let (owner, group_guard) = datastore.create_locked_backup_group(
             backup_group.backup_ns(),
             backup_group.as_ref(),
             &auth_id,
@@ -183,7 +183,7 @@ fn upgrade_to_backup_protocol(
 
         let backup_dir = backup_group.backup_dir(backup_dir_arg.time)?;
 
-        let _last_guard = if let Some(last) = &last_backup {
+        let last_guard = if let Some(last) = &last_backup {
             if backup_dir.backup_time() <= last.backup_dir.backup_time() {
                 bail!("backup timestamp is older than last backup.");
             }
@@ -210,6 +210,12 @@ fn upgrade_to_backup_protocol(
             auth_id.to_string(),
             true,
             move |worker| async move {
+                // Keep flock for the backup runtime by storing guards in backup env shared state.
+                // Drop them on successful backup finish or when dropping the env after cleanup in
+                // case of errors. The former is required for immediate subsequent backups (e.g.
+                // during a push sync) to be able to lock the group and snapshots.
+                let backup_lock_guards = BackupLockGuards::new(last_guard, group_guard, snap_guard);
+
                 let mut env = BackupEnvironment::new(
                     env_type,
                     auth_id,
@@ -217,6 +223,7 @@ fn upgrade_to_backup_protocol(
                     datastore,
                     backup_dir,
                     no_cache,
+                    backup_lock_guards,
                 )?;
 
                 env.debug = debug;
@@ -271,11 +278,6 @@ fn upgrade_to_backup_protocol(
                     });
                 let mut abort_future = abort_future.map(|_| Err(format_err!("task aborted")));
 
-                // keep flock until task ends
-                let _group_guard = _group_guard;
-                let snap_guard = snap_guard;
-                let _last_guard = _last_guard;
-
                 let res = select! {
                     req = req_fut => req,
                     abrt = abort_future => abrt,
@@ -293,7 +295,7 @@ fn upgrade_to_backup_protocol(
                 }
 
                 let verify = |env: BackupEnvironment| {
-                    if let Err(err) = env.verify_after_complete(snap_guard) {
+                    if let Err(err) = env.verify_after_complete() {
                         env.log(format!(
                             "backup finished, but starting the requested verify task failed: {}",
                             err
-- 
2.47.3



_______________________________________________
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 v2] fix #6566: backup: api: conditionally drop group and snapshot locks

[Fabian Grünbichler]

On September 29, 2025 5:17 pm, Christian Ebner wrote:
> To guarantee consistency by possible concurrent operations, the
> backup protocol locks the backup group, the previous backup
> snapshot (if any) and holds a lock for the newly created backup
> snapshot. All of these are currently stored in the backup worker
> task, only released on its destruction.
> 
> The backup API however signals a successful backup via the return
> status of the `finish` call, while still holding the locks.
> Therefore, an immediate subsequent backup of the client to the same
> group can fail because the locks cannot be acquired until the previous
> backup task is completely destroyed, which can however outlive the
> `finish` return for some time. This manifests in e.g. a push sync job
> failing.
> 
> To fix this, store the lock guards inside the RPC environments shared
> state instead, allowing to selectively drop the locks on successful
> backup finish. On error, hold the locks until the cleanup was
> successful.
> 
> Immediate verification of new snapshots already downgraded the lock
> by dropping the exclusive lock and getting a shared lock. Since the
> dropping is now already handled by the finish call, only gathering
> the shared lock is required. While there is now a larger time window
> for concurrent prunes, the underlying possible race between
> verification and prune remains in place.
> 
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6566
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>

Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

it should be very rare that a snapshot is attempted to be removed right
as it was created, so the slightly increased race window there should be
okay.

did you test this with vzdump's `protected` option and verify after
completion?

> ---
> Changes since version 1:
> - rebased onto current master
> 
>  src/api2/backup/environment.rs | 38 +++++++++++++++++++++++++++++-----
>  src/api2/backup/mod.rs         | 18 +++++++++-------
>  2 files changed, 43 insertions(+), 13 deletions(-)
> 
> diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
> index ace305d7e..fa1444ab1 100644
> --- a/src/api2/backup/environment.rs
> +++ b/src/api2/backup/environment.rs
> @@ -96,6 +96,27 @@ struct SharedBackupState {
>      known_chunks: KnownChunksMap,
>      backup_size: u64, // sums up size of all files
>      backup_stat: UploadStatistic,
> +    backup_lock_guards: BackupLockGuards,
> +}
> +
> +pub struct BackupLockGuards {
> +    previous_snapshot: Option<BackupLockGuard>,
> +    group: Option<BackupLockGuard>,
> +    snapshot: Option<BackupLockGuard>,
> +}
> +
> +impl BackupLockGuards {
> +    pub(crate) fn new(
> +        previous_snapshot: Option<BackupLockGuard>,
> +        group: BackupLockGuard,
> +        snapshot: BackupLockGuard,
> +    ) -> Self {
> +        Self {
> +            previous_snapshot,
> +            group: Some(group),
> +            snapshot: Some(snapshot),
> +        }
> +    }
>  }
>  
>  impl SharedBackupState {
> @@ -140,6 +161,7 @@ impl BackupEnvironment {
>          datastore: Arc<DataStore>,
>          backup_dir: BackupDir,
>          no_cache: bool,
> +        backup_lock_guards: BackupLockGuards,
>      ) -> Result<Self, Error> {
>          let state = SharedBackupState {
>              finished: BackupState::Active,
> @@ -150,6 +172,7 @@ impl BackupEnvironment {
>              known_chunks: HashMap::new(),
>              backup_size: 0,
>              backup_stat: UploadStatistic::new(),
> +            backup_lock_guards,
>          };
>  
>          let backend = datastore.backend()?;
> @@ -719,6 +742,8 @@ impl BackupEnvironment {
>                  );
>              }
>          }
> +        // drop previous snapshot lock
> +        state.backup_lock_guards.previous_snapshot.take();
>  
>          let stats = serde_json::to_value(state.backup_stat)?;
>  
> @@ -744,13 +769,17 @@ impl BackupEnvironment {
>          // marks the backup as successful
>          state.finished = BackupState::Finished;
>  
> +        // drop snapshot and group lock only here so any error above will lead to
> +        // the locks still being held in the env for the backup cleanup.
> +        state.backup_lock_guards.snapshot.take();
> +        state.backup_lock_guards.group.take();
> +
>          Ok(())
>      }
>  
>      /// If verify-new is set on the datastore, this will run a new verify task
> -    /// for the backup. If not, this will return and also drop the passed lock
> -    /// immediately.
> -    pub fn verify_after_complete(&self, excl_snap_lock: BackupLockGuard) -> Result<(), Error> {
> +    /// for the backup. If not, this will return.
> +    pub fn verify_after_complete(&self) -> Result<(), Error> {
>          self.ensure_finished()?;
>  
>          if !self.datastore.verify_new() {
> @@ -758,8 +787,7 @@ impl BackupEnvironment {
>              return Ok(());
>          }
>  
> -        // Downgrade to shared lock, the backup itself is finished
> -        drop(excl_snap_lock);
> +        // Get shared lock, the backup itself is finished
>          let snap_lock = self.backup_dir.lock_shared().with_context(|| {
>              format!(
>                  "while trying to verify snapshot '{:?}' after completion",
> diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
> index 8a076a2b0..246b0946d 100644
> --- a/src/api2/backup/mod.rs
> +++ b/src/api2/backup/mod.rs
> @@ -144,7 +144,7 @@ fn upgrade_to_backup_protocol(
>          };
>  
>          // lock backup group to only allow one backup per group at a time
> -        let (owner, _group_guard) = datastore.create_locked_backup_group(
> +        let (owner, group_guard) = datastore.create_locked_backup_group(
>              backup_group.backup_ns(),
>              backup_group.as_ref(),
>              &auth_id,
> @@ -183,7 +183,7 @@ fn upgrade_to_backup_protocol(
>  
>          let backup_dir = backup_group.backup_dir(backup_dir_arg.time)?;
>  
> -        let _last_guard = if let Some(last) = &last_backup {
> +        let last_guard = if let Some(last) = &last_backup {
>              if backup_dir.backup_time() <= last.backup_dir.backup_time() {
>                  bail!("backup timestamp is older than last backup.");
>              }
> @@ -210,6 +210,12 @@ fn upgrade_to_backup_protocol(
>              auth_id.to_string(),
>              true,
>              move |worker| async move {
> +                // Keep flock for the backup runtime by storing guards in backup env shared state.
> +                // Drop them on successful backup finish or when dropping the env after cleanup in
> +                // case of errors. The former is required for immediate subsequent backups (e.g.
> +                // during a push sync) to be able to lock the group and snapshots.
> +                let backup_lock_guards = BackupLockGuards::new(last_guard, group_guard, snap_guard);
> +
>                  let mut env = BackupEnvironment::new(
>                      env_type,
>                      auth_id,
> @@ -217,6 +223,7 @@ fn upgrade_to_backup_protocol(
>                      datastore,
>                      backup_dir,
>                      no_cache,
> +                    backup_lock_guards,
>                  )?;
>  
>                  env.debug = debug;
> @@ -271,11 +278,6 @@ fn upgrade_to_backup_protocol(
>                      });
>                  let mut abort_future = abort_future.map(|_| Err(format_err!("task aborted")));
>  
> -                // keep flock until task ends
> -                let _group_guard = _group_guard;
> -                let snap_guard = snap_guard;
> -                let _last_guard = _last_guard;
> -
>                  let res = select! {
>                      req = req_fut => req,
>                      abrt = abort_future => abrt,
> @@ -293,7 +295,7 @@ fn upgrade_to_backup_protocol(
>                  }
>  
>                  let verify = |env: BackupEnvironment| {
> -                    if let Err(err) = env.verify_after_complete(snap_guard) {
> +                    if let Err(err) = env.verify_after_complete() {
>                          env.log(format!(
>                              "backup finished, but starting the requested verify task failed: {}",
>                              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 mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup v2] fix #6566: backup: api: conditionally drop group and snapshot locks

[Christian Ebner]

On 9/30/25 10:44 AM, Fabian Grünbichler wrote:
> On September 29, 2025 5:17 pm, Christian Ebner wrote:
>> To guarantee consistency by possible concurrent operations, the
>> backup protocol locks the backup group, the previous backup
>> snapshot (if any) and holds a lock for the newly created backup
>> snapshot. All of these are currently stored in the backup worker
>> task, only released on its destruction.
>>
>> The backup API however signals a successful backup via the return
>> status of the `finish` call, while still holding the locks.
>> Therefore, an immediate subsequent backup of the client to the same
>> group can fail because the locks cannot be acquired until the previous
>> backup task is completely destroyed, which can however outlive the
>> `finish` return for some time. This manifests in e.g. a push sync job
>> failing.
>>
>> To fix this, store the lock guards inside the RPC environments shared
>> state instead, allowing to selectively drop the locks on successful
>> backup finish. On error, hold the locks until the cleanup was
>> successful.
>>
>> Immediate verification of new snapshots already downgraded the lock
>> by dropping the exclusive lock and getting a shared lock. Since the
>> dropping is now already handled by the finish call, only gathering
>> the shared lock is required. While there is now a larger time window
>> for concurrent prunes, the underlying possible race between
>> verification and prune remains in place.
>>
>> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6566
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> 
> Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> 
> it should be very rare that a snapshot is attempted to be removed right
> as it was created, so the slightly increased race window there should be
> okay.
> 
> did you test this with vzdump's `protected` option and verify after
> completion?

I did not manage to race the backup finish <-> verify after finished.

But there is the (pre-existing) race between pruning and setting the 
protected marker. This can be triggered if timed right, e.g. just did 
with the latest rebased version. The backup task then fails with, e.g.:
```
...
INFO: stopping kvm after backup task
INFO: adding notes to backup
WARN: unable to add notes - proxmox-backup-client failed: Error: unable 
to update manifest blob - unable to load blob 
'"/devel/datastore/vm/100/2025-09-30T09:09:40Z/index.json.blob"' - No 
such file or directory (os error 2)
INFO: marking backup as protected
ERROR: Backup of VM 100 failed - unable to set protected flag - 400 Bad 
Request
INFO: Failed at 2025-09-30 11:09:52
...
```

To fix that, we would need to extend the backup upgrade api endpoint by 
a protected marker flag, so this can be set a-priori and written when 
the manifest is persisted to disk.



_______________________________________________
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 v2] fix #6566: backup: api: conditionally drop group and snapshot locks

[Fabian Grünbichler]

On Mon, 29 Sep 2025 17:17:59 +0200, Christian Ebner wrote:
> To guarantee consistency by possible concurrent operations, the
> backup protocol locks the backup group, the previous backup
> snapshot (if any) and holds a lock for the newly created backup
> snapshot. All of these are currently stored in the backup worker
> task, only released on its destruction.
> 
> The backup API however signals a successful backup via the return
> status of the `finish` call, while still holding the locks.
> Therefore, an immediate subsequent backup of the client to the same
> group can fail because the locks cannot be acquired until the previous
> backup task is completely destroyed, which can however outlive the
> `finish` return for some time. This manifests in e.g. a push sync job
> failing.
> 
> [...]

Applied, thanks!

[1/1] fix #6566: backup: api: conditionally drop group and snapshot locks
      commit: b1ece6c70c7785191321525576ed6f53e9c4bc18

Best regards,
-- 
Fabian Grünbichler <f.gruenbichler@proxmox.com>


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