[pbs-devel] [PATCH proxmox-backup v2 0/4] datastore: remove config reload on hot path

* [pbs-devel] [PATCH proxmox-backup v2 0/4] datastore: remove config reload on hot path
@ 2025-11-14 15:05 Samuel Rufinatscha
  2025-11-14 15:05 ` [pbs-devel] [PATCH proxmox-backup v2 1/4] partial fix #6049: config: enable config version cache for datastore Samuel Rufinatscha
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Samuel Rufinatscha @ 2025-11-14 15:05 UTC (permalink / raw)
  To: pbs-devel

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::<usize>()?
        } else {
            1000
        };

        let iterations = if let Some(n) = args.next() {
            n.parse::<usize>()?
        } 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::<usize>()?
        } else {
            1000
        };

        let iterations = if let Some(n) = args.next() {
            n.parse::<usize>()?
        } 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(-)

-- 
2.47.3

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

^ permalink raw reply	[flat|nested] 5+ messages in thread