From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <pbs-devel-bounces@lists.proxmox.com> Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 584051FF187 for <inbox@lore.proxmox.com>; Wed, 9 Apr 2025 12:23:23 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9FAB78DCB; Wed, 9 Apr 2025 12:23:19 +0200 (CEST) Mime-Version: 1.0 Date: Wed, 09 Apr 2025 12:22:46 +0200 Message-Id: <D9218ALM6I5W.1P29S3INCGLUT@proxmox.com> To: "Christian Ebner" <c.ebner@proxmox.com>, "Proxmox Backup Server development discussion" <pbs-devel@lists.proxmox.com> From: "Max Carrara" <m.carrara@proxmox.com> X-Mailer: aerc 0.18.2-0-ge037c095a049 References: <20250404134936.425392-1-c.ebner@proxmox.com> <D8Y1UJVBD2KE.I6J4VWZW6PXQ@proxmox.com> <850bd324-9251-4f69-9526-5cf6759bebd6@proxmox.com> In-Reply-To: <850bd324-9251-4f69-9526-5cf6759bebd6@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.078 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com] Subject: Re: [pbs-devel] [PATCH v4 proxmox proxmox-backup 0/7] fix #4182: concurrent group pull/push support for sync jobs X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion <pbs-devel.lists.proxmox.com> List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>, <mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe> List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/> List-Post: <mailto:pbs-devel@lists.proxmox.com> List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help> List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>, <mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe> Reply-To: Proxmox Backup Server development discussion <pbs-devel@lists.proxmox.com> Cc: Thomas Lamprecht <t.lamprecht@proxmox.com> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" <pbs-devel-bounces@lists.proxmox.com> On Sat Apr 5, 2025 at 11:31 AM CEST, Christian Ebner wrote: > On 4/4/25 20:01, Max Carrara wrote: > > On Fri Apr 4, 2025 at 3:49 PM CEST, Christian Ebner wrote: > >> Syncing contents from/to a remote source via a sync job suffers from > >> low throughput on high latency networks because of limitations by the > >> HTTP/2 connection, as described in [0]. To improve, syncing multiple > >> groups in parallel by establishing multiple reader instances has been > >> suggested. > >> > >> This patch series implements the functionality by adding the sync job > >> configuration property `parallel-groups`, allowing to define the > >> number of concurrent groups pull/push futures to be instantiated and > >> executed for each job. > >> The property is currently not exposed on the UI, as intended to be > >> set in the config directly for now. > >> > >> Examplary configuration: > >> ``` > >> sync: s-8764c440-3a6c > >> ns > >> owner root@pam > >> remote local > >> remote-ns > >> remote-store push-target-store > >> remove-vanished false > >> store datastore > >> sync-direction push > >> parallel-groups 4 > >> ``` > >> > >> Since log messages are now also written concurrently, prefix logs > >> related to groups, snapshots and archives with their respective > >> context prefix and add context to error messages. > >> > >> Further, improve logging especially for sync jobs in push direction, > >> which only displayed limited information so far. > >> > >> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=4182 > > > > So, I've given the code a good look -- unfortunately it's too late to do > > any additional testing, but I wanted to shoot this out regardless in the > > meantime. > > > > Code Review > > =========== > > > > As always, the code quality is pristine -- I like that you're factoring > > things out into little helper functions where applicable instead of > > letting the existing methods grow. Very nice. Also applies cleanly > > and is formatted correctly, naturally. Really can't complain, the > > changes are straightforward and easy to follow. > > > > There's only a couple little things I spotted; see my comments inline. > > > > Regarding that large comment about mutexes and atomics: That's something > > I just wanted to mention, so just to make it clear, you don't need to > > apply my suggestion :P It's probably something we should have a look at > > tree-wide for other data structures, too. > > > > Splendid work as always, anyhow! > > > > For now, until I get to test this, consider: > > > > Reviewed-by: Max Carrara <m.carrara@proxmox.com> > > Thanks for your efforts on this one, appreciated! > > There are however 2 things which make me hesitate with bringing this > patch series further in its current approach: > > - Thomas raised valid concerns about the feasibility of adding such > parallelism parameters just for the sake of a quick fix [0]. > > - There was a user report about sync jobs being slow over a VPN > connection, which resulted in networking adjustments which did > significantly increase the throughput on his side [1]. He documented > this in the bugtracker issue upon my request [2]. > > So I would like to rather investigate if congestion control settings can > help increase performance for the sync jobs rather than adding the > parallel group sync for now. > > What do you think? (CC'ing also Thomas asking for his opinion). > > [0] > https://lore.proxmox.com/pbs-devel/e5a2dcac-630e-4797-bbbf-f38bc260c2ca@proxmox.com/ > [1] https://forum.proxmox.com/threads/164450/post-761198 > [2] https://bugzilla.proxmox.com/show_bug.cgi?id=4182#c12 Huh! That's very interesting! Yeah, seems perfectly reasonable to focus on congestion control settings instead, then. If you recall, Gabriel and I had cooked up a scheduler prototype a couple months ago. We had actually managed to get parallel backup jobs going e.g., with the max amount of jobs being adjustable. My plan was to extend this to pretty much any type of job, but there were a *lot* of open questions and design decisions to be made in general there--so we didn't continue iterating on it, as there were more important things to address. So, should be unbury that prototype at some point in the future, perhaps we can incorporate some of the things here in this series. I can't give you an estimate on when that will happen though, as Gabriel is working on SDN and I'm working on Storage atm. _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel