Re: [pbs-devel] [PATCH proxmox-backup v1 1/3] fix #3935: datastore/api2/backup: move datastore locking to '/run'

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Thanks for this, no in-depth review/comments, but some higher-level/meta ones,
sorry for that ;-)

On 06.04.22 11:39, Stefan Sterz wrote:
> to avoid issues when removing a group or snapshot directory where two
> threads hold a lock to the same directory, move locking to the tmpfs
> backed '/run' directory. also adds double stat'ing to make it
> possible to remove locks without certain race condition issues.

maybe it would be slightly easier to review (or "conserver" in the git
history) if adding the lock methods and changing to /run + double-stat
would be in two separate patches?

> 
> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
> i chose to have the lock be the sha256 hash of the relative path of a

while really not user friendly it /could/ be ok to do and from a technical
POV it would make it a bit simpler.

> given group/snapshot/manifest in a datastore, because this way we
> never run into issues where file names might be too long. this has
> been discussed in previously[1]. i could also encode them using

don't see the hash stuff discussed anywhere in [1]? Or did you mean the
"it can get long" point?

> proxmox_sys::systemd::escape_unit, but that would use two characters
> for most ascii characters and, thus, we would run into the 255 byte

how would that use two chars for every ascii?  As mostly / and - would be
affected?

> filename limit even faster in most cases. however, i have no set
> opinion here, if the consesus is, that this is a non-issue ill gladly
> send a v2.
> 
> [1]: https://lists.proxmox.com/pipermail/pbs-devel/2020-December/001669.html
> 
>  pbs-datastore/src/datastore.rs       | 129 +++++++++++++++++++++------
>  pbs-datastore/src/snapshot_reader.rs |  30 ++++---
>  src/api2/backup/environment.rs       |   4 +-
>  src/api2/backup/mod.rs               |   5 +-
>  src/api2/reader/mod.rs               |   7 +-
>  src/backup/verify.rs                 |  12 +--
>  6 files changed, 131 insertions(+), 56 deletions(-)