Re: [pbs-devel] [PATCH proxmox-backup v4 02/22] datastore: make dropping from cache more efficient

[Dietmar Maurer <dietmar@proxmox.com>]

First, I think the commit message it totally misleading. This is not for efficiency, instead, this replaces the former MaintenanceType::Unplugged functionality?

But we still have MaintenanceType::Delete, and I am quite unsure it we should handle those states differently.


> On 16.4.2024 17:23 CEST Hannes Laimer <h.laimer@proxmox.com> wrote:
> 
>  
> ... by not having to read the whole config file on every drop,
> but assume calling `update-datastore-cache` through the scoket implies
> it should be dropped and doing the checking on the sending side.
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  pbs-datastore/src/datastore.rs | 60 +++++++++++++++-------------------
>  src/api2/config/datastore.rs   |  8 +++--
>  2 files changed, 31 insertions(+), 37 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 0685cc84..a7fe3b8c 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -34,7 +34,7 @@ use crate::task_tracking::{self, update_active_operations};
>  use crate::DataBlob;
>  
>  lazy_static! {
> -    static ref DATASTORE_MAP: Mutex<HashMap<String, Arc<DataStoreImpl>>> =
> +    static ref DATASTORE_MAP: Mutex<HashMap<String, (Arc<DataStoreImpl>, bool)>> =
>          Mutex::new(HashMap::new());
>  }
>  
> @@ -111,24 +111,15 @@ impl Drop for DataStore {
>                      last_task = updated_operations.read + updated_operations.write == 0;
>                  }
>              }
> -
> -            // remove datastore from cache iff
> -            //  - last task finished, and
> -            //  - datastore is in a maintenance mode that mandates it
> -            let remove_from_cache = last_task
> -                && pbs_config::datastore::config()
> -                    .and_then(|(s, _)| s.lookup::<DataStoreConfig>("datastore", self.name()))
> -                    .map_or(false, |c| {
> -                        c.get_maintenance_mode().map_or(false, |m| m.is_offline())
> -                    });
> -
> -            if remove_from_cache {
> -                DATASTORE_MAP.lock().unwrap().remove(self.name());
> +            if last_task {
> +                let mut cache = DATASTORE_MAP.lock().unwrap();
> +                if let Some((_, true)) = cache.get(self.name()) {
> +                    cache.remove(self.name());
> +                }
>              }
>          }
>      }
>  }
> -
>  impl DataStore {
>      // This one just panics on everything
>      #[doc(hidden)]
> @@ -169,7 +160,7 @@ impl DataStore {
>          let entry = datastore_cache.get(name);
>  
>          // reuse chunk store so that we keep using the same process locker instance!
> -        let chunk_store = if let Some(datastore) = &entry {
> +        let (chunk_store, drop_from_cache) = if let Some((datastore, drop_from_cache)) = &entry {
>              let last_digest = datastore.last_digest.as_ref();
>              if let Some(true) = last_digest.map(|last_digest| last_digest == &digest) {
>                  return Ok(Arc::new(Self {
> @@ -177,23 +168,26 @@ impl DataStore {
>                      operation,
>                  }));
>              }
> -            Arc::clone(&datastore.chunk_store)
> +            (Arc::clone(&datastore.chunk_store), *drop_from_cache)
>          } else {
>              let tuning: DatastoreTuning = serde_json::from_value(
>                  DatastoreTuning::API_SCHEMA
>                      .parse_property_string(config.tuning.as_deref().unwrap_or(""))?,
>              )?;
> -            Arc::new(ChunkStore::open(
> -                name,
> -                &config.path,
> -                tuning.sync_level.unwrap_or_default(),
> -            )?)
> +            (
> +                Arc::new(ChunkStore::open(
> +                    name,
> +                    &config.path,
> +                    tuning.sync_level.unwrap_or_default(),
> +                )?),
> +                false,
> +            )
>          };
>  
>          let datastore = DataStore::with_store_and_config(chunk_store, config, Some(digest))?;
>  
>          let datastore = Arc::new(datastore);
> -        datastore_cache.insert(name.to_string(), datastore.clone());
> +        datastore_cache.insert(name.to_string(), (datastore.clone(), drop_from_cache));
>  
>          Ok(Arc::new(Self {
>              inner: datastore,
> @@ -211,20 +205,18 @@ impl DataStore {
>          Ok(())
>      }
>  
> -    /// trigger clearing cache entry based on maintenance mode. Entry will only
> -    /// be cleared iff there is no other task running, if there is, the end of the
> +    /// trigger clearing of cache entry. Entry will only be cleared iff
> +    /// there is no other task running, if there is, the end of the
>      /// last running task will trigger the clearing of the cache entry.
>      pub fn update_datastore_cache(name: &str) -> Result<(), Error> {
> -        let (config, _digest) = pbs_config::datastore::config()?;
> -        let datastore: DataStoreConfig = config.lookup("datastore", name)?;
> -        if datastore
> -            .get_maintenance_mode()
> -            .map_or(false, |m| m.is_offline())
> -        {
> -            // the datastore drop handler does the checking if tasks are running and clears the
> -            // cache entry, so we just have to trigger it here
> -            let _ = DataStore::lookup_datastore(name, Some(Operation::Lookup));
> +        let _store = DataStore::lookup_datastore(name, Some(Operation::Lookup));
> +
> +        let mut datastore_cache = DATASTORE_MAP.lock().unwrap();
> +        if let Some((_, drop_from_cache)) = datastore_cache.get_mut(name) {
> +            *drop_from_cache = true;
>          }
> +        drop(datastore_cache);
> +        drop(_store);
>  
>          Ok(())
>      }
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index 3081e1f4..0b3d92f9 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -389,10 +389,12 @@ pub fn update_datastore(
>          data.tuning = update.tuning;
>      }
>  
> -    let mut maintenance_mode_changed = false;
> +    let mut drop_store_from_cache = false;
>      if update.maintenance_mode.is_some() {
> -        maintenance_mode_changed = data.maintenance_mode != update.maintenance_mode;
>          data.maintenance_mode = update.maintenance_mode;
> +        drop_store_from_cache = data
> +            .get_maintenance_mode()
> +            .map_or(false, |m| m.is_offline());
>      }
>  
>      config.set_data(&name, "datastore", &data)?;
> @@ -406,7 +408,7 @@ pub fn update_datastore(
>      }
>  
>      // tell the proxy it might have to clear a cache entry
> -    if maintenance_mode_changed {
> +    if drop_store_from_cache {
>          tokio::spawn(async move {
>              if let Ok(proxy_pid) =
>                  proxmox_rest_server::read_pid(pbs_buildcfg::PROXMOX_BACKUP_PROXY_PID_FN)
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel