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

* [pbs-devel] [PATCH proxmox-backup v4 0/4] datastore: remove config reload on hot path
@ 2025-11-24 15:33 Samuel Rufinatscha
  2025-11-24 15:33 ` [pbs-devel] [PATCH proxmox-backup v4 1/4] partial fix #6049: config: enable config version cache for datastore Samuel Rufinatscha
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Samuel Rufinatscha @ 2025-11-24 15:33 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] Support datastore generation in ConfigVersionCache

[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. The caching logic is implemented
  using the datastore_section_config_cached(update_cache: bool) helper.

[PATCH 3/4] Fast path for Drop
  Make DataStore::Drop use the datastore_section_config_cached()
  helper to avoid re-reading/parsing datastore.cfg on every Drop.
  Bump generation not only on API config saves, but also on slow-path
  lookups (if update_cache is true), to enable Drop handlers see
  eventual newer configs.

[PATCH 4/4] TTL to catch manual edits
  Add a TTL to the cached config and bump the datastore generation iff
  the digest changed but generation stays the same. This catches manual
  edits to datastore.cfg without reintroducing hashing or config
  parsing on every request.

## Benchmark results

### 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.684 s   | 35.3 ms   | 47.7x   |
    | 10 | 100  | 1.689 s   | 35.0 ms   | 48.3x   |
    | 100| 10   | 1.709 s   | 35.8 ms   | 47.7x   |
    |1000| 1    | 1.809 s   | 39.0 ms   | 46.4x   |
    +----+------+-----------+-----------+---------+

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 |   890.6 ms    |   35.5 ms    |  25.1x  |
    | 10   | 100  |   891.3 ms    |   35.1 ms    |  25.4x  |
    | 100  | 10   |   983.9 ms    |   35.6 ms    |  27.6x  |
    | 1000 | 1    |  1829.0 ms    |   45.2 ms    |  40.5x  |
    +------+------+---------------+--------------+---------+

Both variants show that the combination of the cached config lookups
and the cheaper `Drop` handling reduces the hot-path cost from ~1.8 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

## 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

## Maintainer notes

No dependency bumps, no API changes and no breaking changes.

Thanks,
Samuel

Links

[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         | 215 ++++++++++++++++++++-----
 3 files changed, 180 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] 6+ messages in thread