From 5c9c23b6b2bb9ed3cd85b618e2412874af5344a1 Mon Sep 17 00:00:00 2001 From: Thomas Lamprecht Date: Sun, 24 Apr 2022 18:37:15 +0200 Subject: [PATCH] datastore: move manifest locking into BackupDir impl the manifest is owned by the backup dir (snapshot) so it should also handle locking, makes no sense to have the implementation somewhere higher up. Signed-off-by: Thomas Lamprecht --- pbs-datastore/src/backup_info.rs | 28 +++++++++++++++++++-- pbs-datastore/src/datastore.rs | 42 +++----------------------------- 2 files changed, 30 insertions(+), 40 deletions(-) diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs index 555e430c..5b217a98 100644 --- a/pbs-datastore/src/backup_info.rs +++ b/pbs-datastore/src/backup_info.rs @@ -3,11 +3,12 @@ use std::os::unix::io::RawFd; use std::path::PathBuf; use std::sync::Arc; -use anyhow::{bail, Error}; +use anyhow::{bail, format_err, Error}; use pbs_api_types::{BackupType, GroupFilter, BACKUP_DATE_REGEX, BACKUP_FILE_REGEX}; +use pbs_config::{open_backup_lockfile, BackupLockGuard}; -use crate::manifest::MANIFEST_BLOB_NAME; +use crate::manifest::{MANIFEST_BLOB_NAME, MANIFEST_LOCK_NAME}; use crate::DataStore; /// BackupGroup is a directory containing a list of BackupDir @@ -313,6 +314,29 @@ impl BackupDir { // fixme: can this fail? (avoid unwrap) proxmox_time::epoch_to_rfc3339_utc(backup_time) } + + /// Returns the filename to lock a manifest + /// + /// Also creates the basedir. The lockfile is located in + /// '/run/proxmox-backup/locks/{datastore}/{type}/{id}/{timestamp}.index.json.lck' + pub(crate) fn manifest_lock_path(&self) -> Result { + let mut path = format!("/run/proxmox-backup/locks/{}/{self}", self.store.name()); + std::fs::create_dir_all(&path)?; + use std::fmt::Write; + let ts = self.backup_time_string(); + write!(path, "/{ts}{}", &MANIFEST_LOCK_NAME)?; + + Ok(path) + } + + /// Locks the manifest of a snapshot, for example, to update or delete it. + pub(crate) fn lock_manifest(&self) -> Result { + let path = self.manifest_lock_path()?; + + // actions locking the manifest should be relatively short, only wait a few seconds + open_backup_lockfile(&path, Some(std::time::Duration::from_secs(5)), true) + .map_err(|err| format_err!("unable to acquire manifest lock {:?} - {}", &path, err)) + } } impl AsRef for BackupDir { diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs index 05442805..f302ce6e 100644 --- a/pbs-datastore/src/datastore.rs +++ b/pbs-datastore/src/datastore.rs @@ -4,7 +4,6 @@ use std::io::{self, Write}; use std::path::{Path, PathBuf}; use std::str::FromStr; use std::sync::{Arc, Mutex}; -use std::time::Duration; use anyhow::{bail, format_err, Error}; use lazy_static::lazy_static; @@ -21,7 +20,7 @@ use pbs_api_types::{ Authid, BackupType, ChunkOrder, DataStoreConfig, DatastoreTuning, GarbageCollectionStatus, HumanByte, Operation, BACKUP_DATE_REGEX, BACKUP_ID_REGEX, UPID, }; -use pbs_config::{open_backup_lockfile, BackupLockGuard, ConfigVersionCache}; +use pbs_config::ConfigVersionCache; use crate::backup_info::{BackupDir, BackupGroup}; use crate::chunk_store::ChunkStore; @@ -30,7 +29,6 @@ use crate::fixed_index::{FixedIndexReader, FixedIndexWriter}; use crate::index::IndexFile; use crate::manifest::{ archive_type, ArchiveType, BackupManifest, CLIENT_LOG_BLOB_NAME, MANIFEST_BLOB_NAME, - MANIFEST_LOCK_NAME, }; use crate::task_tracking::update_active_operations; use crate::DataBlob; @@ -469,7 +467,7 @@ impl DataStore { let (_guard, _manifest_guard); if !force { _guard = lock_dir_noblock(&full_path, "snapshot", "possibly running or in use")?; - _manifest_guard = self.lock_manifest(&backup_dir)?; + _manifest_guard = backup_dir.lock_manifest()?; } if backup_dir.is_protected() { @@ -482,7 +480,7 @@ impl DataStore { })?; // the manifest does not exists anymore, we do not need to keep the lock - if let Ok(path) = self.manifest_lock_path(&backup_dir) { + if let Ok(path) = backup_dir.manifest_lock_path() { // ignore errors let _ = std::fs::remove_file(path); } @@ -1002,38 +1000,6 @@ impl DataStore { }) } - /// Returns the filename to lock a manifest - /// - /// Also creates the basedir. The lockfile is located in - /// '/run/proxmox-backup/locks/{datastore}/{type}/{id}/{timestamp}.index.json.lck' - fn manifest_lock_path(&self, backup_dir: &BackupDir) -> Result { - let mut path = format!( - "/run/proxmox-backup/locks/{}/{}/{}", - self.name(), - backup_dir.backup_type(), - backup_dir.backup_id(), - ); - std::fs::create_dir_all(&path)?; - use std::fmt::Write; - write!( - path, - "/{}{}", - backup_dir.backup_time_string(), - &MANIFEST_LOCK_NAME - )?; - - Ok(path) - } - - fn lock_manifest(&self, backup_dir: &BackupDir) -> Result { - let path = self.manifest_lock_path(backup_dir)?; - - // update_manifest should never take a long time, so if someone else has - // the lock we can simply block a bit and should get it soon - open_backup_lockfile(&path, Some(Duration::from_secs(5)), true) - .map_err(|err| format_err!("unable to acquire manifest lock {:?} - {}", &path, err)) - } - /// Load the manifest without a lock. Must not be written back. pub fn load_manifest(&self, backup_dir: &BackupDir) -> Result<(BackupManifest, u64), Error> { let blob = self.load_blob(backup_dir, MANIFEST_BLOB_NAME)?; @@ -1049,7 +1015,7 @@ impl DataStore { backup_dir: &BackupDir, update_fn: impl FnOnce(&mut BackupManifest), ) -> Result<(), Error> { - let _guard = self.lock_manifest(backup_dir)?; + let _guard = backup_dir.lock_manifest()?; let (mut manifest, _) = self.load_manifest(backup_dir)?; update_fn(&mut manifest);