[pve-devel] [PATCH installer 0/2] expose zfs arc size setting for all products

[pve-devel] [PATCH installer 0/2] expose zfs arc size setting for all products

[Christoph Heiss]

As suggested by Thomas, sets the default to 50% for all non-PVE
products.

Quickly smoke-tested this, to see if all installer show the correct
value in the GUI/TUI.

Christoph Heiss (2):
  tui: expose arc size setting for zfs bootdisks for all products
  proxinstall: expose arc size setting for zfs bootdisks for all
    products

 Proxmox/Install/RunEnv.pm                   | 20 +++++----
 proxinstall                                 | 24 +++++------
 proxmox-installer-common/src/options.rs     | 45 ++++++++++++---------
 proxmox-tui-installer/src/views/bootdisk.rs | 19 ++++-----
 test/zfs-arc-max.pl                         | 26 ++++++------
 5 files changed, 70 insertions(+), 64 deletions(-)

--
2.42.0

[pve-devel] [PATCH installer 1/2] tui: expose arc size setting for zfs bootdisks for all products

[Christoph Heiss]

For non-PVE products, this defaults to 50% of available system memory
(aka. ZFS default).

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-installer-common/src/options.rs     | 45 ++++++++++++---------
 proxmox-tui-installer/src/views/bootdisk.rs | 19 ++++-----
 2 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index afd12cd..87931d1 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -206,14 +206,15 @@ impl ZfsBootdiskOptions {
 /// # Returns
 /// The default ZFS maximum ARC size in MiB for this system.
 fn default_zfs_arc_max(product: ProxmoxProduct, total_memory: usize) -> usize {
-    if product != ProxmoxProduct::PVE {
-        // Use ZFS default for non-PVE
-        0
+    let mem_mib = if product == ProxmoxProduct::PVE {
+        // For PVE, use 10% as default and clamp to 16 GiB max
+        ((total_memory as f64) / 10.).round().min(16. * 1024.)
     } else {
-        ((total_memory as f64) / 10.)
-            .round()
-            .clamp(64., 16. * 1024.) as usize
-    }
+        // Use 50% default for non-PVE
+        ((total_memory as f64) / 2.).round()
+    };
+
+    mem_mib.max(64.) as usize
 }
 
 #[derive(Clone, Debug)]
@@ -417,23 +418,29 @@ mod tests {
 
     #[test]
     fn zfs_arc_limit() {
-        const TESTS: &[(usize, usize)] = &[
-            (16, 64), // at least 64 MiB
-            (1024, 102),
-            (4 * 1024, 410),
-            (8 * 1024, 819),
-            (150 * 1024, 15360),
-            (160 * 1024, 16384),
-            (1024 * 1024, 16384), // maximum of 16 GiB
+        const TESTS: &[(usize, (usize, usize, usize))] = &[
+            (16, (64, 64, 64)), // at least 64 MiB for all products
+            (1024, (102, 512, 512)),
+            (4 * 1024, (410, 2048, 2048)),
+            (8 * 1024, (819, 4096, 4096)),
+            (150 * 1024, (15360, 76800, 76800)),
+            (160 * 1024, (16384, 81920, 81920)),
+            (1024 * 1024, (16384, 524288, 524288)), // maximum of 16 GiB for PVE
         ];
 
-        for (total_memory, expected) in TESTS {
+        for (total_memory, (pve_expected, pbs_expected, pmg_expected)) in TESTS {
             assert_eq!(
                 default_zfs_arc_max(ProxmoxProduct::PVE, *total_memory),
-                *expected
+                *pve_expected
+            );
+            assert_eq!(
+                default_zfs_arc_max(ProxmoxProduct::PBS, *total_memory),
+                *pbs_expected
+            );
+            assert_eq!(
+                default_zfs_arc_max(ProxmoxProduct::PMG, *total_memory),
+                *pmg_expected
             );
-            assert_eq!(default_zfs_arc_max(ProxmoxProduct::PBS, *total_memory), 0);
-            assert_eq!(default_zfs_arc_max(ProxmoxProduct::PMG, *total_memory), 0);
         }
     }
 }
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 7e13e91..7efe953 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -155,7 +155,7 @@ impl AdvancedBootdiskOptionsView {
                 &product_conf,
             )),
             AdvancedBootdiskOptions::Zfs(zfs) => {
-                view.add_child(ZfsBootdiskOptionsView::new(runinfo, zfs, &product_conf))
+                view.add_child(ZfsBootdiskOptionsView::new(runinfo, zfs))
             }
             AdvancedBootdiskOptions::Btrfs(btrfs) => {
                 view.add_child(BtrfsBootdiskOptionsView::new(&runinfo.disks, btrfs))
@@ -559,13 +559,7 @@ struct ZfsBootdiskOptionsView {
 
 impl ZfsBootdiskOptionsView {
     // TODO: Re-apply previous disk selection from `options` correctly
-    fn new(
-        runinfo: &RuntimeInfo,
-        options: &ZfsBootdiskOptions,
-        product_conf: &ProductConfig,
-    ) -> Self {
-        let is_pve = product_conf.product == ProxmoxProduct::PVE;
-
+    fn new(runinfo: &RuntimeInfo, options: &ZfsBootdiskOptions) -> Self {
         let inner = FormView::new()
             .child("ashift", IntegerEditView::new().content(options.ashift))
             .child(
@@ -592,9 +586,11 @@ impl ZfsBootdiskOptionsView {
                             .unwrap_or_default(),
                     ),
             )
-            .child("copies", IntegerEditView::new().content(options.copies).max_value(3))
-            .child_conditional(
-                is_pve,
+            .child(
+                "copies",
+                IntegerEditView::new().content(options.copies).max_value(3),
+            )
+            .child(
                 "ARC max size",
                 IntegerEditView::new_with_suffix("MiB")
                     .max_value(runinfo.total_memory)
@@ -614,7 +610,6 @@ impl ZfsBootdiskOptionsView {
         Self::new(
             runinfo,
             &ZfsBootdiskOptions::defaults_from(runinfo, product_conf),
-            product_conf,
         )
     }
 
-- 
2.42.0

[pve-devel] [PATCH installer 2/2] proxinstall: expose arc size setting for zfs bootdisks for all products

[Christoph Heiss]

For non-PVE products, this defaults to 50% of available system memory
(aka. ZFS default).

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/Install/RunEnv.pm | 20 +++++++++++++-------
 proxinstall               | 24 +++++++++++-------------
 test/zfs-arc-max.pl       | 26 +++++++++++++-------------
 3 files changed, 37 insertions(+), 33 deletions(-)

diff --git a/Proxmox/Install/RunEnv.pm b/Proxmox/Install/RunEnv.pm
index c393f67..25a8b49 100644
--- a/Proxmox/Install/RunEnv.pm
+++ b/Proxmox/Install/RunEnv.pm
@@ -310,20 +310,26 @@ sub query_installation_environment : prototype() {
 our $ZFS_ARC_MIN_SIZE_MIB = 64; # MiB
 
 # See https://bugzilla.proxmox.com/show_bug.cgi?id=4829
-our $ZFS_ARC_MAX_SIZE_MIB = 16 * 1024; # 16384 MiB = 16 GiB
-our $ZFS_ARC_SYSMEM_PERCENTAGE = 0.1; # use 10% of available system memory by default
+our $ZFS_ARC_PVE_MAX_SIZE_MIB = 16 * 1024; # 16384 MiB = 16 GiB
+our $ZFS_ARC_PVE_SYSMEM_PERCENTAGE = 0.1; # use 10% of available system memory for PVE as default
+our $ZFS_ARC_DEFAULT_SYSMEM_PERCENTAGE = 0.5; # 50% of available system memory otherwise
 
 # Calculates the default upper limit for the ZFS ARC size.
 # Returns the default ZFS maximum ARC size in MiB.
 sub default_zfs_arc_max {
-    # Use ZFS default on non-PVE
-    return 0 if Proxmox::Install::ISOEnv::get('product') ne 'pve';
+    my $percentage = $ZFS_ARC_DEFAULT_SYSMEM_PERCENTAGE;
+    my $max_mib = get('total_memory');
 
-    my $default_mib = get('total_memory') * $ZFS_ARC_SYSMEM_PERCENTAGE;
+    if (Proxmox::Install::ISOEnv::get('product') eq 'pve') {
+	$percentage = $ZFS_ARC_PVE_SYSMEM_PERCENTAGE;
+	$max_mib = $ZFS_ARC_PVE_MAX_SIZE_MIB;
+    }
+
+    my $default_mib = get('total_memory') * $percentage;
     my $rounded_mib = int(sprintf('%.0f', $default_mib));
 
-    if ($rounded_mib > $ZFS_ARC_MAX_SIZE_MIB) {
-	return $ZFS_ARC_MAX_SIZE_MIB;
+    if ($rounded_mib > $max_mib) {
+	return $max_mib;
     } elsif ($rounded_mib < $ZFS_ARC_MIN_SIZE_MIB) {
 	return $ZFS_ARC_MIN_SIZE_MIB;
     }
diff --git a/proxinstall b/proxinstall
index 01d4cfe..97e9462 100755
--- a/proxinstall
+++ b/proxinstall
@@ -1167,20 +1167,18 @@ my $create_raid_advanced_grid = sub {
     $spinbutton_copies->set_value($copies);
     push @$labeled_widgets, ['copies', $spinbutton_copies];
 
-    if ($iso_env->{product} eq 'pve') {
-	my $total_memory = Proxmox::Install::RunEnv::get('total_memory');
+    my $total_memory = Proxmox::Install::RunEnv::get('total_memory');
 
-	my $spinbutton_arc_max = Gtk3::SpinButton->new_with_range(
-	    $Proxmox::Install::RunEnv::ZFS_ARC_MIN_SIZE_MIB, $total_memory, 1);
-	$spinbutton_arc_max->set_tooltip_text('Maximum ARC size in megabytes');
-	$spinbutton_arc_max->signal_connect('value-changed' => sub {
-	    my $w = shift;
-	    Proxmox::Install::Config::set_zfs_opt('arc_max', $w->get_value_as_int());
-	});
-	my $arc_max = Proxmox::Install::Config::get_zfs_opt('arc_max');
-	$spinbutton_arc_max->set_value($arc_max);
-	push @$labeled_widgets, ['ARC max size', $spinbutton_arc_max, 'MiB'];
-    }
+    my $spinbutton_arc_max = Gtk3::SpinButton->new_with_range(
+	$Proxmox::Install::RunEnv::ZFS_ARC_MIN_SIZE_MIB, $total_memory, 1);
+    $spinbutton_arc_max->set_tooltip_text('Maximum ARC size in megabytes');
+    $spinbutton_arc_max->signal_connect('value-changed' => sub {
+	my $w = shift;
+	Proxmox::Install::Config::set_zfs_opt('arc_max', $w->get_value_as_int());
+    });
+    my $arc_max = Proxmox::Install::Config::get_zfs_opt('arc_max');
+    $spinbutton_arc_max->set_value($arc_max);
+    push @$labeled_widgets, ['ARC max size', $spinbutton_arc_max, 'MiB'];
 
     push @$labeled_widgets, ['hdsize', $hdsize_btn, 'GB'];
     return $create_label_widget_grid->($labeled_widgets);;
diff --git a/test/zfs-arc-max.pl b/test/zfs-arc-max.pl
index 74cb9b5..a554d56 100755
--- a/test/zfs-arc-max.pl
+++ b/test/zfs-arc-max.pl
@@ -22,13 +22,13 @@ sub mock_product {
 }
 
 my %default_tests = (
-    16 => 64, # at least 64 MiB
-    1024 => 102,
-    4 * 1024 => 410,
-    8 * 1024 => 819,
-    150 * 1024 => 15360,
-    160 * 1024 => 16384,
-    1024 * 1024 => 16384, # maximum of 16 GiB
+    16 => [64, 64, 64], # at least 64 MiB in any case
+    1024 => [102, 512, 512],
+    4 * 1024 => [410, 2048, 2048],
+    8 * 1024 => [819, 4096, 4096],
+    150 * 1024 => [15360, 76800, 76800],
+    160 * 1024 => [16384, 81920, 81920],
+    1024 * 1024 => [16384, 524288, 524288], # maximum of 16 GiB for PVE
 );
 
 while (my ($total_mem, $expected) = each %default_tests) {
@@ -41,16 +41,16 @@ while (my ($total_mem, $expected) = each %default_tests) {
     );
 
     mock_product('pve');
-    is(Proxmox::Install::RunEnv::default_zfs_arc_max(), $expected,
-	"$expected MiB should be zfs_arc_max for PVE with $total_mem MiB system memory");
+    is(Proxmox::Install::RunEnv::default_zfs_arc_max(), $expected->[0],
+	"$expected->[0] MiB should be zfs_arc_max for PVE with $total_mem MiB system memory");
 
     mock_product('pbs');
-    is(Proxmox::Install::RunEnv::default_zfs_arc_max(), 0,
-	"zfs_arc_max should default to `0` for PBS with $total_mem MiB system memory");
+    is(Proxmox::Install::RunEnv::default_zfs_arc_max(), $expected->[1],
+	"$expected->[1] MiB should be zfs_arc_max for PBS with $total_mem MiB system memory");
 
     mock_product('pmg');
-    is(Proxmox::Install::RunEnv::default_zfs_arc_max(), 0,
-	"zfs_arc_max should default to `0` for PMG with $total_mem MiB system memory");
+    is(Proxmox::Install::RunEnv::default_zfs_arc_max(), $expected->[2],
+	"$expected->[2] MiB should be zfs_arc_max for PMG with $total_mem MiB system memory");
 }
 
 my @clamp_tests = (
-- 
2.42.0

Re: [pve-devel] [PATCH installer 0/2] expose zfs arc size setting for all products

[Christoph Heiss]

Ping.

On Thu, Nov 30, 2023 at 11:01:41AM +0100, Christoph Heiss wrote:
>
> As suggested by Thomas, sets the default to 50% for all non-PVE
> products.
>
> Quickly smoke-tested this, to see if all installer show the correct
> value in the GUI/TUI.
>
> Christoph Heiss (2):
>   tui: expose arc size setting for zfs bootdisks for all products
>   proxinstall: expose arc size setting for zfs bootdisks for all
>     products
>
>  Proxmox/Install/RunEnv.pm                   | 20 +++++----
>  proxinstall                                 | 24 +++++------
>  proxmox-installer-common/src/options.rs     | 45 ++++++++++++---------
>  proxmox-tui-installer/src/views/bootdisk.rs | 19 ++++-----
>  test/zfs-arc-max.pl                         | 26 ++++++------
>  5 files changed, 70 insertions(+), 64 deletions(-)
>
> --
> 2.42.0
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>

Re: [pve-devel] [PATCH installer 0/2] expose zfs arc size setting for all products

[Thomas Lamprecht]

Am 30/11/2023 um 11:01 schrieb Christoph Heiss:
> As suggested by Thomas, sets the default to 50% for all non-PVE
> products.
> 
> Quickly smoke-tested this, to see if all installer show the correct
> value in the GUI/TUI.

What I'm wondering if we should skip actively setting this as module
parameter if the user did not changed the value at all?

That way it would stay dynamic, e.g., if one increased the memory of
a PBS or PMG instance, but one could also interpret that as bad thing,
especially if the MiB number was shown.
For GTK we could just go for a placeholder text showing the number,
but there isn't really such a UX-mechanic available for the TUI one
AFAICT.

Anyhow, I can be OK with always hard-coding, just wanted to know
your (or others) thoughts on this.

Re: [pve-devel] [PATCH installer 0/2] expose zfs arc size setting for all products

[Christoph Heiss]

On Tue, Feb 06, 2024 at 01:49:36PM +0100, Thomas Lamprecht wrote:
> Am 30/11/2023 um 11:01 schrieb Christoph Heiss:
> > As suggested by Thomas, sets the default to 50% for all non-PVE
> > products.
> >
> > Quickly smoke-tested this, to see if all installer show the correct
> > value in the GUI/TUI.
>
> What I'm wondering if we should skip actively setting this as module
> parameter if the user did not changed the value at all?

Sounds like a pretty reasonable "default" actually, just keeping the ZFS
defaults. So I'd be happy to implement that.

>
> That way it would stay dynamic, e.g., if one increased the memory of
> a PBS or PMG instance, but one could also interpret that as bad thing,
> especially if the MiB number was shown.

Should this behaviour only apply to the PBS/PMG installer then? Or to
PVE as well?

> For GTK we could just go for a placeholder text showing the number,
> but there isn't really such a UX-mechanic available for the TUI one
> AFAICT.

Yeah, we'd need to implement placeholders for the TUI ourselves.
Shouldn't be too hard tho, I got a rough idea mind how it be fitted in
there.

>
> Anyhow, I can be OK with always hard-coding, just wanted to know
> your (or others) thoughts on this.

Re: [pve-devel] [PATCH installer 0/2] expose zfs arc size setting for all products

[Thomas Lamprecht]

Am 06/02/2024 um 14:25 schrieb Christoph Heiss:
> On Tue, Feb 06, 2024 at 01:49:36PM +0100, Thomas Lamprecht wrote:
>> That way it would stay dynamic, e.g., if one increased the memory of
>> a PBS or PMG instance, but one could also interpret that as bad thing,
>> especially if the MiB number was shown.
> 
> Should this behaviour only apply to the PBS/PMG installer then? Or to
> PVE as well?

No, I'd keep PVE at the lower limit.

There it makes sense more sense over the default 50 % and as PVE is
more likely to be installed on bare metal, at least for permanent
production installations, it's also less likely that the amount of
memory changes.

> 
>> For GTK we could just go for a placeholder text showing the number,
>> but there isn't really such a UX-mechanic available for the TUI one
>> AFAICT.
> 
> Yeah, we'd need to implement placeholders for the TUI ourselves.
> Shouldn't be too hard tho, I got a rough idea mind how it be fitted in
> there.

As long as one can somewhat easily understand the meaning behind
that, it should be fine.