[pbs-devel] [PATCH proxmox-backup v2 00/12] introduce typestate for datastore/chunkstore

[Hannes Laimer <h.laimer@proxmox.com>]

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



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel