From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 623461FF135 for ; Sun, 19 Apr 2026 23:08:24 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 632BA17678; Sun, 19 Apr 2026 23:08:22 +0200 (CEST) From: Thomas Lamprecht To: c.ebner@proxmox.com Subject: Re: [PATCH proxmox-backup v3 24/30] sync: pull: introduce and use decrypt index writer if crypt config Date: Sun, 19 Apr 2026 22:41:56 +0200 Message-ID: <20260419210610.3915597-7-t.lamprecht@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260414125923.892345-25-c.ebner@proxmox.com> References: <20260419210610.3915597-1-t.lamprecht@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1776632780729 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.002 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_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: NR4KVIEPHXYZEUOYZAY3GIDHJVHR3347 X-Message-ID-Hash: NR4KVIEPHXYZEUOYZAY3GIDHJVHR3347 X-MailFrom: t.lamprecht@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pbs-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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>), > + Dynamic(Arc>), > + 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` 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.