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

[Wolfgang Bumiller]

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>

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