[pbs-devel] [PATCH proxmox-backup v8 0/4] refactor datastore locking to use tmpfs

[Shannon Sterz]

On Tue Mar 25, 2025 at 12:26 PM CET, Wolfgang Bumiller wrote:
> The suggested anyhow::Context changes make sense, and maybe the d/rules
> change I proposed in 2/4.
>
> With those this is:
>
> Acked-by: Wolfgang Bumiller <w.bumiller at proxmox.com>

alright, will add those changes and submit a v9. thanks!

>
> On Mon, Mar 24, 2025 at 01:51:54PM +0100, Shannon Sterz wrote:
>> The goal of this series is to make it safer to remove backup groups &
>> snapshots by separating the corresponding directories from their lock
>> files. By moving the lock files to the tmpfs-backed '/run' directory,
>> we also make sure that the lock files get cleaned up when the system
>> reboots.
>>
>> This series refactors the locking mechanism inside the `DataStore`,
>> `BackupDir` and `BackupGroup` traits. In a first step locking methods
>> are added and the existing code is refactored to use them. Commit two
>> derives a lock file name under '/run' for each group/snapshot. It also
>> adds double stat'ing. To avoid issues when upgrading, the file
>> `/run/proxmox-backup/old-locking` is created through a post-install
>> hook which is used to determine whether the system has been rebooted
>> and we can safely use the new locking mechanism.
>>
>> The third commit refactors locking for manifests and brings it in-line
>> with the group/snapshot locks. Finally, the last commit fixes a race
>> condition when changing the owner of a datastore.
>>
>> ----
>> changes from v7 (thanks @ Christian Ebner):
>> * use anyhow's `Context` to provide more context on the call site of a
>>   locking helper call
>> * rebase on top of current master to apply cleanly again
>>
>> changes from v6:
>> * add old locking safe guards to avoid different versions of the locking
>>   mechanism being used at the same time (see discussion here [1]).
>>
>> [1]: https://lore.proxmox.com/pbs-devel/20250306120810.361035-1-m.sandoval@proxmox.com/T/#u
>>
>> changes from v5:
>> * re-phrase commit messages to make it clear which commit actually
>>   fixes the issue and what the commit implies in-terms of semantic
>>   changes for error messages (thanks @ Thomas Lamprecht)
>> * make it so the series applies cleanly again and clean up a newly
>>   added usage of `lock_dir_noblock`
>>
>> changes from v4 (thanks @ Wolfgang Bumiller):
>> * stop using `to_string_lossy()`
>> * switch funtion signature of `create_locked_backup_group()` and
>>   `create_locked_backup_dir` to use the `Arc` version of a datastore.
>> * smaller clippy fixes
>> * rebased on current master
>>
>> changes from v3:
>> * moved patch 2 to the front so it can be applied separatelly more
>>   easily
>> * rebased on current master
>>
>> changes from v2:
>> * different encoding scheme for lock file names
>> * refactored locking methods to be used by the new BackupDir and
>>   BackupGroup traits
>> * adapted lock file names to include namespaces
>>
>> changes from v1 (thanks @ Wolfgang Bumiller & Thomas Lamprecht):
>> * split adding locking helpers and move '/run' into two commits
>> * instead of stat'ing the path of lock file twice, only use the file
>>   descriptor for one of the stat'ing procedures instead
>> * several improvements to helper functions and documentation
>>
>> Shannon Sterz (4):
>>   datastore/api/backup: prepare for fix of #3935 by adding lock helpers
>>   fix #3935: datastore/api/backup: move datastore locking to '/run'
>>   fix #3935: datastore: move manifest locking to new locking method
>>   fix: api: avoid race condition in set_backup_owner
>>
>>  Cargo.toml                           |   2 +-
>>  debian/postinst                      |   5 +
>>  pbs-config/src/lib.rs                |  32 +++-
>>  pbs-datastore/Cargo.toml             |   1 +
>>  pbs-datastore/src/backup_info.rs     | 236 ++++++++++++++++++++++++---
>>  pbs-datastore/src/datastore.rs       |  86 +++++-----
>>  pbs-datastore/src/snapshot_reader.rs |  20 ++-
>>  src/api2/admin/datastore.rs          |  13 +-
>>  src/api2/backup/environment.rs       |  21 +--
>>  src/api2/backup/mod.rs               |  13 +-
>>  src/api2/reader/mod.rs               |  11 +-
>>  src/backup/verify.rs                 |  12 +-
>>  src/server/sync.rs                   |  13 +-
>>  13 files changed, 342 insertions(+), 123 deletions(-)
>>
>> --
>> 2.39.5