Re: [PATCH proxmox-backup v4 1/7] datastore: add namespace-level locking

[Christian Ebner <c.ebner@proxmox.com>]

On 3/13/26 8:39 AM, Hannes Laimer wrote:
> On 2026-03-12 16:43, Christian Ebner wrote:
>> One high level comment: I think it would make sense to allow to set an
>> overall timeout when acquiring the namespace locks. Most move operations
>> probably are rather fast anyways, so waiting a few seconds in a e.g.
>> prune job will probably be preferable over parts of the prune job being
>> skipped.
>>
> 
> set as in have it configurable? Or just defining one for the locks?
> At least with move in mind, I though that waiting for a lock could mean
> that the things might be gone by the time we get the lock, 50/50 chance
> of that happening actually. So i figured it'd be better to not have
> that, technicaly shouldn't be a problem cause I think we do all
> existance checks after locking, but still, waiting for something that is
> currently being moved (away 1 in 2 times) seemed odd.
> 
> not super oppoed though for a few seconds of hard-coded waiting.

Yes, I was more thinking of a small (hard coded) timeout, which is only 
used when running inside a worker task where it is okay to wait for a 
bit. Similar to what is done when trying to acquire the lock for backups 
in PVE. Although I see the point that the entity might not be present 
anymore after the timeout. But then at least it is skipped as done 
already. But no strong opinion on this either.

