[pbs-devel] [PATCH proxmox-backup 0/3] cleanup directory lock code

[pbs-devel] [PATCH proxmox-backup 0/3] cleanup directory lock code

[Dietmar Maurer]

This applies on top of the first three patches from stefan s series:

[pbs-devel] [PATCH proxmox-backup 1/7] finish_backup: mark backup as finished only after checks have passed (already applied)
[pbs-devel] [PATCH proxmox-backup 2/7] backup: only allow finished backups as base snapshot (already applied)
[pbs-devel] [PATCH proxmox-backup 3/7] datastore: prevent in-use deletion with locks instead of heuristic (not applied)


Dietmar Maurer (3):
  src/tools/fs.rs: new helper lock_dir_noblock
  src/backup/backup_info.rs: remove BackupGroup lock()
  src/backup/backup_info.rs: remove BackupInfo lock()

 src/backup/backup_info.rs | 86 ++-------------------------------------
 src/backup/datastore.rs   | 19 ++++-----
 src/tools/fs.rs           | 39 ++++++++++++++++--
 3 files changed, 47 insertions(+), 97 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 1/3] src/tools/fs.rs: new helper lock_dir_noblock

[Dietmar Maurer]

---
 src/tools/fs.rs | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/src/tools/fs.rs b/src/tools/fs.rs
index ff625d3..5aaaecd 100644
--- a/src/tools/fs.rs
+++ b/src/tools/fs.rs
@@ -7,10 +7,18 @@ use std::os::unix::io::{AsRawFd, RawFd};
 use anyhow::{format_err, Error};
 use nix::dir;
 use nix::dir::Dir;
+use nix::fcntl::OFlag;
+use nix::sys::stat::Mode;
+
 use regex::Regex;
 
+use proxmox::sys::error::SysError;
+
+
 use crate::tools::borrow::Tied;
 
+pub type DirLockGuard = Dir;
+
 /// This wraps nix::dir::Entry with the parent directory's file descriptor.
 pub struct ReadDirEntry {
     entry: dir::Entry,
@@ -94,9 +102,6 @@ impl Iterator for ReadDir {
 /// Create an iterator over sub directory entries.
 /// This uses `openat` on `dirfd`, so `path` can be relative to that or an absolute path.
 pub fn read_subdir<P: ?Sized + nix::NixPath>(dirfd: RawFd, path: &P) -> nix::Result<ReadDir> {
-    use nix::fcntl::OFlag;
-    use nix::sys::stat::Mode;
-
     let dir = Dir::openat(dirfd, path, OFlag::O_RDONLY, Mode::empty())?;
     let fd = dir.as_raw_fd();
     let iter = Tied::new(dir, |dir| {
@@ -259,3 +264,31 @@ impl Default for FSXAttr {
         }
     }
 }
+
+
+pub fn lock_dir_noblock(
+    path: &std::path::Path,
+    what: &str,
+    would_block_msg: &str,
+) -> Result<DirLockGuard, Error> {
+    let mut handle = Dir::open(path, OFlag::O_RDONLY, Mode::empty())
+        .map_err(|err| {
+            format_err!("unable to open {} directory {:?} for locking - {}", what, path, err)
+        })?;
+
+    // acquire in non-blocking mode, no point in waiting here since other
+    // backups could still take a very long time
+    proxmox::tools::fs::lock_file(&mut handle, true, Some(std::time::Duration::from_nanos(0)))
+        .map_err(|err| {
+            format_err!(
+                "unable to acquire lock on {} directory {:?} - {}", what, path,
+                if err.would_block() {
+                    String::from(would_block_msg)
+                } else {
+                    err.to_string()
+                }
+            )
+        })?;
+
+    Ok(handle)
+}
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/3] src/backup/backup_info.rs: remove BackupGroup lock()

[Dietmar Maurer]

And use new lock_dir_noblock() instead.
---
 src/backup/backup_info.rs | 35 +----------------------------------
 src/backup/datastore.rs   | 17 ++++++-----------
 2 files changed, 7 insertions(+), 45 deletions(-)

diff --git a/src/backup/backup_info.rs b/src/backup/backup_info.rs
index c35928c..df2349b 100644
--- a/src/backup/backup_info.rs
+++ b/src/backup/backup_info.rs
@@ -14,6 +14,7 @@ use lazy_static::lazy_static;
 use proxmox::sys::error::SysError;
 
 use super::manifest::MANIFEST_BLOB_NAME;
+use crate::tools::fs::lock_dir_noblock;
 
 macro_rules! BACKUP_ID_RE { () => (r"[A-Za-z0-9][A-Za-z0-9_-]+") }
 macro_rules! BACKUP_TYPE_RE { () => (r"(?:host|vm|ct)") }
@@ -141,40 +142,6 @@ impl BackupGroup {
         Ok(last)
     }
 
-    pub fn lock(&self, base_path: &Path) -> Result<BackupLockGuard, Error> {
-        use nix::fcntl::OFlag;
-        use nix::sys::stat::Mode;
-
-        let mut path = base_path.to_owned();
-        path.push(self.group_path());
-
-        let mut handle = Dir::open(&path, OFlag::O_RDONLY, Mode::empty())
-            .map_err(|err| {
-                format_err!(
-                    "unable to open backup group directory {:?} for locking - {}",
-                    self.group_path(),
-                    err,
-                )
-            })?;
-
-        // acquire in non-blocking mode, no point in waiting here since other
-        // backups could still take a very long time
-        proxmox::tools::fs::lock_file(&mut handle, true, Some(Duration::from_nanos(0)))
-            .map_err(|err| {
-                format_err!(
-                    "unable to acquire lock on backup group {:?} - {}",
-                    self.group_path(),
-                    if err.would_block() {
-                        String::from("another backup is already running")
-                    } else {
-                        err.to_string()
-                    }
-                )
-            })?;
-
-        Ok(handle)
-    }
-
     pub fn list_groups(base_path: &Path) -> Result<Vec<BackupGroup>, Error> {
         let mut list = Vec::new();
 
diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index 395515f..dbd42d6 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -11,7 +11,7 @@ use serde_json::Value;
 
 use proxmox::tools::fs::{replace_file, CreateOptions};
 
-use super::backup_info::{BackupGroup, BackupLockGuard, BackupDir, BackupInfo};
+use super::backup_info::{BackupGroup, BackupDir, BackupInfo};
 use super::chunk_store::ChunkStore;
 use super::dynamic_index::{DynamicIndexReader, DynamicIndexWriter};
 use super::fixed_index::{FixedIndexReader, FixedIndexWriter};
@@ -21,6 +21,7 @@ use super::{DataBlob, ArchiveType, archive_type};
 use crate::config::datastore;
 use crate::server::WorkerTask;
 use crate::tools;
+use crate::tools::fs::{lock_dir_noblock, DirLockGuard};
 use crate::api2::types::GarbageCollectionStatus;
 
 lazy_static! {
@@ -199,13 +200,7 @@ impl DataStore {
 
         let full_path = self.group_path(backup_group);
 
-        let _guard = backup_group.lock(&self.base_path()).map_err(|err| {
-            format_err!(
-                "cannot acquire lock on backup group {}: {}",
-                backup_group,
-                err
-            )
-        })?;
+        let _guard = tools::fs::lock_dir_noblock(&full_path, "backup group", "possible running backup")?;
 
         log::info!("removing backup group {:?}", full_path);
         std::fs::remove_dir_all(&full_path)
@@ -300,7 +295,7 @@ impl DataStore {
     /// current owner (instead of setting the owner).
     ///
     /// This also aquires an exclusive lock on the directory and returns the lock guard.
-    pub fn create_locked_backup_group(&self, backup_group: &BackupGroup, userid: &str) -> Result<(String, BackupLockGuard), Error> {
+    pub fn create_locked_backup_group(&self, backup_group: &BackupGroup, userid: &str) -> Result<(String, DirLockGuard), Error> {
 
         // create intermediate path first:
         let base_path = self.base_path();
@@ -314,13 +309,13 @@ impl DataStore {
         // create the last component now
         match std::fs::create_dir(&full_path) {
             Ok(_) => {
-                let guard = backup_group.lock(&base_path)?;
+                let guard = lock_dir_noblock(&full_path, "backup group", "another backup is already running")?;
                 self.set_owner(backup_group, userid, false)?;
                 let owner = self.get_owner(backup_group)?; // just to be sure
                 Ok((owner, guard))
             }
             Err(ref err) if err.kind() == io::ErrorKind::AlreadyExists => {
-                let guard = backup_group.lock(&base_path)?;
+                let guard = lock_dir_noblock(&full_path, "backup group", "another backup is already running")?;
                 let owner = self.get_owner(backup_group)?; // just to be sure
                 Ok((owner, guard))
             }
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 3/3] src/backup/backup_info.rs: remove BackupInfo lock()

[Dietmar Maurer]

And use new lock_dir_noblock() instead.
---
 src/backup/backup_info.rs | 53 +++------------------------------------
 src/backup/datastore.rs   |  4 +--
 2 files changed, 6 insertions(+), 51 deletions(-)

diff --git a/src/backup/backup_info.rs b/src/backup/backup_info.rs
index df2349b..d6e9551 100644
--- a/src/backup/backup_info.rs
+++ b/src/backup/backup_info.rs
@@ -3,18 +3,13 @@ use crate::tools;
 use anyhow::{bail, format_err, Error};
 use regex::Regex;
 use std::os::unix::io::RawFd;
-use nix::dir::Dir;
 
-use std::time::Duration;
 use chrono::{DateTime, TimeZone, SecondsFormat, Utc};
 
 use std::path::{PathBuf, Path};
 use lazy_static::lazy_static;
 
-use proxmox::sys::error::SysError;
-
 use super::manifest::MANIFEST_BLOB_NAME;
-use crate::tools::fs::lock_dir_noblock;
 
 macro_rules! BACKUP_ID_RE { () => (r"[A-Za-z0-9][A-Za-z0-9_-]+") }
 macro_rules! BACKUP_TYPE_RE { () => (r"(?:host|vm|ct)") }
@@ -41,9 +36,6 @@ lazy_static!{
 
 }
 
-/// Opaque type releasing the corresponding flock when dropped
-pub type BackupLockGuard = Dir;
-
 /// BackupGroup is a directory containing a list of BackupDir
 #[derive(Debug, Eq, PartialEq, Hash, Clone)]
 pub struct BackupGroup {
@@ -95,7 +87,6 @@ impl BackupGroup {
             list.push(BackupInfo {
                 backup_dir,
                 files,
-                base_path: base_path.to_owned()
             });
 
             Ok(())
@@ -270,8 +261,8 @@ pub struct BackupInfo {
     pub backup_dir: BackupDir,
     /// List of data files
     pub files: Vec<String>,
-    /// Full path to dir containing backup_dir
-    pub base_path: PathBuf,
+    // Full path to dir containing backup_dir
+    //pub base_path: PathBuf,
 }
 
 impl BackupInfo {
@@ -282,7 +273,7 @@ impl BackupInfo {
 
         let files = list_backup_files(libc::AT_FDCWD, &path)?;
 
-        Ok(BackupInfo { backup_dir, files, base_path: base_path.to_owned() })
+        Ok(BackupInfo { backup_dir, files })
     }
 
     /// Finds the latest backup inside a backup group
@@ -330,8 +321,7 @@ impl BackupInfo {
                     list.push(BackupInfo {
                         backup_dir,
                         files,
-                        base_path: base_path.to_owned()
-                    });
+                     });
 
                     Ok(())
                 })
@@ -344,41 +334,6 @@ impl BackupInfo {
         // backup is considered unfinished if there is no manifest
         self.files.iter().any(|name| name == super::MANIFEST_BLOB_NAME)
     }
-
-    pub fn lock(&self) -> Result<BackupLockGuard, Error> {
-        use nix::fcntl::OFlag;
-        use nix::sys::stat::Mode;
-
-        let mut path = self.base_path.clone();
-        let dir = self.backup_dir.relative_path();
-        path.push(&dir);
-
-        let mut handle = Dir::open(&path, OFlag::O_RDONLY, Mode::empty())
-            .map_err(|err| {
-                format_err!(
-                    "unable to open snapshot directory {:?} for locking - {}",
-                    &dir,
-                    err,
-                )
-            })?;
-
-        // acquire in non-blocking mode, no point in waiting here since other
-        // backups could still take a very long time
-        proxmox::tools::fs::lock_file(&mut handle, true, Some(Duration::from_nanos(0)))
-            .map_err(|err| {
-                format_err!(
-                    "unable to acquire lock on snapshot {:?} - {}",
-                    &dir,
-                    if err.would_block() {
-                        String::from("snapshot is running or being used as base")
-                    } else {
-                        err.to_string()
-                    }
-                )
-            })?;
-
-        Ok(handle)
-    }
 }
 
 fn list_backup_files<P: ?Sized + nix::NixPath>(dirfd: RawFd, path: &P) -> Result<Vec<String>, Error> {
diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index dbd42d6..3bed167 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -11,7 +11,7 @@ use serde_json::Value;
 
 use proxmox::tools::fs::{replace_file, CreateOptions};
 
-use super::backup_info::{BackupGroup, BackupDir, BackupInfo};
+use super::backup_info::{BackupGroup, BackupDir};
 use super::chunk_store::ChunkStore;
 use super::dynamic_index::{DynamicIndexReader, DynamicIndexWriter};
 use super::fixed_index::{FixedIndexReader, FixedIndexWriter};
@@ -222,7 +222,7 @@ impl DataStore {
 
         let _guard;
         if !force {
-            _guard = BackupInfo::new(&self.base_path(), backup_dir.clone())?.lock()?;
+            _guard = tools::fs::lock_dir_noblock(&full_path, "snapshot", "backup is running or snapshot is used as base")?;
         }
 
         log::info!("removing backup snapshot {:?}", full_path);
-- 
2.20.1