From 9ccf933be5a0a8ae9f15221346ac67b8d99954da Mon Sep 17 00:00:00 2001 From: Thomas Lamprecht Date: Wed, 11 May 2022 18:55:57 +0200 Subject: [PATCH] datastore: move update_manifest into BackupDir impl Signed-off-by: Thomas Lamprecht --- pbs-datastore/src/backup_info.rs | 19 ++++++++++++++-- pbs-datastore/src/datastore.rs | 33 +--------------------------- pbs-datastore/src/snapshot_reader.rs | 2 +- src/api2/admin/datastore.rs | 16 ++++++-------- src/api2/backup/environment.rs | 4 ++-- src/api2/backup/mod.rs | 2 +- src/api2/config/sync.rs | 5 +---- src/backup/verify.rs | 7 +++--- 8 files changed, 33 insertions(+), 55 deletions(-) diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs index c9aa72b8..5440cdb3 100644 --- a/pbs-datastore/src/backup_info.rs +++ b/pbs-datastore/src/backup_info.rs @@ -6,7 +6,7 @@ use std::sync::Arc; use anyhow::{bail, format_err, Error}; -use proxmox_sys::fs::lock_dir_noblock; +use proxmox_sys::fs::{lock_dir_noblock, replace_file, CreateOptions}; use pbs_api_types::{ Authid, BackupNamespace, BackupType, GroupFilter, BACKUP_DATE_REGEX, BACKUP_FILE_REGEX, @@ -493,7 +493,22 @@ impl BackupDir { &self, update_fn: impl FnOnce(&mut BackupManifest), ) -> Result<(), Error> { - self.store.update_manifest(self, update_fn) + let _guard = self.lock_manifest()?; + let (mut manifest, _) = self.load_manifest()?; + + update_fn(&mut manifest); + + let manifest = serde_json::to_value(manifest)?; + let manifest = serde_json::to_string_pretty(&manifest)?; + let blob = DataBlob::encode(manifest.as_bytes(), None, true)?; + let raw_data = blob.raw_data(); + + let mut path = self.full_path(); + path.push(MANIFEST_BLOB_NAME); + + // atomic replace invalidates flock - no other writes past this point! + replace_file(&path, raw_data, CreateOptions::new(), false)?; + Ok(()) } /// Cleans up the backup directory by removing any file not mentioned in the manifest. diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs index 9a63a9e0..e2057a00 100644 --- a/pbs-datastore/src/datastore.rs +++ b/pbs-datastore/src/datastore.rs @@ -28,7 +28,7 @@ use crate::chunk_store::ChunkStore; use crate::dynamic_index::{DynamicIndexReader, DynamicIndexWriter}; use crate::fixed_index::{FixedIndexReader, FixedIndexWriter}; use crate::index::IndexFile; -use crate::manifest::{archive_type, ArchiveType, BackupManifest, MANIFEST_BLOB_NAME}; +use crate::manifest::{archive_type, ArchiveType}; use crate::task_tracking::update_active_operations; use crate::DataBlob; @@ -1098,37 +1098,6 @@ impl DataStore { }) } - /// Load the manifest without a lock. Must not be written back. - pub fn load_manifest(&self, backup_dir: &BackupDir) -> Result<(BackupManifest, u64), Error> { - backup_dir.load_manifest() - } - - /// Update the manifest of the specified snapshot. Never write a manifest directly, - /// only use this method - anything else may break locking guarantees. - pub fn update_manifest( - &self, - backup_dir: &BackupDir, - update_fn: impl FnOnce(&mut BackupManifest), - ) -> Result<(), Error> { - let _guard = backup_dir.lock_manifest()?; - let (mut manifest, _) = self.load_manifest(backup_dir)?; - - update_fn(&mut manifest); - - let manifest = serde_json::to_value(manifest)?; - let manifest = serde_json::to_string_pretty(&manifest)?; - let blob = DataBlob::encode(manifest.as_bytes(), None, true)?; - let raw_data = blob.raw_data(); - - let mut path = backup_dir.full_path(); - path.push(MANIFEST_BLOB_NAME); - - // atomic replace invalidates flock - no other writes past this point! - replace_file(&path, raw_data, CreateOptions::new(), false)?; - - Ok(()) - } - /// Updates the protection status of the specified snapshot. pub fn update_protection(&self, backup_dir: &BackupDir, protection: bool) -> Result<(), Error> { let full_path = backup_dir.full_path(); diff --git a/pbs-datastore/src/snapshot_reader.rs b/pbs-datastore/src/snapshot_reader.rs index 44b76940..54a2b963 100644 --- a/pbs-datastore/src/snapshot_reader.rs +++ b/pbs-datastore/src/snapshot_reader.rs @@ -46,7 +46,7 @@ impl SnapshotReader { let datastore_name = datastore.name().to_string(); - let manifest = match datastore.load_manifest(&snapshot) { + let manifest = match snapshot.load_manifest() { Ok((manifest, _)) => manifest, Err(err) => { bail!( diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs index 5dc7c596..f9d50a5b 100644 --- a/src/api2/admin/datastore.rs +++ b/src/api2/admin/datastore.rs @@ -135,10 +135,9 @@ fn check_privs_and_load_store( } fn read_backup_index( - store: &DataStore, backup_dir: &BackupDir, ) -> Result<(BackupManifest, Vec), Error> { - let (manifest, index_size) = store.load_manifest(backup_dir)?; + let (manifest, index_size) = backup_dir.load_manifest()?; let mut result = Vec::new(); for item in manifest.files() { @@ -162,10 +161,9 @@ fn read_backup_index( } fn get_all_snapshot_files( - store: &DataStore, info: &BackupInfo, ) -> Result<(BackupManifest, Vec), Error> { - let (manifest, mut files) = read_backup_index(store, &info.backup_dir)?; + let (manifest, mut files) = read_backup_index(&info.backup_dir)?; let file_set = files.iter().fold(HashSet::new(), |mut acc, item| { acc.insert(item.filename.clone()); @@ -373,7 +371,7 @@ pub fn list_snapshot_files( let info = BackupInfo::new(snapshot)?; - let (_manifest, files) = get_all_snapshot_files(&datastore, &info)?; + let (_manifest, files) = get_all_snapshot_files(&info)?; Ok(files) } @@ -503,7 +501,7 @@ pub fn list_snapshots( }; let protected = info.backup_dir.is_protected(); - match get_all_snapshot_files(&datastore, &info) { + match get_all_snapshot_files(&info) { Ok((manifest, files)) => { // extract the first line from notes let comment: Option = manifest.unprotected["notes"] @@ -1369,7 +1367,7 @@ pub fn download_file_decoded( let file_name = required_string_param(¶m, "file-name")?.to_owned(); - let (manifest, files) = read_backup_index(&datastore, &backup_dir)?; + let (manifest, files) = read_backup_index(&backup_dir)?; for file in files { if file.filename == file_name && file.crypt_mode == Some(CryptMode::Encrypt) { bail!("cannot decode '{}' - is encrypted", file_name); @@ -1572,7 +1570,7 @@ pub fn catalog( let file_name = CATALOG_NAME; - let (manifest, files) = read_backup_index(&datastore, &backup_dir)?; + let (manifest, files) = read_backup_index(&backup_dir)?; for file in files { if file.filename == file_name && file.crypt_mode == Some(CryptMode::Encrypt) { bail!("cannot decode '{}' - is encrypted", file_name); @@ -1662,7 +1660,7 @@ pub fn pxar_file_download( let mut split = components.splitn(2, |c| *c == b'/'); let pxar_name = std::str::from_utf8(split.next().unwrap())?; let file_path = split.next().unwrap_or(b"/"); - let (manifest, files) = read_backup_index(&datastore, &backup_dir)?; + let (manifest, files) = read_backup_index(&backup_dir)?; for file in files { if file.filename == pxar_name && file.crypt_mode == Some(CryptMode::Encrypt) { bail!("cannot decode '{}' - is encrypted", pxar_name); diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs index 8adfce1b..81879d4a 100644 --- a/src/api2/backup/environment.rs +++ b/src/api2/backup/environment.rs @@ -607,8 +607,8 @@ impl BackupEnvironment { // check for valid manifest and store stats let stats = serde_json::to_value(state.backup_stat)?; - self.datastore - .update_manifest(&self.backup_dir, |manifest| { + self.backup_dir + .update_manifest(|manifest| { manifest.unprotected["chunk_upload_stats"] = stats; }) .map_err(|err| format_err!("unable to update manifest blob - {}", err))?; diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs index 70e28e79..db856ecd 100644 --- a/src/api2/backup/mod.rs +++ b/src/api2/backup/mod.rs @@ -155,7 +155,7 @@ fn upgrade_to_backup_protocol( let last_backup = { let info = backup_group.last_backup(true).unwrap_or(None); if let Some(info) = info { - let (manifest, _) = datastore.load_manifest(&info.backup_dir)?; + let (manifest, _) = info.backup_dir.load_manifest()?; let verify = manifest.unprotected["verify_state"].clone(); match serde_json::from_value::(verify) { Ok(verify) => match verify.state { diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs index b02ae691..966ce2a4 100644 --- a/src/api2/config/sync.rs +++ b/src/api2/config/sync.rs @@ -351,10 +351,7 @@ pub fn update_sync_job( data.remote_store = remote_store; } if let Some(remote_ns) = update.remote_ns { - check_max_depth( - &remote_ns, - update.max_depth.unwrap_or(data.max_depth), - )?; + check_max_depth(&remote_ns, update.max_depth.unwrap_or(data.max_depth))?; data.remote_ns = Some(remote_ns); } if let Some(owner) = update.owner { diff --git a/src/backup/verify.rs b/src/backup/verify.rs index 41ffdc66..a5ec9a4c 100644 --- a/src/backup/verify.rs +++ b/src/backup/verify.rs @@ -357,7 +357,7 @@ pub fn verify_backup_dir_with_lock( filter: Option<&dyn Fn(&BackupManifest) -> bool>, _snap_lock: Dir, ) -> Result { - let manifest = match verify_worker.datastore.load_manifest(backup_dir) { + let manifest = match backup_dir.load_manifest() { Ok((manifest, _)) => manifest, Err(err) => { task_log!( @@ -425,9 +425,8 @@ pub fn verify_backup_dir_with_lock( upid, }; let verify_state = serde_json::to_value(verify_state)?; - verify_worker - .datastore - .update_manifest(backup_dir, |manifest| { + backup_dir + .update_manifest(|manifest| { manifest.unprotected["verify_state"] = verify_state; }) .map_err(|err| format_err!("unable to update manifest blob - {}", err))?; -- 2.39.5