> 
>> Another comment inline.
>>
>> On 3/11/26 4:13 PM, Hannes Laimer wrote:
>>> Add exclusive/shared namespace locking keyed at
>>> /run/proxmox-backup/locks/{store}/{ns}/.ns-lock.
>>>
>>> Operations that read from or write into a namespace hold a shared lock
>>> for their duration. Structural operations (move, delete) hold an
>>> exclusive lock. The shared lock is hierarchical: locking a/b/c also
>>> locks a/b and a, so an exclusive lock on any ancestor blocks all
>>> active operations below it. Walking up the ancestor chain costs
>>> O(depth), which is bounded by the maximum namespace depth of 8,
>>> whereas locking all descendants would be arbitrarily expensive.
>>>
>>> Backup jobs and pull/push sync acquire the shared lock via
>>> create_locked_backup_group and pull_ns/push_namespace respectively.
>>> Verify and prune acquire it per snapshot/group and skip gracefully if
>>> the lock cannot be taken, since a concurrent move is a transient
>>> condition.
>>>
>>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>>> ---
>>>    pbs-datastore/src/backup_info.rs | 92 ++++++++++++++++++++++++++++++++
>>>    pbs-datastore/src/datastore.rs   | 45 +++++++++++++---
>>>    src/api2/admin/namespace.rs      |  6 ++-
>>>    src/api2/backup/environment.rs   |  4 ++
>>>    src/api2/backup/mod.rs           | 14 +++--
>>>    src/api2/tape/restore.rs         |  9 ++--
>>>    src/backup/verify.rs             | 19 ++++++-
>>>    src/server/prune_job.rs          | 11 ++++
>>>    src/server/pull.rs               |  8 ++-
>>>    src/server/push.rs               |  6 +++
>>>    10 files changed, 193 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/
>>> backup_info.rs
>>> index c33eb307..476daa61 100644
>>> --- a/pbs-datastore/src/backup_info.rs
>>> +++ b/pbs-datastore/src/backup_info.rs
>>> @@ -937,6 +937,98 @@ fn lock_file_path_helper(ns: &BackupNamespace,
>>> path: PathBuf) -> PathBuf {
>>>        to_return.join(format!("{first_eigthy}...{last_eighty}-{hash}"))
>>>    }
>>>    +/// Returns the lock file path for a backup namespace.
>>> +///
>>> +/// The lock file will be located at:
>>> +/// `${DATASTORE_LOCKS_DIR}/${store_name}/${ns_colon_encoded}/.ns-lock`
>>> +pub(crate) fn ns_lock_path(store_name: &str, ns: &BackupNamespace) ->
>>> PathBuf {
>>> +    let ns_part = ns
>>> +        .components()
>>> +        .map(String::from)
>>> +        .reduce(|acc, n| format!("{acc}:{n}"))
>>> +        .unwrap_or_default();
>>> +    Path::new(DATASTORE_LOCKS_DIR)
>>> +        .join(store_name)
>>> +        .join(ns_part)
>>> +        .join(".ns-lock")
>>> +}
>>> +
>>> +/// Holds namespace locks acquired for a structural operation or a
>>> read/write operation.
>>> +///
>>> +/// For exclusive locks the first guard is exclusive (on the
>>> namespace itself), and any remaining
>>> +/// guards are shared (on ancestor namespaces). For shared locks all
>>> guards are shared.
>>> +pub struct NamespaceLockGuard {
>>> +    _guards: Vec<BackupLockGuard>,
>>> +}
>>
>> comment: While it might not be that critical in practice, it probably
>> makes sense to also control the order with which the lock guards are
>> dropped by implementing the Drop trait and consuming the vector in order
>> to avoid possible inconsistencies with the assumed order.
>>
> 
> makes sense, will add in v5, thanks!
> 
>>> +
>>> +fn lock_namespace_exclusive_single(
>>> +    store_name: &str,
>>> +    ns: &BackupNamespace,
>>> +) -> Result<BackupLockGuard, Error> {
>>> +    let path = ns_lock_path(store_name, ns);
>>> +    lock_helper(store_name, &path, |p| {
>>> +        open_backup_lockfile(p, Some(Duration::from_secs(0)), true)
>>> +            .with_context(|| format!("unable to acquire exclusive
>>> namespace lock for '{ns}'"))
>>> +    })
>>> +}
>>> +
>>> +fn lock_namespace_shared_single(
>>> +    store_name: &str,
>>> +    ns: &BackupNamespace,
>>> +) -> Result<BackupLockGuard, Error> {
>>> +    let path = ns_lock_path(store_name, ns);
>>> +    lock_helper(store_name, &path, |p| {
>>> +        open_backup_lockfile(p, Some(Duration::from_secs(0)), false)
>>> +            .with_context(|| format!("unable to acquire shared
>>> namespace lock for '{ns}'"))
>>> +    })
>>> +}
>>> +
>>> +/// Acquires an exclusive lock on `ns` and shared locks on all its
>>> non-root ancestors.
>>> +///
>>> +/// Used by operations that structurally modify a namespace (e.g.
>>> move). The shared ancestor locks
>>> +/// ensure that a concurrent structural operation on any ancestor
>>> namespace blocks until this one
>>> +/// completes, and vice versa - mirroring the hierarchical behavior
>>> of `lock_namespace_shared`.
>>> +pub(crate) fn lock_namespace(
>>> +    store_name: &str,
>>> +    ns: &BackupNamespace,
>>> +) -> Result<NamespaceLockGuard, Error> {
>>> +    // Acquire the exclusive lock on ns first, then shared locks on
>>> ancestors.
>>> +    // Order matters: taking exclusive before shared avoids a
>>> scenario where we hold shared on
>>> +    // an ancestor and then block waiting for exclusive on ns (which
>>> another holder of that
>>> +    // ancestor's shared lock might be waiting to promote - not
>>> currently possible but fragile).
>>> +    let exclusive = lock_namespace_exclusive_single(store_name, ns)?;
>>> +    let mut guards = vec![exclusive];
>>> +    let mut current = ns.clone();
>>> +    while !current.is_root() {
>>> +        current = current.parent();
>>> +        if !current.is_root() {
>>> +            guards.push(lock_namespace_shared_single(store_name,
>>> &current)?);
>>> +        }
>>> +    }
>>> +    Ok(NamespaceLockGuard { _guards: guards })
>>> +}
>>> +
>>> +/// Acquires shared locks on a namespace and all its non-root ancestors.
>>> +///
>>> +/// Held by operations that read from or write into a namespace
>>> (backup, sync, verify, prune),
>>> +/// preventing concurrent exclusive operations (e.g. move) on the
>>> namespace itself or any ancestor
>>> +/// from proceeding while this guard is alive.
>>> +///
>>> +/// Locking up the ancestor chain (bounded by max namespace depth)
>>> rather than down the subtree
>>> +/// keeps the cost O(depth) regardless of how wide the namespace tree
>>> is.
>>> +pub(crate) fn lock_namespace_shared(
>>> +    store_name: &str,
>>> +    ns: &BackupNamespace,
>>> +) -> Result<NamespaceLockGuard, Error> {
>>> +    let mut guards = Vec::new();
>>> +    let mut current = ns.clone();
>>> +    while !current.is_root() {
>>> +        guards.push(lock_namespace_shared_single(store_name,
>>> &current)?);
>>> +        current = current.parent();
>>> +    }
>>> +    Ok(NamespaceLockGuard { _guards: guards })
>>> +}
>>> +
>>>    /// Helps implement the double stat'ing procedure. It avoids certain
>>> race conditions upon lock
>>>    /// deletion.
>>>    ///
>>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/
>>> datastore.rs
>>> index ef378c69..564c44a1 100644
>>> --- a/pbs-datastore/src/datastore.rs
>>> +++ b/pbs-datastore/src/datastore.rs
>>> @@ -38,7 +38,8 @@ use pbs_config::{BackupLockGuard, ConfigVersionCache};
>>>    use proxmox_section_config::SectionConfigData;
>>>      use crate::backup_info::{
>>> -    BackupDir, BackupGroup, BackupInfo, OLD_LOCKING,
>>> PROTECTED_MARKER_FILENAME,
>>> +    lock_namespace, lock_namespace_shared, BackupDir, BackupGroup,
>>> BackupInfo, NamespaceLockGuard,
>>> +    OLD_LOCKING, PROTECTED_MARKER_FILENAME,
>>>    };
>>>    use crate::chunk_store::ChunkStore;
>>>    use crate::dynamic_index::{DynamicIndexReader, DynamicIndexWriter};
>>> @@ -1123,18 +1124,46 @@ impl DataStore {
>>>            Ok(())
>>>        }
>>>    -    /// Create (if it does not already exists) and lock a backup group
>>> +    /// Acquires an exclusive lock on a backup namespace and shared
>>> locks on all its ancestors.
>>>        ///
>>> -    /// And set the owner to 'userid'. If the group already exists,
>>> it returns the
>>> -    /// current owner (instead of setting the owner).
>>> +    /// Operations that structurally modify a namespace (move,
>>> delete) must hold this for their
>>> +    /// duration to prevent concurrent readers or writers from
>>> accessing the namespace while it is
>>> +    /// being relocated or destroyed.
>>> +    pub fn lock_namespace(
>>> +        self: &Arc<Self>,
>>> +        ns: &BackupNamespace,
>>> +    ) -> Result<NamespaceLockGuard, Error> {
>>> +        lock_namespace(self.name(), ns)
>>> +    }
>>> +
>>> +    /// Acquires shared locks on a backup namespace and all its non-
>>> root ancestors.
>>>        ///
>>> -    /// This also acquires an exclusive lock on the directory and
>>> returns the lock guard.
>>> +    /// Operations that read from or write into a namespace (backup,
>>> sync, verify, prune) must hold
>>> +    /// this for their duration to prevent a concurrent
>>> `move_namespace` or `delete_namespace` from
>>> +    /// relocating or destroying the namespace under them.
>>> +    pub fn lock_namespace_shared(
>>> +        self: &Arc<Self>,
>>> +        ns: &BackupNamespace,
>>> +    ) -> Result<NamespaceLockGuard, Error> {
>>> +        lock_namespace_shared(self.name(), ns)
>>> +    }
>>> +
>>> +    /// Create (if it does not already exist) and lock a backup group.
>>> +    ///
>>> +    /// Sets the owner to `auth_id`. If the group already exists,
>>> returns the current owner.
>>> +    ///
>>> +    /// Returns `(owner, ns_lock, group_lock)`. Both locks must be
>>> kept alive for the duration of
>>> +    /// the backup session: the shared namespace lock prevents
>>> concurrent namespace moves or
>>> +    /// deletions, and the exclusive group lock prevents concurrent
>>> backups to the same group.
>>>        pub fn create_locked_backup_group(
>>>            self: &Arc<Self>,
>>>            ns: &BackupNamespace,
>>>            backup_group: &pbs_api_types::BackupGroup,
>>>            auth_id: &Authid,
>>> -    ) -> Result<(Authid, BackupLockGuard), Error> {
>>> +    ) -> Result<(Authid, NamespaceLockGuard, BackupLockGuard), Error> {
>>> +        let ns_guard = lock_namespace_shared(self.name(), ns)
>>> +            .with_context(|| format!("failed to acquire shared
>>> namespace lock for '{ns}'"))?;
>>> +
>>>            let backup_group = self.backup_group(ns.clone(),
>>> backup_group.clone());
>>>              // create intermediate path first
>>> @@ -1155,14 +1184,14 @@ impl DataStore {
>>>                        return Err(err);
>>>                    }
>>>                    let owner = self.get_owner(ns,
>>> backup_group.group())?; // just to be sure
>>> -                Ok((owner, guard))
>>> +                Ok((owner, ns_guard, guard))
>>>                }
>>>                Err(ref err) if err.kind() ==
>>> io::ErrorKind::AlreadyExists => {
>>>                    let guard = backup_group.lock().with_context(|| {
>>>                        format!("while creating locked backup group
>>> '{backup_group:?}'")
>>>                    })?;
>>>                    let owner = self.get_owner(ns,
>>> backup_group.group())?; // just to be sure
>>> -                Ok((owner, guard))
>>> +                Ok((owner, ns_guard, guard))
>>>                }
>>>                Err(err) => bail!("unable to create backup group {:?} -
>>> {}", full_path, err),
>>>            }
>>> diff --git a/src/api2/admin/namespace.rs b/src/api2/admin/namespace.rs
>>> index 30e24d8d..ec913001 100644
>>> --- a/src/api2/admin/namespace.rs
>>> +++ b/src/api2/admin/namespace.rs
>>> @@ -1,4 +1,4 @@
>>> -use anyhow::{bail, Error};
>>> +use anyhow::{bail, Context, Error};
>>>      use pbs_config::CachedUserInfo;
>>>    use proxmox_router::{http_bail, ApiMethod, Permission, Router,
>>> RpcEnvironment};
>>> @@ -164,6 +164,10 @@ pub fn delete_namespace(
>>>          let datastore = DataStore::lookup_datastore(&store,
>>> Operation::Write)?;
>>>    +    let _ns_lock = datastore
>>> +        .lock_namespace(&ns)
>>> +        .with_context(|| format!("failed to lock namespace '{ns}' for
>>> deletion"))?;
>>> +
>>>        let (removed_all, stats) =
>>> datastore.remove_namespace_recursive(&ns, delete_groups)?;
>>>        if !removed_all {
>>>            let err_msg = if delete_groups {
>>> diff --git a/src/api2/backup/environment.rs b/src/api2/backup/
>>> environment.rs
>>> index ab623f1f..44483add 100644
>>> --- a/src/api2/backup/environment.rs
>>> +++ b/src/api2/backup/environment.rs
>>> @@ -1,5 +1,6 @@
>>>    use anyhow::{bail, format_err, Context, Error};
>>>    use pbs_config::BackupLockGuard;
>>> +use pbs_datastore::backup_info::NamespaceLockGuard;
>>>    use proxmox_sys::process_locker::ProcessLockSharedGuard;
>>>      use std::collections::HashMap;
>>> @@ -100,6 +101,7 @@ struct SharedBackupState {
>>>      pub struct BackupLockGuards {
>>>        previous_snapshot: Option<BackupLockGuard>,
>>> +    _namespace: Option<NamespaceLockGuard>,
>>>        group: Option<BackupLockGuard>,
>>>        snapshot: Option<BackupLockGuard>,
>>>        chunk_store: Option<ProcessLockSharedGuard>,
>>> @@ -108,12 +110,14 @@ pub struct BackupLockGuards {
>>>    impl BackupLockGuards {
>>>        pub(crate) fn new(
>>>            previous_snapshot: Option<BackupLockGuard>,
>>> +        namespace: NamespaceLockGuard,
>>>            group: BackupLockGuard,
>>>            snapshot: BackupLockGuard,
>>>            chunk_store: ProcessLockSharedGuard,
>>>        ) -> Self {
>>>            Self {
>>>                previous_snapshot,
>>> +            _namespace: Some(namespace),
>>>                group: Some(group),
>>>                snapshot: Some(snapshot),
>>>                chunk_store: Some(chunk_store),
>>> diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
>>> index 6df0d34b..80e70dac 100644
>>> --- a/src/api2/backup/mod.rs
>>> +++ b/src/api2/backup/mod.rs
>>> @@ -143,8 +143,9 @@ fn upgrade_to_backup_protocol(
>>>                "backup"
>>>            };
>>>    -        // lock backup group to only allow one backup per group at
>>> a time
>>> -        let (owner, group_guard) = datastore.create_locked_backup_group(
>>> +        // lock backup group to only allow one backup per group at a
>>> time,
>>> +        // also acquires a shared namespace lock to prevent
>>> concurrent namespace moves
>>> +        let (owner, ns_guard, group_guard) =
>>> datastore.create_locked_backup_group(
>>>                backup_group.backup_ns(),
>>>                backup_group.as_ref(),
>>>                &auth_id,
>>> @@ -215,8 +216,13 @@ fn upgrade_to_backup_protocol(
>>>                    // 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 chunk_store_guard =
>>> datastore.try_shared_chunk_store_lock()?;
>>> -                let backup_lock_guards =
>>> -                    BackupLockGuards::new(last_guard, group_guard,
>>> snap_guard, chunk_store_guard);
>>> +                let backup_lock_guards = BackupLockGuards::new(
>>> +                    last_guard,
>>> +                    ns_guard,
>>> +                    group_guard,
>>> +                    snap_guard,
>>> +                    chunk_store_guard,
>>> +                );
>>>                      let mut env = BackupEnvironment::new(
>>>                        env_type,
>>> diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
>>> index 92529a76..1cb85f2d 100644
>>> --- a/src/api2/tape/restore.rs
>>> +++ b/src/api2/tape/restore.rs
>>> @@ -848,11 +848,8 @@ fn restore_list_worker(
>>>                                Some(restore_owner),
>>>                            )?;
>>>    -                        let (owner, _group_lock) =
>>> datastore.create_locked_backup_group(
>>> -                            &ns,
>>> -                            backup_dir.as_ref(),
>>> -                            restore_owner,
>>> -                        )?;
>>> +                        let (owner, _ns_guard, _group_lock) = datastore
>>> +                            .create_locked_backup_group(&ns,
>>> backup_dir.as_ref(), restore_owner)?;
>>>                            if restore_owner != &owner {
>>>                                bail!(
>>>                                    "cannot restore snapshot
>>> '{snapshot}' into group '{}', owner check \
>>> @@ -1354,7 +1351,7 @@ fn restore_archive<'a>(
>>>                            auth_id,
>>>                            Some(restore_owner),
>>>                        )?;
>>> -                    let (owner, _group_lock) =
>>> datastore.create_locked_backup_group(
>>> +                    let (owner, _ns_guard, _group_lock) =
>>> datastore.create_locked_backup_group(
>>>                            &backup_ns,
>>>                            backup_dir.as_ref(),
>>>                            restore_owner,
>>> diff --git a/src/backup/verify.rs b/src/backup/verify.rs
>>> index f52d7781..ce5d6f69 100644
>>> --- a/src/backup/verify.rs
>>> +++ b/src/backup/verify.rs
>>> @@ -393,10 +393,27 @@ impl VerifyWorker {
>>>                return Ok(true);
>>>            }
>>>    +        let ns_lock = match
>>> self.datastore.lock_namespace_shared(backup_dir.backup_ns()) {
>>> +            Ok(lock) => lock,
>>> +            Err(err) => {
>>> +                info!(
>>> +                    "SKIPPED: verify {}:{} - could not acquire
>>> namespace lock: {}",
>>> +                    self.datastore.name(),
>>> +                    backup_dir.dir(),
>>> +                    err,
>>> +                );
>>> +                return Ok(true);
>>> +            }
>>> +        };
>>> +
>>>            let snap_lock = backup_dir.lock_shared();
>>>              match snap_lock {
>>> -            Ok(snap_lock) =>
>>> self.verify_backup_dir_with_lock(backup_dir, upid, filter, snap_lock),
>>> +            Ok(snap_lock) => {
>>> +                let result =
>>> self.verify_backup_dir_with_lock(backup_dir, upid, filter, snap_lock);
>>> +                drop(ns_lock);
>>> +                result
>>> +            }
>>>                Err(err) => {
>>>                    info!(
>>>                        "SKIPPED: verify {}:{} - could not acquire
>>> snapshot lock: {}",
>>> diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
>>> index bb86a323..8103f59c 100644
>>> --- a/src/server/prune_job.rs
>>> +++ b/src/server/prune_job.rs
>>> @@ -54,6 +54,17 @@ pub fn prune_datastore(
>>>        )? {
>>>            let group = group?;
>>>            let ns = group.backup_ns();
>>> +        let _ns_lock = match datastore.lock_namespace_shared(ns) {
>>> +            Ok(lock) => lock,
>>> +            Err(err) => {
>>> +                warn!(
>>> +                    "skipping prune of group '{}:{}' - could not
>>> acquire namespace lock: {err}",
>>> +                    ns,
>>> +                    group.group(),
>>> +                );
>>> +                continue;
>>> +            }
>>> +        };
>>>            let list = group.list_backups()?;
>>>              let mut prune_info = compute_prune_info(list,
>>> &prune_options.keep)?;
>>> diff --git a/src/server/pull.rs b/src/server/pull.rs
>>> index 0ac6b5b8..40173324 100644
>>> --- a/src/server/pull.rs
>>> +++ b/src/server/pull.rs
>>> @@ -1061,6 +1061,12 @@ async fn pull_ns(
>>>        params: &mut PullParameters,
>>>        encountered_chunks: Arc<Mutex<EncounteredChunks>>,
>>>    ) -> Result<(StoreProgress, SyncStats, bool), Error> {
>>> +    let _ns_lock = params
>>> +        .target
>>> +        .store
>>> +        .lock_namespace_shared(namespace)
>>> +        .with_context(|| format!("failed to acquire shared namespace
>>> lock for '{namespace}'"))?;
>>> +
>>>        let list: Vec<BackupGroup> =
>>> params.source.list_groups(namespace, &params.owner).await?;
>>>          let unfiltered_count = list.len();
>>> @@ -1093,7 +1099,7 @@ async fn pull_ns(
>>>            progress.done_snapshots = 0;
>>>            progress.group_snapshots = 0;
>>>    -        let (owner, _lock_guard) =
>>> +        let (owner, _ns_guard, _lock_guard) =
>>>                match params
>>>                    .target
>>>                    .store
>>> diff --git a/src/server/push.rs b/src/server/push.rs
>>> index 27c5b22d..47067d66 100644
>>> --- a/src/server/push.rs
>>> +++ b/src/server/push.rs
>>> @@ -525,6 +525,12 @@ pub(crate) async fn push_namespace(
>>>        check_ns_remote_datastore_privs(params, &target_namespace,
>>> PRIV_REMOTE_DATASTORE_BACKUP)
>>>            .context("Pushing to remote namespace not allowed")?;
>>>    +    let _ns_lock = params
>>> +        .source
>>> +        .store
>>> +        .lock_namespace_shared(namespace)
>>> +        .with_context(|| format!("failed to acquire shared namespace
>>> lock for '{namespace}'"))?;
>>> +
>>>        let mut list: Vec<BackupGroup> = params
>>>            .source
>>>            .list_groups(namespace, &params.local_user)
>>
>