public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Stefan Sterz <s.sterz@proxmox.com>
To: Wolfgang Bumiller <w.bumiller@proxmox.com>,
	Thomas Lamprecht <t.lamprecht@proxmox.com>
Cc: Proxmox Backup Server development discussion
	<pbs-devel@lists.proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox-backup v1 1/3] fix #3935: datastore/api2/backup: move datastore locking to '/run'
Date: Thu, 7 Apr 2022 10:35:52 +0200	[thread overview]
Message-ID: <cf525baa-880c-655c-d696-32aa619832ea@proxmox.com> (raw)
In-Reply-To: <20220407081047.f3szlutdma4t4az4@olga.proxmox.com>

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




  reply	other threads:[~2022-04-07  8:35 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-06  9:39 [pbs-devel] [PATCH proxmox-backup v1 0/3] refactor datastore locking to use tmpfs Stefan Sterz
2022-04-06  9:39 ` [pbs-devel] [PATCH proxmox-backup v1 1/3] fix #3935: datastore/api2/backup: move datastore locking to '/run' Stefan Sterz
2022-04-06 14:51   ` Thomas Lamprecht
2022-04-07  8:10     ` Wolfgang Bumiller
2022-04-07  8:35       ` Stefan Sterz [this message]
2022-04-07  8:45   ` Wolfgang Bumiller
2022-04-06  9:39 ` [pbs-devel] [PATCH proxmox-backup v1 2/3] fix: datastore: move manifest locking to new locking method Stefan Sterz
2022-04-07  8:52   ` Wolfgang Bumiller
2022-04-06  9:39 ` [pbs-devel] [PATCH proxmox-backup v1 3/3] fix: api: avoid race condition in set_backup_owner Stefan Sterz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cf525baa-880c-655c-d696-32aa619832ea@proxmox.com \
    --to=s.sterz@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    --cc=t.lamprecht@proxmox.com \
    --cc=w.bumiller@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal