[pbs-devel] [PATCH backup] src/api2/backup.rs: aquire backup lock earlier in create_locked_backup_group()

[pbs-devel] [PATCH backup] src/api2/backup.rs: aquire backup lock earlier in create_locked_backup_group()

[Dietmar Maurer]

---
 src/api2/backup.rs      |  8 ++++----
 src/backup/datastore.rs | 14 +++++++++-----
 src/client/pull.rs      |  2 +-
 3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/src/api2/backup.rs b/src/api2/backup.rs
index b9dff1f..ad13faa 100644
--- a/src/api2/backup.rs
+++ b/src/api2/backup.rs
@@ -88,7 +88,10 @@ async move {
     let env_type = rpcenv.env_type();
 
     let backup_group = BackupGroup::new(backup_type, backup_id);
-    let owner = datastore.create_backup_group(&backup_group, &username)?;
+
+    // lock backup group to only allow one backup per group at a time
+    let (owner, _group_guard) = datastore.create_locked_backup_group(&backup_group, &username)?;
+
     // permission check
     if owner != username { // only the owner is allowed to create additional snapshots
         bail!("backup owner check failed ({} != {})", username, owner);
@@ -103,9 +106,6 @@ async move {
         }
     }
 
-    // lock backup group to only allow one backup per group at a time
-    let _group_guard = backup_group.lock(&datastore.base_path())?;
-
     let (path, is_new) = datastore.create_backup_dir(&backup_dir)?;
     if !is_new { bail!("backup directory already exists."); }
 
diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index 9c2f285..af964f6 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -8,7 +8,7 @@ use anyhow::{bail, format_err, Error};
 use lazy_static::lazy_static;
 use chrono::{DateTime, Utc};
 
-use super::backup_info::{BackupGroup, BackupDir, BackupInfo};
+use super::backup_info::{BackupGroup, BackupGroupGuard, BackupDir, BackupInfo};
 use super::chunk_store::ChunkStore;
 use super::dynamic_index::{DynamicIndexReader, DynamicIndexWriter};
 use super::fixed_index::{FixedIndexReader, FixedIndexWriter};
@@ -318,11 +318,13 @@ impl DataStore {
         Ok(())
     }
 
-    /// Create a backup group if it does not already exists.
+    /// Create (if it does not already exists) and lock a backup group
     ///
     /// And set the owner to 'userid'. If the group already exists, it returns the
     /// current owner (instead of setting the owner).
-    pub fn create_backup_group(&self, backup_group: &BackupGroup, userid: &str) -> Result<String, Error> {
+    ///
+    /// 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, BackupGroupGuard), Error> {
 
         // create intermediate path first:
         let base_path = self.base_path();
@@ -336,13 +338,15 @@ impl DataStore {
         // create the last component now
         match std::fs::create_dir(&full_path) {
             Ok(_) => {
+                let guard = backup_group.lock(&base_path)?;
                 self.set_owner(backup_group, userid, false)?;
                 let owner = self.get_owner(backup_group)?; // just to be sure
-                Ok(owner)
+                Ok((owner, guard))
             }
             Err(ref err) if err.kind() == io::ErrorKind::AlreadyExists => {
+                let guard = backup_group.lock(&base_path)?;
                 let owner = self.get_owner(backup_group)?; // just to be sure
-                Ok(owner)
+                Ok((owner, guard))
             }
             Err(err) => bail!("unable to create backup group {:?} - {}", full_path, err),
         }
diff --git a/src/client/pull.rs b/src/client/pull.rs
index c44cb9f..fc193a8 100644
--- a/src/client/pull.rs
+++ b/src/client/pull.rs
@@ -406,7 +406,7 @@ pub async fn pull_store(
     for item in list {
         let group = BackupGroup::new(&item.backup_type, &item.backup_id);
 
-        let owner = tgt_store.create_backup_group(&group, &username)?;
+        let (owner, _lock_guard) = tgt_store.create_locked_backup_group(&group, &username)?;
         // permission check
         if owner != username { // only the owner is allowed to create additional snapshots
             worker.log(format!("sync group {}/{} failed - owner check failed ({} != {})",
-- 
2.20.1

Re: [pbs-devel] [PATCH backup] src/api2/backup.rs: aquire backup lock earlier in create_locked_backup_group()

[Stefan Reiter]

Looking again, I don't think the race would have mattered (since we 
checked the owner after create_backup_group() anyway), but locking early 
is still superior and also works nicely for pull.rs.

Reviewed-By: Stefan Reiter <s.reiter@proxmox.com>

On 7/30/20 10:50 AM, Dietmar Maurer wrote:
> ---
>   src/api2/backup.rs      |  8 ++++----
>   src/backup/datastore.rs | 14 +++++++++-----
>   src/client/pull.rs      |  2 +-
>   3 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/src/api2/backup.rs b/src/api2/backup.rs
> index b9dff1f..ad13faa 100644
> --- a/src/api2/backup.rs
> +++ b/src/api2/backup.rs
> @@ -88,7 +88,10 @@ async move {
>       let env_type = rpcenv.env_type();
>   
>       let backup_group = BackupGroup::new(backup_type, backup_id);
> -    let owner = datastore.create_backup_group(&backup_group, &username)?;
> +
> +    // lock backup group to only allow one backup per group at a time
> +    let (owner, _group_guard) = datastore.create_locked_backup_group(&backup_group, &username)?;
> +
>       // permission check
>       if owner != username { // only the owner is allowed to create additional snapshots
>           bail!("backup owner check failed ({} != {})", username, owner);
> @@ -103,9 +106,6 @@ async move {
>           }
>       }
>   
> -    // lock backup group to only allow one backup per group at a time
> -    let _group_guard = backup_group.lock(&datastore.base_path())?;
> -
>       let (path, is_new) = datastore.create_backup_dir(&backup_dir)?;
>       if !is_new { bail!("backup directory already exists."); }
>   
> diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
> index 9c2f285..af964f6 100644
> --- a/src/backup/datastore.rs
> +++ b/src/backup/datastore.rs
> @@ -8,7 +8,7 @@ use anyhow::{bail, format_err, Error};
>   use lazy_static::lazy_static;
>   use chrono::{DateTime, Utc};
>   
> -use super::backup_info::{BackupGroup, BackupDir, BackupInfo};
> +use super::backup_info::{BackupGroup, BackupGroupGuard, BackupDir, BackupInfo};
>   use super::chunk_store::ChunkStore;
>   use super::dynamic_index::{DynamicIndexReader, DynamicIndexWriter};
>   use super::fixed_index::{FixedIndexReader, FixedIndexWriter};
> @@ -318,11 +318,13 @@ impl DataStore {
>           Ok(())
>       }
>   
> -    /// Create a backup group if it does not already exists.
> +    /// Create (if it does not already exists) and lock a backup group
>       ///
>       /// And set the owner to 'userid'. If the group already exists, it returns the
>       /// current owner (instead of setting the owner).
> -    pub fn create_backup_group(&self, backup_group: &BackupGroup, userid: &str) -> Result<String, Error> {
> +    ///
> +    /// 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, BackupGroupGuard), Error> {
>   
>           // create intermediate path first:
>           let base_path = self.base_path();
> @@ -336,13 +338,15 @@ impl DataStore {
>           // create the last component now
>           match std::fs::create_dir(&full_path) {
>               Ok(_) => {
> +                let guard = backup_group.lock(&base_path)?;
>                   self.set_owner(backup_group, userid, false)?;
>                   let owner = self.get_owner(backup_group)?; // just to be sure
> -                Ok(owner)
> +                Ok((owner, guard))
>               }
>               Err(ref err) if err.kind() == io::ErrorKind::AlreadyExists => {
> +                let guard = backup_group.lock(&base_path)?;
>                   let owner = self.get_owner(backup_group)?; // just to be sure
> -                Ok(owner)
> +                Ok((owner, guard))
>               }
>               Err(err) => bail!("unable to create backup group {:?} - {}", full_path, err),
>           }
> diff --git a/src/client/pull.rs b/src/client/pull.rs
> index c44cb9f..fc193a8 100644
> --- a/src/client/pull.rs
> +++ b/src/client/pull.rs
> @@ -406,7 +406,7 @@ pub async fn pull_store(
>       for item in list {
>           let group = BackupGroup::new(&item.backup_type, &item.backup_id);
>   
> -        let owner = tgt_store.create_backup_group(&group, &username)?;
> +        let (owner, _lock_guard) = tgt_store.create_locked_backup_group(&group, &username)?;
>           // permission check
>           if owner != username { // only the owner is allowed to create additional snapshots
>               worker.log(format!("sync group {}/{} failed - owner check failed ({} != {})",
>