* [pbs-devel] [PATCH backup] src/api2/backup.rs: aquire backup lock earlier in create_locked_backup_group()
@ 2020-07-30 8:50 Dietmar Maurer
2020-07-30 9:31 ` Stefan Reiter
0 siblings, 1 reply; 2+ messages in thread
From: Dietmar Maurer @ 2020-07-30 8:50 UTC (permalink / raw)
To: pbs-devel
---
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [pbs-devel] [PATCH backup] src/api2/backup.rs: aquire backup lock earlier in create_locked_backup_group()
2020-07-30 8:50 [pbs-devel] [PATCH backup] src/api2/backup.rs: aquire backup lock earlier in create_locked_backup_group() Dietmar Maurer
@ 2020-07-30 9:31 ` Stefan Reiter
0 siblings, 0 replies; 2+ messages in thread
From: Stefan Reiter @ 2020-07-30 9:31 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Dietmar Maurer
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 ({} != {})",
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-07-30 9:31 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-30 8:50 [pbs-devel] [PATCH backup] src/api2/backup.rs: aquire backup lock earlier in create_locked_backup_group() Dietmar Maurer
2020-07-30 9:31 ` Stefan Reiter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox