From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id A23F7B6FD for ; Thu, 7 Apr 2022 10:35:54 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A03F729E3 for ; Thu, 7 Apr 2022 10:35:54 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id A3FAC29DA for ; Thu, 7 Apr 2022 10:35:53 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 7BB53459D7 for ; Thu, 7 Apr 2022 10:35:53 +0200 (CEST) Message-ID: Date: Thu, 7 Apr 2022 10:35:52 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Content-Language: en-US To: Wolfgang Bumiller , Thomas Lamprecht Cc: Proxmox Backup Server development discussion References: <20220406093955.3180508-1-s.sterz@proxmox.com> <20220406093955.3180508-2-s.sterz@proxmox.com> <20220407081047.f3szlutdma4t4az4@olga.proxmox.com> From: Stefan Sterz In-Reply-To: <20220407081047.f3szlutdma4t4az4@olga.proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.281 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -2.873 Looks like a legit reply (A) POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com] Subject: Re: [pbs-devel] [PATCH proxmox-backup v1 1/3] fix #3935: datastore/api2/backup: move datastore locking to '/run' X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Apr 2022 08:35:54 -0000 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 >>> --- >>> 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