From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 08E9E1FF184 for ; Thu, 20 Nov 2025 14:07:25 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2A545818E; Thu, 20 Nov 2025 14:07:31 +0100 (CET) Message-ID: <83333385-ee83-470e-83d1-36932768f5f1@proxmox.com> Date: Thu, 20 Nov 2025 14:07:27 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: pbs-devel@lists.proxmox.com References: <20251114150544.224839-1-s.rufinatscha@proxmox.com> Content-Language: en-US From: Samuel Rufinatscha In-Reply-To: <20251114150544.224839-1-s.rufinatscha@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -1.152 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_ASCII_DIVIDERS 0.8 Email that uses ascii formatting dividers and possible spam tricks KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment KAM_MAILER 2 Automated Mailer Tag Left in Email SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pbs-devel] superseded: [PATCH proxmox-backup v2 0/4] datastore: remove config reload on hot path X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" https://lore.proxmox.com/pbs-devel/20251120130342.248815-1-s.rufinatscha@proxmox.com/T/#t On 11/14/25 4:05 PM, Samuel Rufinatscha wrote: > Hi, > > this series reduces CPU time in datastore lookups by avoiding repeated > datastore.cfg reads/parses in both `lookup_datastore()` and > `DataStore::Drop`. It also adds a TTL so manual config edits are > noticed without reintroducing hashing on every request. > > While investigating #6049 [1], cargo-flamegraph [2] showed hotspots during > repeated `/status` calls in `lookup_datastore()` and in `Drop`, > dominated by `pbs_config::datastore::config()` (config parse). > > The parsing cost itself should eventually be investigated in a future > effort. Furthermore, cargo-flamegraph showed that when using a > token-based auth method to access the API, a significant amount of time > is spent in validation on every request request [3]. > > ## Approach > > [PATCH 1/4] Extend ConfigVersionCache for datastore generation > Expose a dedicated datastore generation counter and an increment > helper so callers can cheaply track datastore.cfg versions. > > [PATCH 2/4] Fast path for datastore lookups > Cache the parsed datastore.cfg keyed by the shared datastore > generation. lookup_datastore() reuses both the cached config and an > existing DataStoreImpl when the generation matches, and falls back > to the old slow path otherwise. > > [PATCH 3/4] Fast path for Drop > Make DataStore::Drop use the cached config if possible instead of > rereading datastore.cfg from disk. > > [PATCH 4/4] TTL to catch manual edits > Add a small TTL around the cached config and bump the datastore > generation whenever the config is reloaded. This catches manual > edits to datastore.cfg without reintroducing hashing or > config parsing on every request. > > ## Benchmark results > > All the following benchmarks are based on top of > https://lore.proxmox.com/pbs-devel/20251112131525.645971-1-f.gruenbichler@proxmox.com/T/#u > > ### End-to-end > > Testing `/status?verbose=0` end-to-end with 1000 stores, 5 req/store > and parallel=16 before/after the series: > > Metric Before After > ---------------------------------------- > Total time 12s 9s > Throughput (all) 416.67 555.56 > Cold RPS (round #1) 83.33 111.11 > Warm RPS (#2..N) 333.33 444.44 > > Running under flamegraph [2], TLS appears to consume a significant > amount of CPU time and blur the results. Still, a ~33% higher overall > throughput and ~25% less end-to-end time for this workload. > > ### Isolated benchmarks (hyperfine) > > In addition to the end-to-end tests, I measured two standalone benchmarks > with hyperfine, each using a config with 1000 > datastores. `M` is the number of distinct datastores looked up and > `N` is the number of lookups per datastore. > > Drop-direct variant: > > Drops the `DataStore` after every lookup, so the `Drop` path runs on > every iteration: > > use anyhow::Error; > > use pbs_api_types::Operation; > use pbs_datastore::DataStore; > > fn main() -> Result<(), Error> { > let mut args = std::env::args(); > args.next(); > > let datastores = if let Some(n) = args.next() { > n.parse::()? > } else { > 1000 > }; > > let iterations = if let Some(n) = args.next() { > n.parse::()? > } else { > 1000 > }; > > for d in 1..=datastores { > let name = format!("ds{:04}", d); > > for i in 1..=iterations { > DataStore::lookup_datastore(&name, Some(Operation::Write))?; > } > } > > Ok(()) > } > > +----+------+-----------+-----------+---------+ > | M | N | Baseline | Patched | Speedup | > +----+------+-----------+-----------+---------+ > | 1 | 1000 | 1.670 s | 34.3 ms | 48.7x | > | 10 | 100 | 1.672 s | 34.5 ms | 48.4x | > | 100| 10 | 1.679 s | 35.1 ms | 47.8x | > |1000| 1 | 1.787 s | 38.2 ms | 46.8x | > +----+------+-----------+-----------+---------+ > > Bulk-drop variant: > > Keeps the `DataStore` instances alive for > all `N` lookups of a given datastore and then drops them in bulk, > mimicking a task that performs many lookups while it is running and > only triggers the expensive `Drop` logic when the last user exits. > > use anyhow::Error; > > use pbs_api_types::Operation; > use pbs_datastore::DataStore; > > fn main() -> Result<(), Error> { > let mut args = std::env::args(); > args.next(); > > let datastores = if let Some(n) = args.next() { > n.parse::()? > } else { > 1000 > }; > > let iterations = if let Some(n) = args.next() { > n.parse::()? > } else { > 1000 > }; > > for d in 1..=datastores { > let name = format!("ds{:04}", d); > > let mut stores = Vec::with_capacity(iterations); > for i in 1..=iterations { > stores.push(DataStore::lookup_datastore(&name, Some(Operation::Write))?); > } > } > > Ok(()) > } > > +------+------+---------------+--------------+---------+ > | M | N | Baseline mean | Patched mean | Speedup | > +------+------+---------------+--------------+---------+ > | 1 | 1000 | 884.0 ms | 33.9 ms | 26.1x | > | 10 | 100 | 881.8 ms | 35.3 ms | 25.0x | > | 100 | 10 | 969.3 ms | 35.9 ms | 27.0x | > | 1000 | 1 | 1827.0 ms | 40.7 ms | 44.9x | > +------+------+---------------+--------------+---------+ > > Both variants show that the combination of the cached config lookups > and the cheaper `Drop` handling reduces the hot-path cost from ~1.7 s > per run to a few tens of milliseconds in these benchmarks. > > ## Reproduction steps > > VM: 4 vCPU, ~8 GiB RAM, VirtIO-SCSI; disks: > - scsi0 32G (OS) > - scsi1 1000G (datastores) > > Install PBS from ISO on the VM. > > Set up ZFS on /dev/sdb (adjust if different): > > zpool create -f -o ashift=12 pbsbench /dev/sdb > zfs set mountpoint=/pbsbench pbsbench > zfs create pbsbench/pbs-bench > > Raise file-descriptor limit: > > sudo systemctl edit proxmox-backup-proxy.service > > Add the following lines: > > [Service] > LimitNOFILE=1048576 > > Reload systemd and restart the proxy: > > sudo systemctl daemon-reload > sudo systemctl restart proxmox-backup-proxy.service > > Verify the limit: > > systemctl show proxmox-backup-proxy.service | grep LimitNOFILE > > Create 1000 ZFS-backed datastores (as used in #6049 [1]): > > seq -w 001 1000 | xargs -n1 -P1 bash -c ' > id=$0 > name="ds${id}" > dataset="pbsbench/pbs-bench/${name}" > path="/pbsbench/pbs-bench/${name}" > zfs create -o mountpoint="$path" "$dataset" > proxmox-backup-manager datastore create "$name" "$path" \ > --comment "ZFS dataset-based datastore" > ' > > Build PBS from this series, then run the server under manually > under flamegraph: > > systemctl stop proxmox-backup-proxy > cargo flamegraph --release --bin proxmox-backup-proxy > > ## Other resources: > > ### E2E benchmark script: > > #!/usr/bin/env bash > set -euo pipefail > > # --- Config --------------------------------------------------------------- > HOST='https://localhost:8007' > USER='root@pam' > PASS="$(cat passfile)" > > DATASTORE_PATH="/pbsbench/pbs-bench" > MAX_STORES=1000 # how many stores to include > PARALLEL=16 # concurrent workers > REPEAT=5 # requests per store (1 cold + REPEAT-1 warm) > > PRINT_FIRST=false # true => log first request's HTTP code per store > > # --- Helpers -------------------------------------------------------------- > fmt_rps () { > local n="$1" t="$2" > awk -v n="$n" -v t="$t" 'BEGIN { if (t > 0) printf("%.2f\n", n/t); else print "0.00" }' > } > > # --- Login --------------------------------------------------------------- > auth=$(curl -ks -X POST "$HOST/api2/json/access/ticket" \ > -d "username=$USER" -d "password=$PASS") > ticket=$(echo "$auth" | jq -r '.data.ticket') > > if [[ -z "${ticket:-}" || "$ticket" == "null" ]]; then > echo "[ERROR] Login failed (no ticket)" > exit 1 > fi > > # --- Collect stores (deterministic order) -------------------------------- > mapfile -t STORES < <( > find "$DATASTORE_PATH" -mindepth 1 -maxdepth 1 -type d -printf '%f\n' \ > | sort | head -n "$MAX_STORES" > ) > > USED_STORES=${#STORES[@]} > if (( USED_STORES == 0 )); then > echo "[ERROR] No datastore dirs under $DATASTORE_PATH" > exit 1 > fi > > echo "[INFO] Running with stores=$USED_STORES, repeat=$REPEAT, parallel=$PARALLEL" > > # --- Temp counters -------------------------------------------------------- > SUCCESS_ALL="$(mktemp)" > FAIL_ALL="$(mktemp)" > COLD_OK="$(mktemp)" > WARM_OK="$(mktemp)" > trap 'rm -f "$SUCCESS_ALL" "$FAIL_ALL" "$COLD_OK" "$WARM_OK"' EXIT > > export HOST ticket REPEAT SUCCESS_ALL FAIL_ALL COLD_OK WARM_OK PRINT_FIRST > > SECONDS=0 > > # --- Fire requests -------------------------------------------------------- > printf "%s\n" "${STORES[@]}" \ > | xargs -P"$PARALLEL" -I{} bash -c ' > store="$1" > url="$HOST/api2/json/admin/datastore/$store/status?verbose=0" > > for ((i=1;i<=REPEAT;i++)); do > code=$(curl -ks -o /dev/null -w "%{http_code}" -b "PBSAuthCookie=$ticket" "$url" || echo 000) > > if [[ "$code" == "200" ]]; then > echo 1 >> "$SUCCESS_ALL" > if (( i == 1 )); then > echo 1 >> "$COLD_OK" > else > echo 1 >> "$WARM_OK" > fi > if [[ "$PRINT_FIRST" == "true" && $i -eq 1 ]]; then > ts=$(date +%H:%M:%S) > echo "[$ts] $store #$i HTTP:200" > fi > else > echo 1 >> "$FAIL_ALL" > if [[ "$PRINT_FIRST" == "true" && $i -eq 1 ]]; then > ts=$(date +%H:%M:%S) > echo "[$ts] $store #$i HTTP:$code (FAIL)" > fi > fi > done > ' _ {} > > # --- Summary -------------------------------------------------------------- > elapsed=$SECONDS > ok=$(wc -l < "$SUCCESS_ALL" 2>/dev/null || echo 0) > fail=$(wc -l < "$FAIL_ALL" 2>/dev/null || echo 0) > cold_ok=$(wc -l < "$COLD_OK" 2>/dev/null || echo 0) > warm_ok=$(wc -l < "$WARM_OK" 2>/dev/null || echo 0) > > expected=$(( USED_STORES * REPEAT )) > total=$(( ok + fail )) > > rps_all=$(fmt_rps "$ok" "$elapsed") > rps_cold=$(fmt_rps "$cold_ok" "$elapsed") > rps_warm=$(fmt_rps "$warm_ok" "$elapsed") > > echo "===== Summary =====" > echo "Stores used: $USED_STORES" > echo "Expected requests: $expected" > echo "Executed requests: $total" > echo "OK (HTTP 200): $ok" > echo "Failed: $fail" > printf "Total time: %dm %ds\n" $((elapsed/60)) $((elapsed%60)) > echo "Throughput all RPS: $rps_all" > echo "Cold RPS (round #1): $rps_cold" > echo "Warm RPS (#2..N): $rps_warm" > > ## Maintainer notes > > No dependency bumps, no API changes and no breaking changes. > > ## Patch summary > > [PATCH 1/4] partial fix #6049: config: enable config version cache for datastore > [PATCH 2/4] partial fix #6049: datastore: impl ConfigVersionCache fast path for lookups > [PATCH 3/4] partial fix #6049: datastore: use config fast-path in Drop > [PATCH 4/4] partial fix #6049: datastore: add TTL fallback to catch manual config edits > > Thanks, > Samuel > > [1] Bugzilla #6049: https://bugzilla.proxmox.com/show_bug.cgi?id=6049 > [2] cargo-flamegraph: https://github.com/flamegraph-rs/flamegraph > [3] Bugzilla #7017: https://bugzilla.proxmox.com/show_bug.cgi?id=7017 > > Samuel Rufinatscha (4): > partial fix #6049: config: enable config version cache for datastore > partial fix #6049: datastore: impl ConfigVersionCache fast path for > lookups > partial fix #6049: datastore: use config fast-path in Drop > partial fix #6049: datastore: add TTL fallback to catch manual config > edits > > pbs-config/src/config_version_cache.rs | 10 +- > pbs-datastore/Cargo.toml | 1 + > pbs-datastore/src/datastore.rs | 187 +++++++++++++++++++------ > 3 files changed, 152 insertions(+), 46 deletions(-) > _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel