[pbs-devel] superseded: [PATCH proxmox-backup RFC 00/10] introduce typestate for datastore/chunkstore

[Hannes Laimer]

superseded by 
https://lore.proxmox.com/pbs-devel/20240903123401.91513-1-h.laimer@proxmox.com/

On 9/3/24 14:33, 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<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 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.
> 
>   
> 
> Hannes Laimer (10):
>    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: replace datastore_lookup with new, state-typed datastore
>      returning functions
>    server/bin: replace datastore_lookup with new, state-typed datastore
>      returning functions
>    api: add generics and separate functions into impl blocks
>    backup/server/tape: add generics and separate functions into impl
>      blocks
> 
>   pbs-datastore/src/backup_info.rs            |  179 +-
>   pbs-datastore/src/chunk_store.rs            |  228 ++-
>   pbs-datastore/src/datastore.rs              | 1688 ++++++++++---------
>   pbs-datastore/src/dynamic_index.rs          |   22 +-
>   pbs-datastore/src/fixed_index.rs            |   50 +-
>   pbs-datastore/src/hierarchy.rs              |   92 +-
>   pbs-datastore/src/local_chunk_reader.rs     |   13 +-
>   pbs-datastore/src/prune.rs                  |   19 +-
>   pbs-datastore/src/snapshot_reader.rs        |   30 +-
>   src/api2/admin/datastore.rs                 |  170 +-
>   src/api2/admin/namespace.rs                 |    8 +-
>   src/api2/backup/environment.rs              |  176 +-
>   src/api2/backup/mod.rs                      |   25 +-
>   src/api2/backup/upload_chunk.rs             |   19 +-
>   src/api2/reader/environment.rs              |   31 +-
>   src/api2/reader/mod.rs                      |    9 +-
>   src/api2/status.rs                          |    8 +-
>   src/api2/tape/backup.rs                     |   21 +-
>   src/api2/tape/drive.rs                      |    2 +-
>   src/api2/tape/restore.rs                    |   75 +-
>   src/backup/hierarchy.rs                     |   26 +-
>   src/backup/verify.rs                        |   53 +-
>   src/bin/proxmox-backup-proxy.rs             |    4 +-
>   src/server/gc_job.rs                        |    8 +-
>   src/server/prune_job.rs                     |    9 +-
>   src/server/pull.rs                          |   29 +-
>   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 +-
>   30 files changed, 1585 insertions(+), 1436 deletions(-)
>