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

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

[Hannes Laimer]

... 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);
 
@@ -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

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

[Fabian Grünbichler]

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

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

[Hannes Laimer]

On Thu Apr 25, 2024 at 10:41 AM CEST, Fabian Grünbichler wrote:
> 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?
>
actually yes, I missed that we check for active operations when
deleting. we have to keep the lock at least until we updated the
operations, so we just don't drop it manually. I'll send a v2

> >  
> > @@ -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



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