From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pbs-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id EC47B1FF15C
for <inbox@lore.proxmox.com>; Wed, 26 Mar 2025 16:15:31 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id EB8E33AFC1;
Wed, 26 Mar 2025 16:15:25 +0100 (CET)
Message-ID: <b89b04cb-1a44-4095-9564-111567d9ffc4@proxmox.com>
Date: Wed, 26 Mar 2025 16:14:52 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird Beta
To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>, Shannon Sterz <s.sterz@proxmox.com>
References: <20250326114414.132478-1-s.sterz@proxmox.com>
<20250326114414.132478-3-s.sterz@proxmox.com>
Content-Language: en-GB, de-AT
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
Autocrypt: addr=t.lamprecht@proxmox.com; keydata=
xsFNBFsLjcYBEACsaQP6uTtw/xHTUCKF4VD4/Wfg7gGn47+OfCKJQAD+Oyb3HSBkjclopC5J
uXsB1vVOfqVYE6PO8FlD2L5nxgT3SWkc6Ka634G/yGDU3ZC3C/7NcDVKhSBI5E0ww4Qj8s9w
OQRloemb5LOBkJNEUshkWRTHHOmk6QqFB/qBPW2COpAx6oyxVUvBCgm/1S0dAZ9gfkvpqFSD
90B5j3bL6i9FIv3YGUCgz6Ue3f7u+HsEAew6TMtlt90XV3vT4M2IOuECG/pXwTy7NtmHaBQ7
UJBcwSOpDEweNob50+9B4KbnVn1ydx+K6UnEcGDvUWBkREccvuExvupYYYQ5dIhRFf3fkS4+
wMlyAFh8PQUgauod+vqs45FJaSgTqIALSBsEHKEs6IoTXtnnpbhu3p6XBin4hunwoBFiyYt6
YHLAM1yLfCyX510DFzX/Ze2hLqatqzY5Wa7NIXqYYelz7tXiuCLHP84+sV6JtEkeSUCuOiUY
virj6nT/nJK8m0BzdR6FgGtNxp7RVXFRz/+mwijJVLpFsyG1i0Hmv2zTn3h2nyGK/I6yhFNt
dX69y5hbo6LAsRjLUvZeHXpTU4TrpN/WiCjJblbj5um5eEr4yhcwhVmG102puTtuCECsDucZ
jpKpUqzXlpLbzG/dp9dXFH3MivvfuaHrg3MtjXY1i+/Oxyp5iwARAQABzTNUaG9tYXMgTGFt
cHJlY2h0IChBdXRoLTQpIDx0LmxhbXByZWNodEBwcm94bW94LmNvbT7CwY4EEwEIADgWIQQO
R4qbEl/pah9K6VrTZCM6gDZWBgUCWwuNxgIbAwULCQgHAgYVCAkKCwIEFgIDAQIeAQIXgAAK
CRDTZCM6gDZWBm/jD/4+6JB2s67eaqoP6x9VGaXNGJPCscwzLuxDTCG90G9FYu29VcXtubH/
bPwsyBbNUQpqTm/s4XboU2qpS5ykCuTjqavrcP33tdkYfGcItj2xMipJ1i3TWvpikQVsX42R
G64wovLs/dvpTYphRZkg5DwhgTmy3mRkmofFCTa+//MOcNOORltemp984tWjpR3bUJETNWpF
sKGZHa3N4kCNxb7A+VMsJZ/1gN3jbQbQG7GkJtnHlWkw9rKCYqBtWrnrHa4UAvSa9M/XCIAB
FThFGqZI1ojdVlv5gd6b/nWxfOPrLlSxbUo5FZ1i/ycj7/24nznW1V4ykG9iUld4uYUY86bB
UGSjew1KYp9FmvKiwEoB+zxNnuEQfS7/Bj1X9nxizgweiHIyFsRqgogTvLh403QMSGNSoArk
tqkorf1U+VhEncIn4H3KksJF0njZKfilrieOO7Vuot1xKr9QnYrZzJ7m7ZxJ/JfKGaRHXkE1
feMmrvZD1AtdUATZkoeQtTOpMu4r6IQRfSdwm/CkppZXfDe50DJxAMDWwfK2rr2bVkNg/yZI
tKLBS0YgRTIynkvv0h8d9dIjiicw3RMeYXyqOnSWVva2r+tl+JBaenr8YTQw0zARrhC0mttu
cIZGnVEvQuDwib57QLqMjQaC1gazKHvhA15H5MNxUhwm229UmdH3KM7BTQRbC43GARAAyTkR
D6KRJ9Xa2fVMh+6f186q0M3ni+5tsaVhUiykxjsPgkuWXWW9MbLpYXkzX6h/RIEKlo2BGA95
QwG5+Ya2Bo3g7FGJHAkXY6loq7DgMp5/TVQ8phsSv3WxPTJLCBq6vNBamp5hda4cfXFUymsy
HsJy4dtgkrPQ/bnsdFDCRUuhJHopnAzKHN8APXpKU6xV5e3GE4LwFsDhNHfH/m9+2yO/trcD
txSFpyftbK2gaMERHgA8SKkzRhiwRTt9w5idOfpJVkYRsgvuSGZ0pcD4kLCOIFrer5xXudk6
NgJc36XkFRMnwqrL/bB4k6Pi2u5leyqcXSLyBgeHsZJxg6Lcr2LZ35+8RQGPOw9C0ItmRjtY
ZpGKPlSxjxA1WHT2YlF9CEt3nx7c4C3thHHtqBra6BGPyW8rvtq4zRqZRLPmZ0kt/kiMPhTM
8wZAlObbATVrUMcZ/uNjRv2vU9O5aTAD9E5r1B0dlqKgxyoImUWB0JgpILADaT3VybDd3C8X
s6Jt8MytUP+1cEWt9VKo4vY4Jh5vwrJUDLJvzpN+TsYCZPNVj18+jf9uGRaoK6W++DdMAr5l
gQiwsNgf9372dbMI7pt2gnT5/YdG+ZHnIIlXC6OUonA1Ro/Itg90Q7iQySnKKkqqnWVc+qO9
GJbzcGykxD6EQtCSlurt3/5IXTA7t6sAEQEAAcLBdgQYAQgAIBYhBA5HipsSX+lqH0rpWtNk
IzqANlYGBQJbC43GAhsMAAoJENNkIzqANlYGD1sP/ikKgHgcspEKqDED9gQrTBvipH85si0j
/Jwu/tBtnYjLgKLh2cjv1JkgYYjb3DyZa1pLsIv6rGnPX9bH9IN03nqirC/Q1Y1lnbNTynPk
IflgvsJjoTNZjgu1wUdQlBgL/JhUp1sIYID11jZphgzfDgp/E6ve/8xE2HMAnf4zAfJaKgD0
F+fL1DlcdYUditAiYEuN40Ns/abKs8I1MYx7Yglu3RzJfBzV4t86DAR+OvuF9v188WrFwXCS
RSf4DmJ8tntyNej+DVGUnmKHupLQJO7uqCKB/1HLlMKc5G3GLoGqJliHjUHUAXNzinlpE2Vj
C78pxpwxRNg2ilE3AhPoAXrY5qED5PLE9sLnmQ9AzRcMMJUXjTNEDxEYbF55SdGBHHOAcZtA
kEQKub86e+GHA+Z8oXQSGeSGOkqHi7zfgW1UexddTvaRwE6AyZ6FxTApm8wq8NT2cryWPWTF
BDSGB3ujWHMM8ERRYJPcBSjTvt0GcEqnd+OSGgxTkGOdufn51oz82zfpVo1t+J/FNz6MRMcg
8nEC+uKvgzH1nujxJ5pRCBOquFZaGn/p71Yr0oVitkttLKblFsqwa+10Lt6HBxm+2+VLp4Ja
0WZNncZciz3V3cuArpan/ZhhyiWYV5FD0pOXPCJIx7WS9PTtxiv0AOS4ScWEUmBxyhFeOpYa DrEx
In-Reply-To: <20250326114414.132478-3-s.sterz@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.039 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [environment.rs, proxmox.com, sync.rs, lib.rs, datastore.rs,
verify.rs]
Subject: Re: [pbs-devel] [PATCH proxmox-backup v9 2/4] fix #3935:
datastore/api/backup: move datastore locking to '/run'
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pbs-devel-bounces@lists.proxmox.com
Sender: "pbs-devel" <pbs-devel-bounces@lists.proxmox.com>
Am 26.03.25 um 12:44 schrieb Shannon Sterz:
> to avoid issues when removing a group or snapshot directory where two
> threads hold a lock to the same directory, move locking to the tmpfs
> backed '/run' directory. also adds double stat'ing to make it possible
> to remove locks without certain race condition issues.
>
> this new mechanism is only employed when we can be sure, that a reboot
> has occured so that all processes are using the new locking mechanism.
> otherwise, two separate process could assume they have exclusive
> rights to a group or snapshot.
>
> bumps the rust version to 1.81 so we can use `std::fs::exists` without
> issue.
>
> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
> ---
> Cargo.toml | 2 +-
> debian/postinst | 5 +
> debian/rules | 2 +
> pbs-config/src/lib.rs | 32 ++++-
> pbs-datastore/Cargo.toml | 1 +
> pbs-datastore/src/backup_info.rs | 195 +++++++++++++++++++++++----
> pbs-datastore/src/datastore.rs | 6 +-
> pbs-datastore/src/snapshot_reader.rs | 15 ++-
> src/api2/backup/environment.rs | 5 +-
> src/backup/verify.rs | 4 +-
> src/server/sync.rs | 4 +-
> 11 files changed, 229 insertions(+), 42 deletions(-)
>
> diff --git a/Cargo.toml b/Cargo.toml
> index 306d50e9..645f2d81 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -13,7 +13,7 @@ authors = [
> edition = "2021"
> license = "AGPL-3"
> repository = "https://git.proxmox.com/?p=proxmox-backup.git"
> -rust-version = "1.80"
> +rust-version = "1.81"
>
> [package]
> name = "proxmox-backup"
> diff --git a/debian/postinst b/debian/postinst
> index 0485ca34..81c15826 100644
> --- a/debian/postinst
> +++ b/debian/postinst
> @@ -80,6 +80,11 @@ EOF
> update_sync_job "$prev_job"
> fi
> fi
> +
> + if dpkg --compare-versions "$2" 'lt' '3.3.4~'; then
> + # ensure old locking is used by the daemon until a reboot happened
> + touch "/run/proxmox-backup/old-locking"
> + fi
> fi
> ;;
>
> diff --git a/debian/rules b/debian/rules
> index a03fe11b..6e1844e1 100755
> --- a/debian/rules
> +++ b/debian/rules
> @@ -22,6 +22,8 @@ ifneq ("$(wildcard .repoid)","")
> endif
>
> %:
> + @echo "fix locking version in postinst"
> + false
> dh $@ --with=bash-completion
>
> override_dh_auto_configure:
> diff --git a/pbs-config/src/lib.rs b/pbs-config/src/lib.rs
> index 20a8238d..9c4d77c2 100644
> --- a/pbs-config/src/lib.rs
> +++ b/pbs-config/src/lib.rs
> @@ -22,6 +22,8 @@ pub use config_version_cache::ConfigVersionCache;
>
> use anyhow::{format_err, Error};
> use nix::unistd::{Gid, Group, Uid, User};
> +use proxmox_sys::fs::DirLockGuard;
> +use std::os::unix::prelude::AsRawFd;
>
> pub use pbs_buildcfg::{BACKUP_GROUP_NAME, BACKUP_USER_NAME};
>
> @@ -46,13 +48,34 @@ pub fn backup_group() -> Result<nix::unistd::Group, Error> {
> }
>
> pub struct BackupLockGuard {
> - _file: Option<std::fs::File>,
> + file: Option<std::fs::File>,
> + // TODO: Remove `_legacy_dir` with PBS 5
> + _legacy_dir: Option<DirLockGuard>,
> +}
> +
> +impl AsRawFd for BackupLockGuard {
> + fn as_raw_fd(&self) -> i32 {
> + self.file.as_ref().map_or(-1, |f| f.as_raw_fd())
> + }
> +}
> +
> +// TODO: Remove with PBS 5
> +impl From<DirLockGuard> for BackupLockGuard {
> + fn from(value: DirLockGuard) -> Self {
> + Self {
> + file: None,
> + _legacy_dir: Some(value),
> + }
> + }
> }
>
> #[doc(hidden)]
> /// Note: do not use for production code, this is only intended for tests
> pub unsafe fn create_mocked_lock() -> BackupLockGuard {
> - BackupLockGuard { _file: None }
> + BackupLockGuard {
> + file: None,
> + _legacy_dir: None,
> + }
> }
>
> /// Open or create a lock file owned by user "backup" and lock it.
> @@ -76,7 +99,10 @@ pub fn open_backup_lockfile<P: AsRef<std::path::Path>>(
> let timeout = timeout.unwrap_or(std::time::Duration::new(10, 0));
>
> let file = proxmox_sys::fs::open_file_locked(&path, timeout, exclusive, options)?;
> - Ok(BackupLockGuard { _file: Some(file) })
> + Ok(BackupLockGuard {
> + file: Some(file),
> + _legacy_dir: None,
> + })
> }
>
> /// Atomically write data to file owned by "root:backup" with permission "0640"
> diff --git a/pbs-datastore/Cargo.toml b/pbs-datastore/Cargo.toml
> index 4ebc5fdc..7623adc2 100644
> --- a/pbs-datastore/Cargo.toml
> +++ b/pbs-datastore/Cargo.toml
> @@ -35,6 +35,7 @@ proxmox-lang.workspace=true
> proxmox-schema = { workspace = true, features = [ "api-macro" ] }
> proxmox-serde = { workspace = true, features = [ "serde_json" ] }
> proxmox-sys.workspace = true
> +proxmox-systemd.workspace = true
> proxmox-time.workspace = true
> proxmox-uuid.workspace = true
> proxmox-worker-task.workspace = true
> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
> index c7f7ed53..4e381142 100644
> --- a/pbs-datastore/src/backup_info.rs
> +++ b/pbs-datastore/src/backup_info.rs
> @@ -1,13 +1,15 @@
> use std::fmt;
> -use std::os::unix::io::RawFd;
> +use std::os::unix::io::{AsRawFd, RawFd};
> +use std::os::unix::prelude::OsStrExt;
> +use std::path::Path;
> use std::path::PathBuf;
> -use std::sync::Arc;
> +use std::sync::{Arc, LazyLock};
> +use std::time::Duration;
>
> use anyhow::{bail, format_err, Context, Error};
>
> -use proxmox_sys::fs::{
> - lock_dir_noblock, lock_dir_noblock_shared, replace_file, CreateOptions, DirLockGuard,
> -};
> +use proxmox_sys::fs::{lock_dir_noblock, lock_dir_noblock_shared, replace_file, CreateOptions};
> +use proxmox_systemd::escape_unit;
>
> use pbs_api_types::{
> Authid, BackupGroupDeleteStats, BackupNamespace, BackupType, GroupFilter, VerifyState,
> @@ -18,6 +20,18 @@ use pbs_config::{open_backup_lockfile, BackupLockGuard};
> use crate::manifest::{BackupManifest, MANIFEST_LOCK_NAME};
> use crate::{DataBlob, DataStore};
>
> +pub const DATASTORE_LOCKS_DIR: &str = "/run/proxmox-backup/locks";
I was shortly tempted to question if this should be in /run/lock/proxmox-backup,
but we already keep the index manifest locks in there, so it's probably fine
and might be even stranger if we would move just these new lock paths to
/run/lock/proxmox-backup I think. We could always create a symlink that points
/run/lock/proxmox-backup -> /run/proxmox-backup/locks if one really questions this.
So just to vent this thought out, it's fine as is.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel