From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 065471FF09C for ; Mon, 05 Oct 2026 12:27:27 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id D0DB9215AD; Mon, 05 Oct 2026 12:27:24 +0200 (CEST) Content-Type: text/plain; charset=UTF-8 Date: Mon, 05 Oct 2026 12:27:20 +0200 Message-Id: From: =?utf-8?q?Michael_K=C3=B6ppl?= To: "Fiona Ebner" , =?utf-8?q?Michael_K=C3=B6ppl?= , Subject: Re: [PATCH guest-common v6 07/18] guest id: keep used ID list below the pmxcfs file size limit Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Mailer: aerc 0.22.0 References: <20260924161510.847362-1-m.koeppl@proxmox.com> <20260924161510.847362-8-m.koeppl@proxmox.com> <23c3d0c2-2853-4029-ae74-e3c272f3b980@proxmox.com> In-Reply-To: <23c3d0c2-2853-4029-ae74-e3c272f3b980@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1791196040217 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.398 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust 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: 2VQB5BF5LJ6OZJ3NBYAGNTG5KDE5BGUZ X-Message-ID-Hash: 2VQB5BF5LJ6OZJ3NBYAGNTG5KDE5BGUZ X-MailFrom: m.koeppl@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 X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Thu Oct 1, 2026 at 2:03 PM CEST, Fiona Ebner wrote: [snip] >> merging ranges until the file sizes is at $SIZE_WARN again. That would >> mean that even more guest IDs would be marked as used even though not >> strictly necessary. Feedback here would be much appreciated. I think it >> can be argued that repeated warnings if the file size if between >> $SIZE_WARN and $SIZE_MAX are annoying, but I'm not sure if the "lost" >> guest IDs would be worth it. > > In practice, this should be very rare. Essentially only with very > specific ID usage patterns like only using even numbers. And if using > such patterns, closing the gaps only gets rids of IDs they didn't use in > their patterns anyways. If somebody really complains about too many > warnings, we can still adapt later, but I think the current approach is > fine. ack, thanks for the feedback! Will leave it as is for now. > >> >> src/PVE/GuestID.pm | 104 ++++++++++++++++++++++++++++++++++++++++----- >> 1 file changed, 93 insertions(+), 11 deletions(-) >> >> diff --git a/src/PVE/GuestID.pm b/src/PVE/GuestID.pm >> index 862d92b..4ab6f2d 100644 >> --- a/src/PVE/GuestID.pm >> +++ b/src/PVE/GuestID.pm >> @@ -11,6 +11,10 @@ use PVE::Cluster qw( >> >> my $FILENAME =3D 'virtual-guest/used-guest-ids'; >> >> +# keep well below the 1 MiB file size limit of pmxcfs, writes fail beyo= nd it >> +my $SIZE_WARN =3D 512 * 1024; >> +my $SIZE_MAX =3D 768 * 1024; >> + >> my sub parse_id_list($filename, $raw) { >> my $ranges =3D []; >> >> @@ -54,24 +58,102 @@ my sub format_entry($start, $last) { >> return $start =3D=3D $last ? "$start\n" : "$start-$last\n"; >> } >> >> -my sub write_id_list($filename, $ranges) { >> - my $output =3D ''; >> - my ($start, $last); >> +my sub merge_ranges($ranges) { >> + my $merged =3D []; >> >> for my $range ($ranges->@*) { >> - my ($curr_start, $curr_end) =3D $range->@*; >> + my ($start, $end) =3D $range->@*; >> + my $last =3D $merged->[-1]; > > I'd suggest s/$last/$current_range/ or s/$last/$current_merge/ > > IMHO, $last is too similar to $end. will change to $current_merge in v7, to avoid confusion with $range. Thanks! > >> >> - if (!defined($start)) { >> - ($start, $last) =3D ($curr_start, $curr_end); >> - } elsif ($curr_start <=3D $last + 1) { >> - $last =3D $curr_end if $curr_end > $last; >> + if ($last && $start <=3D $last->[1] + 1) { >> + $last->[1] =3D $end if $end > $last->[1]; >> } else { >> - $output .=3D format_entry($start, $last); >> - ($start, $last) =3D ($curr_start, $curr_end); >> + push $merged->@*, [$start, $end]; >> } >> } >> >> - $output .=3D format_entry($start, $last) if defined($start); >> + return $merged; >> +} >> + >> +my sub format_ranges($ranges) { >> + return join('', map { format_entry($_->@*) } $ranges->@*); >> +} >> + >> +# Closes the smallest gaps between ranges in $merged until the >> +# formatted list of $size bytes fits into $SIZE_MAX. The unused IDs in >> +# a closed gap count as used from then on. Returns the resulting ranges >> +# and the number of unused IDs that got marked as used. >> +my sub close_smallest_gaps($merged, $size) { > > Since there are different sizes handled below, I'd be more precise with > something like $output_size will also update, thanks! > >> + my @gap_sizes; >> + for my $i (1 .. $#$merged) { >> + my $prev_end =3D $merged->[$i - 1]->[1]; >> + my $curr_start =3D $merged->[$i]->[0]; >> + >> + $gap_sizes[$i] =3D $curr_start - $prev_end - 1; >> + } >> + >> + my @gap_order =3D sort { $gap_sizes[$a] <=3D> $gap_sizes[$b] || $a = <=3D> $b } 1 .. $#$merged; >> + >> + # Record start and end of each group. Initially, with e.g. 6 ranges= , >> + # this would be: >> + # start: (0, 1, 2, 3, 4, 5) >> + # end: (0, 1, 2, 3, 4, 5) >> + # After joining the ranges at indices 1-3 and at 4 5, this would be= : >> + # start: (0, 1, 1, 1, 4, 4) >> + # end: (0, 3, 2, 3, 5, 5) > > Since it was not immediately obvious to me why it would be 3, 2, 3 > instead of 3, 3, 3, I suggest expanding the comment: > >> # Record start and end of each group. Initially, with e.g. 6 ranges, >> # this would be: >> # start: (0, 1, 2, 3, 4, 5) >> # end: (0, 1, 2, 3, 4, 5) >> # After closing the gap between 1 and 2: >> # start: (0, 1, 1, 3, 4, 5) >> # end: (0, 2, 2, 3, 4, 5) >> # After closing the gap between 2 and 3 (note that the range 2 is no= w fully >> # merged into adjacent ranges, so its group indices won't be accesse= d >> # anymore): >> # start: (0, 1, 1, 1, 4, 5) >> # end: (0, 3, 2, 3, 4, 5) >> # After closing the gap between 4 and 5: >> # start: (0, 1, 1, 1, 4, 4) >> # end: (0, 3, 2, 3, 5, 5) > > Hope I didn't get it wrong ^^ You got it exactly right and it's a good addition to the example that makes it a bit clearer what's happening. Will adapt the comment for v7, thanks! :) > >> + my @group_start =3D (0 .. $#$merged); >> + my @group_end =3D (0 .. $#$merged); >> + >> + my $closed_id_count =3D 0; >> + [snip]