[pbs-devel] [PATCH proxmox-backup] pbs-datastore: use ConfigVersionCache for datastore

[pbs-devel] [PATCH proxmox-backup] pbs-datastore: use ConfigVersionCache for datastore

[Dominik Csapak]

instead of relying on the content of some configs

previously, we always read and parsed the config file, and only
generated a new config object when the path or the 'verify-new' option
changed.

now, we increase the datastore generation on config save, and if that
changed (or the last load is 1 minute in the past), we always
generate a new object

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
i'm a bit torn by removing the checks for the changed config parts...
on one hand, this fixes the 'we forgot to add our new option' problem,
but on the other hand, we now basically create a new datastore object
every minute

 pbs-config/src/config_version_cache.rs | 18 ++++++++++++-
 pbs-config/src/datastore.rs            | 11 ++++++--
 pbs-datastore/src/datastore.rs         | 36 +++++++++++++++++---------
 3 files changed, 50 insertions(+), 15 deletions(-)

diff --git a/pbs-config/src/config_version_cache.rs b/pbs-config/src/config_version_cache.rs
index c0d181d6..39a433ed 100644
--- a/pbs-config/src/config_version_cache.rs
+++ b/pbs-config/src/config_version_cache.rs
@@ -24,10 +24,12 @@ struct ConfigVersionCacheData {
     user_cache_generation: AtomicUsize,
     // Traffic control (traffic-control.cfg) generation/version.
     traffic_control_generation: AtomicUsize,
+    // datastore (datastore.cfg) generation/version
+    datastore_generation: AtomicUsize,
 
     // Add further atomics here (and reduce padding size)
 
-    padding: [u8; 4096 - 3*8],
+    padding: [u8; 4096 - 4*8],
 }
 
 
@@ -118,4 +120,18 @@ impl ConfigVersionCache {
             .fetch_add(1, Ordering::AcqRel);
     }
 
+    /// Returns the datastore generation number.
+    pub fn datastore_generation(&self) -> usize {
+        self.shmem
+            .data()
+            .datastore_generation
+            .load(Ordering::Acquire)
+    }
+
+    /// Increase the datastore generation number.
+    pub fn increase_datastore_generation(&self) -> usize {
+        self.shmem
+            .data()
+            .datastore_generation.fetch_add(1, Ordering::Acquire)
+    }
 }
diff --git a/pbs-config/src/datastore.rs b/pbs-config/src/datastore.rs
index 12071a9f..6d17da63 100644
--- a/pbs-config/src/datastore.rs
+++ b/pbs-config/src/datastore.rs
@@ -7,7 +7,7 @@ use proxmox_section_config::{SectionConfig, SectionConfigData, SectionConfigPlug
 
 use pbs_api_types::{DataStoreConfig, DATASTORE_SCHEMA};
 
-use crate::{open_backup_lockfile, replace_backup_config, BackupLockGuard};
+use crate::{open_backup_lockfile, replace_backup_config, BackupLockGuard, ConfigVersionCache};
 
 lazy_static! {
     pub static ref CONFIG: SectionConfig = init();
@@ -46,7 +46,14 @@ pub fn config() -> Result<(SectionConfigData, [u8;32]), Error> {
 
 pub fn save_config(config: &SectionConfigData) -> Result<(), Error> {
     let raw = CONFIG.write(DATASTORE_CFG_FILENAME, config)?;
-    replace_backup_config(DATASTORE_CFG_FILENAME, raw.as_bytes())
+    replace_backup_config(DATASTORE_CFG_FILENAME, raw.as_bytes())?;
+
+    // increase datastore version
+    // We use this in pbs-datastore
+    let version_cache = ConfigVersionCache::new()?;
+    version_cache.increase_datastore_generation();
+
+    Ok(())
 }
 
 // shell completion helper
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 8397da00..d416c8d8 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -21,7 +21,7 @@ use pbs_api_types::{
     UPID, DataStoreConfig, Authid, GarbageCollectionStatus, HumanByte,
     ChunkOrder, DatastoreTuning,
 };
-use pbs_config::{open_backup_lockfile, BackupLockGuard};
+use pbs_config::{open_backup_lockfile, BackupLockGuard, ConfigVersionCache};
 
 use crate::DataBlob;
 use crate::backup_info::{BackupGroup, BackupDir};
@@ -63,26 +63,30 @@ pub struct DataStore {
     last_gc_status: Mutex<GarbageCollectionStatus>,
     verify_new: bool,
     chunk_order: ChunkOrder,
+    last_generation: usize,
+    last_update: i64,
 }
 
 impl DataStore {
     pub fn lookup_datastore(name: &str) -> Result<Arc<DataStore>, Error> {
-        let (config, _digest) = pbs_config::datastore::config()?;
-        let config: DataStoreConfig = config.lookup("datastore", name)?;
-        let path = PathBuf::from(&config.path);
+        let version_cache = ConfigVersionCache::new()?;
+        let generation = version_cache.datastore_generation();
+        let now = proxmox_time::epoch_i64();
 
         let mut map = DATASTORE_MAP.lock().unwrap();
+        let entry = map.get(name);
 
-        if let Some(datastore) = map.get(name) {
-            // Compare Config - if changed, create new Datastore object!
-            if datastore.chunk_store.base() == path &&
-                datastore.verify_new == config.verify_new.unwrap_or(false)
-            {
-                return Ok(datastore.clone());
+        if let Some(datastore) = &entry {
+            if datastore.last_generation == generation && now < (datastore.last_update + 60) {
+                return Ok(Arc::clone(datastore));
             }
         }
 
-        let datastore = DataStore::open_with_path(name, &path, config)?;
+        let (config, _digest) = pbs_config::datastore::config()?;
+        let config: DataStoreConfig = config.lookup("datastore", name)?;
+        let path = PathBuf::from(&config.path);
+
+        let datastore = DataStore::open_with_path(name, &path, config, generation, now)?;
 
         let datastore = Arc::new(datastore);
         map.insert(name.to_string(), datastore.clone());
@@ -102,7 +106,13 @@ impl DataStore {
         Ok(())
     }
 
-    fn open_with_path(store_name: &str, path: &Path, config: DataStoreConfig) -> Result<Self, Error> {
+    fn open_with_path(
+        store_name: &str,
+        path: &Path,
+        config: DataStoreConfig,
+        last_generation: usize,
+        last_update: i64,
+    ) -> Result<Self, Error> {
         let chunk_store = ChunkStore::open(store_name, path)?;
 
         let mut gc_status_path = chunk_store.base_path();
@@ -131,6 +141,8 @@ impl DataStore {
             last_gc_status: Mutex::new(gc_status),
             verify_new: config.verify_new.unwrap_or(false),
             chunk_order,
+            last_generation,
+            last_update,
         })
     }
 
-- 
2.30.2

[pbs-devel] applied: [PATCH proxmox-backup] pbs-datastore: use ConfigVersionCache for datastore

[Dietmar Maurer]

applied

Re: [pbs-devel] [PATCH proxmox-backup] pbs-datastore: use ConfigVersionCache for datastore

[Thomas Lamprecht]

On 28.02.22 07:51, Dominik Csapak wrote:
> On 2/25/22 16:25, Dietmar Maurer wrote:
>>> but on the other hand, we now basically create a new datastore object
>>> every minute
>>
>> And you think this is a problem (why)?
> 
> it seems unnecessary, and if it was not a problem, why didn't we do it before?
> we already read/parsed the config anyway....
> 
> i assumed that there was a good reason not to construct a new object
> every time, but i did not find it
> 

Yeah I also do not get this, especially as there's zero info about "why"
and the benefits in the commit message - that's more important than
describing the diff that is attached to the commit anyway (not that this
cannot help but focus should be on the former).

Any benchmarking, timing assumptions or other things done? As more and
more complex code should have /some/ arguing in favor of it sent along..

Re: [pbs-devel] [PATCH proxmox-backup] pbs-datastore: use ConfigVersionCache for datastore

[Dominik Csapak]

On 2/25/22 16:25, Dietmar Maurer wrote:
>> but on the other hand, we now basically create a new datastore object
>> every minute
> 
> And you think this is a problem (why)?

it seems unnecessary, and if it was not a problem, why didn't we do it before?
we already read/parsed the config anyway....

i assumed that there was a good reason not to construct a new object
every time, but i did not find it

Re: [pbs-devel] [PATCH proxmox-backup] pbs-datastore: use ConfigVersionCache for datastore

[Dietmar Maurer]

> but on the other hand, we now basically create a new datastore object
> every minute

And you think this is a problem (why)?