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

[Stefan Sterz]

On 07.04.22 10:10, Wolfgang Bumiller wrote:
> 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?
>>

sure, ill add that to a v2

>>>
>>> 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).

ok so i personally, i don't have a preference, although i don't
understand the strong dislike for hashes. anyway, what i meant by "has
been previously discussed" is this [1]:

>> So if I see this right, the file will then be
>>
/run/proxmox-backup/.locks/$store/${type}/${id}/${timestamp}.index.json.lck
>>
>> seems reasonable apart from the dot in `.locks` ;-)
>>
>> However, do we really need the directory structure here?
>> Shouldn't a flat `.../locks/${type}.${id}.${timestamp}.index.json` be
>> fine as well? (I don't really mind, it would just be less code ;-) )
>
> for now, ids do not really have a length limit besides the fs filename
> limit of 255 bytes
> and since i had to factor that out, i did for datastore/type as well
> (would look even weirder to use something like:
> .../locks/${datastore}.${type}/${id}/${timestamp}.index.json.lck
> )
>
> though we probably should limit the id length anyway...

so if i understand this correctly, the reasoning for why manifest
locks are deeply nested was that the backup id could by itself reach
the file name limit. i might have missed something, but afaict this is
still the case? if not please correct me. so any encoding or hashing
would need to deal with this in one way or another. hashing has the
advantage of fixed length file names, any encoding will likely make
this problem worse as some characters will always need more than one
character to encode. at least that's what it looks like to me, but id
be happily corrected :)

[1]:
https://lists.proxmox.com/pipermail/pbs-devel/2020-December/001669.html