Re: [pbs-devel] [PATCH proxmox-backup v2 1/4] traffic control: use factored out shared rate limiter

[Hannes Laimer <h.laimer@proxmox.com>]

Reviewed-by: Hannes Laimer <h.laimer@proxmox.com>
Tested-by: Hannes Laimer <h.laimer@proxmox.com>

On 9/16/25 14:42, Christian Ebner wrote:
> The shared rate limiter implementation was factored out into a
> dedicated crate so it can be easily reused for other code paths, e.g.
> the s3 client implementation.
> 
> Use that implementation and drop the now dead code.
> 
> No functional changes intended.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> Changes since version 1:
> - depend on proxmox-rate-limiter
> 
>   Cargo.toml                       |   3 +
>   debian/control                   |   1 -
>   pbs-client/Cargo.toml            |   3 +-
>   pbs-client/src/http_client.rs    |   3 +-
>   src/tools/mod.rs                 |   4 -
>   src/tools/shared_rate_limiter.rs | 122 -------------------------------
>   src/traffic_control_cache.rs     |   8 +-
>   7 files changed, 11 insertions(+), 133 deletions(-)
>   delete mode 100644 src/tools/shared_rate_limiter.rs
> 
> diff --git a/Cargo.toml b/Cargo.toml
> index b3f55b4db..e276c11de 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -74,6 +74,7 @@ proxmox-network-api = "1"
>   proxmox-notify = "1"
>   proxmox-openid = "1"
>   proxmox-product-config = "1"
> +proxmox-rate-limiter = "1.0.0"
>   proxmox-rest-server = { version = "1.0.1", features = [ "templates" ] }
>   # some use "cli", some use "cli" and "server", pbs-config uses nothing
>   proxmox-router = { version = "3.2.2", default-features = false }
> @@ -227,6 +228,7 @@ proxmox-network-api = { workspace = true, features = [ "impl" ] }
>   proxmox-notify = { workspace = true, features = [ "pbs-context" ] }
>   proxmox-openid.workspace = true
>   proxmox-product-config.workspace = true
> +proxmox-rate-limiter = { workspace = true, features = [ "shared-rate-limiter" ] }
>   proxmox-rest-server = { workspace = true, features = [ "rate-limited-stream" ] }
>   proxmox-router = { workspace = true, features = [ "cli", "server"] }
>   proxmox-s3-client.workspace = true
> @@ -288,6 +290,7 @@ proxmox-rrd-api-types.workspace = true
>   #proxmox-notify = { path = "../proxmox/proxmox-notify" }
>   #proxmox-openid = { path = "../proxmox/proxmox-openid" }
>   #proxmox-product-config = { path = "../proxmox/proxmox-product-config" }
> +#proxmox-rate-limiter = { path = "../proxmox/proxmox-rate-limiter" }
>   #proxmox-rest-server = { path = "../proxmox/proxmox-rest-server" }
>   #proxmox-router = { path = "../proxmox/proxmox-router" }
>   #proxmox-rrd = { path = "../proxmox/proxmox-rrd" }
> diff --git a/debian/control b/debian/control
> index 4346acd38..58ea000c6 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -81,7 +81,6 @@ Build-Depends: bash-completion,
>                  librust-proxmox-http-1+http-helpers-dev (>= 1.0.2-~~),
>                  librust-proxmox-http-1+proxmox-async-dev (>= 1.0.2-~~),
>                  librust-proxmox-http-1+rate-limited-stream-dev (>= 1.0.2-~~),
> -               librust-proxmox-http-1+rate-limiter-dev (>= 1.0.2-~~),
>                  librust-proxmox-http-1+websocket-dev (>= 1.0.2-~~),
>                  librust-proxmox-human-byte-1+default-dev,
>                  librust-proxmox-io-1+default-dev (>= 1.0.1-~~),
> diff --git a/pbs-client/Cargo.toml b/pbs-client/Cargo.toml
> index 4c6c80983..233969f65 100644
> --- a/pbs-client/Cargo.toml
> +++ b/pbs-client/Cargo.toml
> @@ -36,10 +36,11 @@ pathpatterns.workspace = true
>   proxmox-async.workspace = true
>   proxmox-auth-api.workspace = true
>   proxmox-compression.workspace = true
> -proxmox-http = { workspace = true, features = [ "body", "rate-limiter" ] }
> +proxmox-http = { workspace = true, features = [ "body" ] }
>   proxmox-human-byte.workspace = true
>   proxmox-io = { workspace = true, features = [ "tokio" ] }
>   proxmox-log = { workspace = true }
> +proxmox-rate-limiter = { workspace = true, features = [ "rate-limiter" ] }
>   proxmox-router = { workspace = true, features = [ "cli", "server" ] }
>   proxmox-schema.workspace = true
>   proxmox-sys.workspace = true
> diff --git a/pbs-client/src/http_client.rs b/pbs-client/src/http_client.rs
> index 74b076244..3cdae2c01 100644
> --- a/pbs-client/src/http_client.rs
> +++ b/pbs-client/src/http_client.rs
> @@ -31,8 +31,9 @@ use proxmox_async::broadcast_future::BroadcastFuture;
>   use proxmox_http::client::HttpsConnector;
>   use proxmox_http::uri::{build_authority, json_object_to_query};
>   use proxmox_http::Body;
> -use proxmox_http::{ProxyConfig, RateLimiter};
> +use proxmox_http::ProxyConfig;
>   use proxmox_log::{error, info, warn};
> +use proxmox_rate_limiter::RateLimiter;
>   
>   use pbs_api_types::percent_encoding::DEFAULT_ENCODE_SET;
>   use pbs_api_types::{Authid, RateLimitConfig, Userid};
> diff --git a/src/tools/mod.rs b/src/tools/mod.rs
> index 6556effe3..a97aba5b6 100644
> --- a/src/tools/mod.rs
> +++ b/src/tools/mod.rs
> @@ -16,10 +16,6 @@ use pbs_datastore::manifest::BackupManifest;
>   pub mod config;
>   pub mod disks;
>   pub mod fs;
> -
> -mod shared_rate_limiter;
> -pub use shared_rate_limiter::SharedRateLimiter;
> -
>   pub mod statistics;
>   pub mod systemd;
>   pub mod ticket;
> diff --git a/src/tools/shared_rate_limiter.rs b/src/tools/shared_rate_limiter.rs
> deleted file mode 100644
> index db754728b..000000000
> --- a/src/tools/shared_rate_limiter.rs
> +++ /dev/null
> @@ -1,122 +0,0 @@
> -use std::mem::MaybeUninit;
> -use std::path::PathBuf;
> -use std::time::{Duration, Instant};
> -
> -use anyhow::{bail, Error};
> -use nix::sys::stat::Mode;
> -
> -use proxmox_sys::fs::{create_path, CreateOptions};
> -
> -use proxmox_http::{RateLimit, RateLimiter, ShareableRateLimit};
> -use proxmox_shared_memory::{check_subtype, initialize_subtype};
> -use proxmox_shared_memory::{Init, SharedMemory, SharedMutex};
> -
> -// openssl::sha::sha256(b"Proxmox Backup SharedRateLimiter v1.0")[0..8];
> -pub const PROXMOX_BACKUP_SHARED_RATE_LIMITER_MAGIC_1_0: [u8; 8] =
> -    [6, 58, 213, 96, 161, 122, 130, 117];
> -
> -const BASE_PATH: &str = pbs_buildcfg::rundir!("/shmem/tbf");
> -
> -// Wrap RateLimiter, so that we can provide an Init impl
> -#[repr(C)]
> -struct WrapLimiter(RateLimiter);
> -
> -impl Init for WrapLimiter {
> -    fn initialize(this: &mut MaybeUninit<Self>) {
> -        // default does not matter here, because we override later
> -        this.write(WrapLimiter(RateLimiter::new(1_000_000, 1_000_000)));
> -    }
> -}
> -
> -#[repr(C)]
> -struct SharedRateLimiterData {
> -    magic: [u8; 8],
> -    tbf: SharedMutex<WrapLimiter>,
> -    padding: [u8; 4096 - 104],
> -}
> -
> -impl Init for SharedRateLimiterData {
> -    fn initialize(this: &mut MaybeUninit<Self>) {
> -        unsafe {
> -            let me = &mut *this.as_mut_ptr();
> -            me.magic = PROXMOX_BACKUP_SHARED_RATE_LIMITER_MAGIC_1_0;
> -            initialize_subtype(&mut me.tbf);
> -        }
> -    }
> -
> -    fn check_type_magic(this: &MaybeUninit<Self>) -> Result<(), Error> {
> -        unsafe {
> -            let me = &*this.as_ptr();
> -            if me.magic != PROXMOX_BACKUP_SHARED_RATE_LIMITER_MAGIC_1_0 {
> -                bail!("SharedRateLimiterData: wrong magic number");
> -            }
> -            check_subtype(&me.tbf)?;
> -            Ok(())
> -        }
> -    }
> -}
> -
> -/// Rate limiter designed for shared memory ([SharedMemory])
> -///
> -/// The actual [RateLimiter] is protected by a [SharedMutex] and
> -/// implements [Init]. This way we can share the limiter between
> -/// different processes.
> -pub struct SharedRateLimiter {
> -    shmem: SharedMemory<SharedRateLimiterData>,
> -}
> -
> -impl SharedRateLimiter {
> -    /// Creates a new mmap'ed instance.
> -    ///
> -    /// Data is mapped in `/var/run/proxmox-backup/shmem/tbf/<name>` using
> -    /// `TMPFS`.
> -    pub fn mmap_shmem(name: &str, rate: u64, burst: u64) -> Result<Self, Error> {
> -        let mut path = PathBuf::from(BASE_PATH);
> -
> -        let user = pbs_config::backup_user()?;
> -
> -        let dir_opts = CreateOptions::new()
> -            .perm(Mode::from_bits_truncate(0o770))
> -            .owner(user.uid)
> -            .group(user.gid);
> -
> -        create_path(&path, Some(dir_opts), Some(dir_opts))?;
> -
> -        path.push(name);
> -
> -        let file_opts = CreateOptions::new()
> -            .perm(Mode::from_bits_truncate(0o660))
> -            .owner(user.uid)
> -            .group(user.gid);
> -
> -        let shmem: SharedMemory<SharedRateLimiterData> = SharedMemory::open(&path, file_opts)?;
> -
> -        shmem.data().tbf.lock().0.update_rate(rate, burst);
> -
> -        Ok(Self { shmem })
> -    }
> -}
> -
> -impl ShareableRateLimit for SharedRateLimiter {
> -    fn update_rate(&self, rate: u64, bucket_size: u64) {
> -        self.shmem
> -            .data()
> -            .tbf
> -            .lock()
> -            .0
> -            .update_rate(rate, bucket_size);
> -    }
> -
> -    fn traffic(&self) -> u64 {
> -        self.shmem.data().tbf.lock().0.traffic()
> -    }
> -
> -    fn register_traffic(&self, current_time: Instant, data_len: u64) -> Duration {
> -        self.shmem
> -            .data()
> -            .tbf
> -            .lock()
> -            .0
> -            .register_traffic(current_time, data_len)
> -    }
> -}
> diff --git a/src/traffic_control_cache.rs b/src/traffic_control_cache.rs
> index 830a8c043..bc52d37ee 100644
> --- a/src/traffic_control_cache.rs
> +++ b/src/traffic_control_cache.rs
> @@ -8,7 +8,7 @@ use std::time::Instant;
>   use anyhow::Error;
>   use cidr::IpInet;
>   
> -use proxmox_http::{RateLimiter, ShareableRateLimit};
> +use proxmox_rate_limiter::{RateLimiter, ShareableRateLimit, SharedRateLimiter};
>   use proxmox_section_config::SectionConfigData;
>   
>   use proxmox_time::{parse_daily_duration, DailyDuration, TmEditor};
> @@ -17,8 +17,6 @@ use pbs_api_types::TrafficControlRule;
>   
>   use pbs_config::ConfigVersionCache;
>   
> -use crate::tools::SharedRateLimiter;
> -
>   pub type SharedRateLimit = Arc<dyn ShareableRateLimit>;
>   
>   /// Shared traffic control cache singleton.
> @@ -110,7 +108,9 @@ fn create_limiter(
>       burst: u64,
>   ) -> Result<SharedRateLimit, Error> {
>       if use_shared_memory {
> -        let limiter = SharedRateLimiter::mmap_shmem(name, rate, burst)?;
> +        let user = pbs_config::backup_user()?;
> +        let base_path = pbs_buildcfg::rundir!("/shmem/tbf");
> +        let limiter = SharedRateLimiter::mmap_shmem(name, rate, burst, user, base_path)?;
>           Ok(Arc::new(limiter))
>       } else {
>           Ok(Arc::new(Mutex::new(RateLimiter::new(rate, burst))))



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel