public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Hannes Laimer <h.laimer@proxmox.com>
To: pbs-devel@lists.proxmox.com
Subject: Re: [pbs-devel] superseded: [PATCH proxmox-backup RFC 00/10] introduce typestate for datastore/chunkstore
Date: Mon, 26 May 2025 16:18:34 +0200	[thread overview]
Message-ID: <e1a6415a-2555-41de-a231-1eef126afea7@proxmox.com> (raw)
In-Reply-To: <20240903123401.91513-1-h.laimer@proxmox.com>

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(-)
> 



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


      parent reply	other threads:[~2025-05-26 14:18 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-09-03 12:33 [pbs-devel] " Hannes Laimer
2024-09-03 12:33 ` [pbs-devel] [PATCH proxmox-backup RFC 01/10] chunkstore: add CanRead and CanWrite trait Hannes Laimer
2024-09-03 12:33 ` [pbs-devel] [PATCH proxmox-backup RFC 02/10] chunkstore: separate functions into impl block Hannes Laimer
2024-09-03 12:33 ` [pbs-devel] [PATCH proxmox-backup RFC 03/10] datastore: add generics and new lookup functions Hannes Laimer
2024-09-03 12:33 ` [pbs-devel] [PATCH proxmox-backup RFC 04/10] datastore: separate functions into impl block Hannes Laimer
2024-09-03 12:33 ` [pbs-devel] [PATCH proxmox-backup RFC 05/10] backup_info: add generics and separate functions into impl blocks Hannes Laimer
2024-09-03 12:33 ` [pbs-devel] [PATCH proxmox-backup RFC 06/10] pbs-datastore: " Hannes Laimer
2024-09-03 12:33 ` [pbs-devel] [PATCH proxmox-backup RFC 07/10] api: replace datastore_lookup with new, state-typed datastore returning functions Hannes Laimer
2024-09-03 12:33 ` [pbs-devel] [PATCH proxmox-backup RFC 08/10] server/bin: " Hannes Laimer
2024-09-03 12:34 ` [pbs-devel] [PATCH proxmox-backup RFC 09/10] api: add generics and separate functions into impl blocks Hannes Laimer
2024-09-03 12:34 ` [pbs-devel] [PATCH proxmox-backup RFC 10/10] backup/server/tape: " Hannes Laimer
2024-09-04  7:34 ` [pbs-devel] [PATCH proxmox-backup RFC 00/10] introduce typestate for datastore/chunkstore Wolfgang Bumiller
2025-05-26 14:18 ` Hannes Laimer [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e1a6415a-2555-41de-a231-1eef126afea7@proxmox.com \
    --to=h.laimer@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal