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 848241FF18A for ; Mon, 26 May 2025 16:14:48 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2670E33EEC; Mon, 26 May 2025 16:14:58 +0200 (CEST) From: Hannes Laimer To: pbs-devel@lists.proxmox.com Date: Mon, 26 May 2025 16:14:33 +0200 Message-Id: <20250526141445.228717-1-h.laimer@proxmox.com> X-Mailer: git-send-email 2.39.5 MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.026 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pbs-devel] [PATCH proxmox-backup v2 00/12] 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 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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 three times - once in datastore_lookup() - 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. -(new in v2) and the checking of if a gc is currently running is now done without the need for a datastore reference instead we just try to get the gc lock directly from the cached write reference(only if one even exists) of the datastore in question. This was only used once by the job scheduler, now we just call a function that checks the relevant cache entries instead of actually getting the whole store reference. changes since v1: - seal trait implementations - re-structure patches - changed how checking if gc is running is done - "rebased" onto master, was actually mostly rewritten, given the age and type of changes it just wouldn't really apply all that well anymore... - we used Operation::Read for verification, turns out verification does also rename currupted chunks, only noticed because the compiler yelled at me :). Not necessarily changed from v1, but didn't mention it there. -- Since I didn't add new comp times for v1, @Wolfgang suggested to maybe monomorphise some functions manually to potentially reduce the impact on comp time/binary sizes. But given the minimal differences on comp time and binary sizes, I don't think that would be worth the effort. Binary sizes were unchanged(`ls -lah`). Compile times: | dbg | release --------|------|--------- master | 52s | 92s series | 53s | 94s individual measurements: * master -> dbg: 52s,52s,53s release: 92s,93s,92s * series -> dbg: 53s,53s,53s release: 94s,96s,95s Hannes Laimer (12): chunkstore: add CanRead and CanWrite trait chunkstore: separate functions into impl block datastore: add generics and new lookup functions datastore: separate functions into impl block backup_info: add generics and separate functions into impl blocks pbs-datastore: add generics and separate functions into impl blocks api: backup: env: add generics and separate functions into impl block api/backup/bin/server/tape: add missing generics examples/tests: add missing generics api: admin: pull datastore loading out of check_privs helper datastore: move `fn gc_running` out of DataStoreImpl api/server: replace datastore_lookup with new, state-typed datastore returning functions pbs-datastore/examples/ls-snapshots.rs | 4 +- pbs-datastore/src/backup_info.rs | 579 ++++---- pbs-datastore/src/chunk_store.rs | 329 +++-- pbs-datastore/src/datastore.rs | 1342 ++++++++++--------- pbs-datastore/src/dynamic_index.rs | 22 +- pbs-datastore/src/fixed_index.rs | 50 +- pbs-datastore/src/hierarchy.rs | 92 +- pbs-datastore/src/lib.rs | 3 +- pbs-datastore/src/local_chunk_reader.rs | 13 +- pbs-datastore/src/prune.rs | 19 +- pbs-datastore/src/snapshot_reader.rs | 31 +- src/api2/admin/datastore.rs | 161 +-- src/api2/admin/namespace.rs | 10 +- src/api2/backup/environment.rs | 337 ++--- src/api2/backup/mod.rs | 29 +- src/api2/backup/upload_chunk.rs | 19 +- src/api2/config/datastore.rs | 5 +- src/api2/reader/environment.rs | 30 +- src/api2/reader/mod.rs | 13 +- src/api2/status/mod.rs | 8 +- src/api2/tape/backup.rs | 21 +- src/api2/tape/drive.rs | 3 +- src/api2/tape/restore.rs | 83 +- src/backup/hierarchy.rs | 23 +- src/backup/verify.rs | 53 +- src/bin/proxmox-backup-proxy.rs | 26 +- src/server/gc_job.rs | 7 +- src/server/prune_job.rs | 9 +- src/server/pull.rs | 32 +- src/server/push.rs | 7 +- src/server/sync.rs | 13 +- src/server/verify_job.rs | 4 +- src/tape/file_formats/snapshot_archive.rs | 5 +- src/tape/pool_writer/mod.rs | 11 +- src/tape/pool_writer/new_chunks_iterator.rs | 7 +- tests/prune.rs | 8 +- 36 files changed, 1794 insertions(+), 1614 deletions(-) -- 2.39.5 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel