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

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

On 3/12/26 4:43 PM, 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.
> 
> 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)

As discussed off-list, the namespace directory itself might be used for 
locking instead of a dedicated file, in order to reduce the number of 
i-nodes which are located on this tmpfs.

>> +        .join(".ns-lock")
>> +}
>> +