Re: [pbs-devel] [PATCH] datastore: fix problem with operations counting

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On April 25, 2024 10:32 am, Hannes Laimer wrote:
> ... if `.chunks/` is not available(deleted/moved) ChunkStore::open
> fails, but that would happen after updating the active operations on the
> datastore, so no reference that could be dropped is returned. Leading to
> the operations counter to always increase. This only updates the counter
> when a reference is returned, not before.
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  pbs-datastore/src/datastore.rs | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index f95da761..2f8952ae 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -158,10 +158,6 @@ impl DataStore {
>              }
>          }
>  
> -        if let Some(operation) = operation {
> -            update_active_operations(name, operation, 1)?;
> -        }
> -
>          // Our operation is registered, unlock the config.
>          drop(config_lock);

this comment here, and the bigger one just out of context at the start
of this fn:

        // Avoid TOCTOU between checking maintenance mode and updating active operation counter, as
        // we use it to decide whether it is okay to delete the datastore.
        let config_lock = pbs_config::datastore::lock_config()?;

make me think that this commit at least lacks a rationale why this
change is safe, but possibly means that we would need to hold the config
lock longer here?

>  
> @@ -172,6 +168,9 @@ impl DataStore {
>          let chunk_store = if let Some(datastore) = &entry {
>              let last_digest = datastore.last_digest.as_ref();
>              if let Some(true) = last_digest.map(|last_digest| last_digest == &digest) {
> +                if let Some(operation) = operation {
> +                    update_active_operations(name, operation, 1)?;
> +                }
>                  return Ok(Arc::new(Self {
>                      inner: Arc::clone(datastore),
>                      operation,
> @@ -195,6 +194,10 @@ impl DataStore {
>          let datastore = Arc::new(datastore);
>          datastore_cache.insert(name.to_string(), datastore.clone());
>  
> +        if let Some(operation) = operation {
> +            update_active_operations(name, operation, 1)?;
> +        }
> +
>          Ok(Arc::new(Self {
>              inner: datastore,
>              operation,
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 


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