From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 7A3B31FF15C for ; Wed, 4 Sep 2024 09:34:41 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4DDEA14659; Wed, 4 Sep 2024 09:35:15 +0200 (CEST) Date: Wed, 4 Sep 2024 09:34:41 +0200 From: Wolfgang Bumiller To: Hannes Laimer Message-ID: References: <20240903123401.91513-1-h.laimer@proxmox.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240903123401.91513-1-h.laimer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.085 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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 - Subject: Re: [pbs-devel] [PATCH proxmox-backup RFC 00/10] introduce typestate for datastore/chunkstore 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: , Reply-To: Proxmox Backup Server development discussion Cc: pbs-devel@lists.proxmox.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" On Tue, Sep 03, 2024 at 02:33:51PM GMT, Hannes Laimer wrote: > This patch series introduces two traits, CanRead and CanWrite, to define whether > a datastore reference is readable, writable, or neither. Functions that read > or write are now implemented in `impl` or `impl` blocks, ensuring > that they are only available to references that are supposed to read/write. > > Motivation: > Currently, we track the number of read/write references of a datastore but we don't > track Lookup operations as they don't read or write, they still need a chunkstore, so > eventhough they don't neccessarily directly do IO, they hold an open file handle. > This is a problem for things like unmounting, currently lookup operations are only really > short, so you'd need really unlucky timing to actually run into problems, but still, > if a datastore is in "offline" maintenance mode, we shouldn't open filehandles on it. > > By encoding state in the type: > 1. We can assign non-readable/writable references for lookup operations. > 2. The compiler ensures correct usage of references. Since it is easy to miss > what might happen a few function calls down the line, having the compiler > yell at you for easily missed things like this, is a really good thing > I think. > > Changes: > * Added CanRead and CanWrite traits. > * Separated functions into impl or impl. > * Introduced three new datastore lookup functions that return concrete types implementing > CanRead, CanWrite, or neither. > * Renamed lookup_datastore() to open_datastore() and made it private. > > The main downside is needing separate datastore caches for read and write references due to > concrete type requirements in the cache HashMap. > > Almost all changes are either adding generics or moving functions into the appropriate > trait implementations. The logic itself is only touched twice, once in datastore_lookup() > and once check_privs_and_load_store() in /api/admin/datastore, this function now only checks > the privs, the datastore opening happens in the endpoint function directly. So apart from some details (like sealing the marker traits and some whitespace issues between the patches etc.), I'd like to get some generic feedback from the others here. The main fear I'm having here is that it might increase codegen time, but IMO there are a bunch of methods where it should be easy to manually monomorphise the actual logic by just wrapping the actual logic in a simple `fn()` right inside the method body. While this is definitely additional work, keep in mind that most of *this* patch set is just moving code between different impl<> blocks, so the changes aren't as huge as they appear. Additionally, I wouldn't consider having to separate the cache into read and write caches a downside. Note: The locking: the process locker - which is one current source of "dangerous uses" - has a FIXME comment about switching to `OFD` locks once they are available in the `nix` crate - which they are already (also, could've just used libc back then?). This might give us a chance to make the locking generally less error prone as well, but will need more detailed analysis. If we *can* make that switch, then eg. creating another `ChunkStore` instance would cease to be dangerous. All this to say: I do like this change. I was at first a bit shocked, that the marker traits ended up seeping through into even the `BackupDir` & friends, but since they are *handles* to the dirs, and not just the mere names, I think this is actually fine, and we get additional safety from the compiler. Some compile-time benchmarks before & after would be nice, though ;-) _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel