Re: [PATCH proxmox-backup v3 24/30] sync: pull: introduce and use decrypt index writer if crypt config

Thomas Lamprecht
2026-04-19T21:08:24Z

Am 14.04.26 um 14:59 schrieb Christian Ebner:
> diff --git a/src/server/pull.rs b/src/server/pull.rs
>
> + if let DecryptedIndexWriter::None = decrypted_index_writer {
> + stream
> + .try_buffer_unordered(20)
> + .try_for_each(|_res| futures::future::ok(()))
> + .await?;
> + } else {
> + // must keep chunk order to correctly rewrite index file
> + stream.try_for_each(|item| item).await?;
> + }

This drops back to fully sequential processing for decrypt-pulls (each
chunk: read -> decrypt -> re-encode -> index-append, one at a time), which
will make decrypt-pull significantly slower than a normal pull on large
archives - no concurrency for network reads, decryption or re-encoding.

With the AtomicU64 offset counter added in patch 27, the offset is no longer
dependent on serial processing order, so ordered buffering (e.g.
futures::stream::StreamExt::buffered(N)) might (!) be safe and restore
some concurrency. The critical sections around encountered_chunks /
index.add_chunk are already guarded by an Mutex anyway.

But also something we can revisit after initially landing this work,
so such optimization shouldn't be an all to big focus for now IMO.

> + if let DecryptedIndexWriter::Dynamic(index) = &new_index_writer {
> + let csum = index.lock().unwrap().close()?;
> +
> + // Overwrite current tmp file so it will be persisted instead
> + std::fs::rename(&path, &tmp_path)?;
> + }

nit: `csum` is unused here (and in the FixedIndex branch further down). In
patch 28 it does get used for new_manifest.add_file(), so please just fold
that usage into this patch, or drop the binding until patch 28 introduces
the consumer.

> + // Overwrite current tmp file so it will be persisted instead
> + std::fs::rename(&path, &tmp_path)?;

nit: renaming the just-created index file (at `path`) back over `tmp_path`
so the outer persist step picks it up is a pretty subtle dance. A sentence
on *why* the writer can't just create directly at `tmp_path` would help the
next reader - otherwise this looks like an accidentally-reversed rename at
first glance.

> +#[derive(Clone)]
> +enum DecryptedIndexWriter {
> + Fixed(Arc<Mutex<FixedIndexWriter>>),
> + Dynamic(Arc<Mutex<DynamicIndexWriter>>),
> + None,
> +}

nit: having a `None` variant alongside Fixed/Dynamic doubles the nesting in
all callers (needs either `if let DecryptedIndexWriter::None` or
match-with-default arm). An `Option<DecryptedIndexWriter>` OTOH with
DecryptedIndexWriter only containing the two real variants would make usage
slightly more ergonomic I think and FWIW match the existing `crypt_config:
Option<_>` idiom in this file.