[pbs-devel] [PATCH proxmox-backup v2 00/12] introduce typestate for datastore/chunkstore
Hannes Laimer
h.laimer at proxmox.com
Mon May 26 16:14:33 CEST 2025
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<T: CanRead>` or `impl<T: CanWrite>` 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<T: CanRead> or impl<T: CanWrite>.
* 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
More information about the pbs-devel
mailing list