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

[Wolfgang Bumiller]

On Wed, Apr 06, 2022 at 04:51:50PM +0200, Thomas Lamprecht wrote:
> 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 at 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.

I really dislike hashes here very much 😕.

> 
> > 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?

Given our schemas it wouldn't be all that bad I think?  But it really
does escape a *lot* and we may as well just use percent encoding with a
less inconvenient escape list (eg. '/', '\', '%' & control chars).