[pve-devel] [PATCH installer v4 0/6] fix #4829: wire up `arc_max` ZFS option to GUI/TUI for PVE

[pve-devel] [PATCH installer v4 0/6] fix #4829: wire up `arc_max` ZFS option to GUI/TUI for PVE

[Christoph Heiss]

The installer `arc_max` option ZFS was introduced in [0], this exposes
this option in the GUI/TUI installer for PVE installations.

This can be adjusted when creating a ZFS RAID under "Advanced Options".
The default value is choosen as 10% of system memory, clamped to between
64 MiB as lower limit and 16 GiB as upper limit. For PBS and PMG, the
option is (currently) hidden.

Tested by installing PVE, PBS and PMG, using both the GUI and TUI
installer. For PVE, checked that the `zfs` module option gets correctly
written & applied, the latter by looking at the output of `arc_summary`.
For PBS and PMG, verified that no modprobe options file is created and
the ARC size is set to default.

[0] https://lists.proxmox.com/pipermail/pve-devel/2023-October/059731.html

v1: https://lists.proxmox.com/pipermail/pve-devel/2023-August/058830.html
v2: https://lists.proxmox.com/pipermail/pve-devel/2023-October/059606.html
v3: https://lists.proxmox.com/pipermail/pve-devel/2023-October/059731.html

Notable changes v1 -> v2:
  * rebased on latest master
  * fix arc_max value set in TUI not being applied correctly

Notable changes v2 -> v3:
  * rebased on latest master
  * new patch explaining query_total_memory, which is used extensively
    in this patchset
  * new patch unifying product handling a bit
  * documented all calculations better w.r.t. to their units
  * moved modprobe setup into separate sub

Notable changes v3 -> v4:
  * rebased on latest master
  * dropped already applied patches from series
  * added new patch to drop debug print
  * added new patch fixing zfs_arc_max modprobe setting

Christoph Heiss (6):
  run env: remove debug print
  install: use correct variable names in zfs_setup_module_conf()
  proxinstall: expose `arc_max` ZFS option for PVE installations
  test: add tests for zfs_arc_max calculations
  common: add ZFS `arc_max` installer setup option
  tui: bootdisk: expose `arc_max` ZFS option for PVE installations

 Makefile                                    |  3 +
 Proxmox/Install.pm                          |  4 +-
 Proxmox/Install/RunEnv.pm                   |  1 -
 debian/control                              |  1 +
 proxinstall                                 | 15 ++++
 proxmox-installer-common/src/options.rs     | 62 ++++++++++++++--
 proxmox-installer-common/src/setup.rs       |  2 +
 proxmox-tui-installer/src/main.rs           |  2 +-
 proxmox-tui-installer/src/views/bootdisk.rs | 82 ++++++++++++++-------
 proxmox-tui-installer/src/views/mod.rs      |  1 -
 test/Makefile                               | 10 +++
 test/zfs-arc-max.pl                         | 81 ++++++++++++++++++++
 12 files changed, 228 insertions(+), 36 deletions(-)
 create mode 100644 test/Makefile
 create mode 100755 test/zfs-arc-max.pl

--
2.41.0

[pve-devel] [PATCH installer v4 1/6] run env: remove debug print

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/Install/RunEnv.pm | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Proxmox/Install/RunEnv.pm b/Proxmox/Install/RunEnv.pm
index 9116397..5f68d82 100644
--- a/Proxmox/Install/RunEnv.pm
+++ b/Proxmox/Install/RunEnv.pm
@@ -319,7 +319,6 @@ sub default_zfs_arc_max {
 
     my $default_mib = get('total_memory') * $ZFS_ARC_SYSMEM_PERCENTAGE;
     my $rounded_mib = int(sprintf('%.0f', $default_mib));
-    print "total_memory:" . get('total_memory') . " mib_rounded:$rounded_mib\n";
 
     if ($rounded_mib > $ZFS_ARC_MAX_SIZE_MIB) {
 	return $ZFS_ARC_MAX_SIZE_MIB;
-- 
2.42.0

[pve-devel] [PATCH installer v4 2/6] install: use correct variable names in zfs_setup_module_conf()

[Christoph Heiss]

That's what happens when you do some last-minute variable renaming and
trust that nothing broke ..

Fixes: 42aa2fa ("fix #4829: install: add new ZFS `arc_max` setup option")
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/Install.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index 693ab15..c4b0700 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -296,8 +296,8 @@ sub get_zfs_raid_setup {
 my sub zfs_setup_module_conf {
     my ($targetdir) = @_;
 
-    my $arc_max = Proxmox::Install::Config::get_zfs_opt('arc_max');
-    my $arc_max_mib = Proxmox::Install::RunEnv::clamp_zfs_arc_max($arc_max) * 1024 * 1024;
+    my $arc_max_mib = Proxmox::Install::Config::get_zfs_opt('arc_max');
+    my $arc_max = Proxmox::Install::RunEnv::clamp_zfs_arc_max($arc_max_mib) * 1024 * 1024;
 
     if ($arc_max > 0) {
 	file_write_all("$targetdir/etc/modprobe.d/zfs.conf", "options zfs zfs_arc_max=$arc_max\n")
-- 
2.42.0

[pve-devel] [PATCH installer v4 3/6] proxinstall: expose `arc_max` ZFS option for PVE installations

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxinstall | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/proxinstall b/proxinstall
index 113bf37..857281d 100755
--- a/proxinstall
+++ b/proxinstall
@@ -1162,6 +1162,21 @@ 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 $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;
+    }
+
     push @$labeled_widgets, "hdsize", $hdsize_btn;
     return $create_label_widget_grid->($labeled_widgets);;
 };
-- 
2.42.0

[pve-devel] [PATCH installer v4 4/6] test: add tests for zfs_arc_max calculations

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Makefile            |  3 ++
 debian/control      |  1 +
 test/Makefile       | 10 ++++++
 test/zfs-arc-max.pl | 81 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 95 insertions(+)
 create mode 100644 test/Makefile
 create mode 100755 test/zfs-arc-max.pl

diff --git a/Makefile b/Makefile
index e792e0e..5e94c29 100644
--- a/Makefile
+++ b/Makefile
@@ -45,6 +45,7 @@ $(BUILDDIR):
 	  proxmox-tui-installer/ \
 	  proxmox-installer-common/ \
 	  spice-vdagent.sh \
+	  test/ \
 	  unconfigured.sh \
 	  xinitrc \
 	  $@.tmp
@@ -76,7 +77,9 @@ $(DSC): $(BUILDDIR)
 sbuild: $(DSC)
 	sbuild $(DSC)
 
+.PHONY: test
 test:
+	$(MAKE) -C test check
 	$(CARGO) test --workspace $(CARGO_BUILD_ARGS)
 
 DESTDIR=
diff --git a/debian/control b/debian/control
index 3d13019..d77b12a 100644
--- a/debian/control
+++ b/debian/control
@@ -11,6 +11,7 @@ Build-Depends: cargo:native,
                librust-regex-1+default-dev (>= 1.7~~),
                librust-serde-1+default-dev,
                librust-serde-json-1+default-dev,
+               libtest-mockmodule-perl,
                perl,
                rustc:native,
 Standards-Version: 4.5.1
diff --git a/test/Makefile b/test/Makefile
new file mode 100644
index 0000000..fb80fc4
--- /dev/null
+++ b/test/Makefile
@@ -0,0 +1,10 @@
+all:
+
+export PERLLIB=..
+
+.PHONY: check
+check: test-zfs-arc-max
+
+.PHONY: test-zfs-arc-max
+test-zfs-arc-max:
+	./zfs-arc-max.pl
diff --git a/test/zfs-arc-max.pl b/test/zfs-arc-max.pl
new file mode 100755
index 0000000..74cb9b5
--- /dev/null
+++ b/test/zfs-arc-max.pl
@@ -0,0 +1,81 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use Test::More;
+use Test::MockModule qw(strict);
+
+my $proxmox_install_runenv = Test::MockModule->new('Proxmox::Install::RunEnv');
+my $proxmox_install_isoenv = Test::MockModule->new('Proxmox::Install::ISOEnv');
+
+sub mock_product {
+    my ($product) = @_;
+
+    $proxmox_install_isoenv->redefine(
+	get => sub {
+	    my ($k) = @_;
+	    return $product if $k eq 'product';
+	    die "iso environment key $k not mocked!\n";
+	},
+    );
+}
+
+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
+);
+
+while (my ($total_mem, $expected) = each %default_tests) {
+    $proxmox_install_runenv->redefine(
+	get => sub {
+	    my ($k) = @_;
+	    return $total_mem if $k eq 'total_memory';
+	    die "runtime environment key $k not mocked!\n";
+	},
+    );
+
+    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");
+
+    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");
+
+    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");
+}
+
+my @clamp_tests = (
+    # input, total system memory, expected
+    [ 0, 4 * 1024, 0 ],
+    [ 16, 4 * 1024, 64 ],
+    [ 4 * 1024, 4 * 1024, 4 * 1024 ],
+    [ 4 * 1024, 6 * 1024, 4 * 1024 ],
+    [ 8 * 1024, 4 * 1024, 4 * 1024 ],
+);
+
+mock_product('pve');
+foreach (@clamp_tests) {
+    my ($input, $total_mem, $expected) = @$_;
+
+    $proxmox_install_runenv->redefine(
+	get => sub {
+	    my ($k) = @_;
+	    return $total_mem if $k eq 'total_memory';
+	    die "runtime environment key $k not mocked!\n";
+	},
+    );
+
+    is(Proxmox::Install::RunEnv::clamp_zfs_arc_max($input), $expected,
+	"$input MiB zfs_arc_max should be clamped to $expected MiB with $total_mem MiB system memory");
+}
+
+done_testing();
-- 
2.42.0

[pve-devel] [PATCH installer v4 5/6] common: add ZFS `arc_max` installer setup option

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-installer-common/src/options.rs     | 62 +++++++++++++++++++--
 proxmox-installer-common/src/setup.rs       |  2 +
 proxmox-tui-installer/src/views/bootdisk.rs | 18 ++++--
 3 files changed, 71 insertions(+), 11 deletions(-)

diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index 185be2e..afd12cd 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -1,7 +1,9 @@
 use std::net::{IpAddr, Ipv4Addr};
 use std::{cmp, fmt};
 
-use crate::setup::{LocaleInfo, NetworkInfo, RuntimeInfo, SetupInfo};
+use crate::setup::{
+    LocaleInfo, NetworkInfo, ProductConfig, ProxmoxProduct, RuntimeInfo, SetupInfo,
+};
 use crate::utils::{CidrAddress, Fqdn};
 
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
@@ -172,25 +174,48 @@ pub struct ZfsBootdiskOptions {
     pub compress: ZfsCompressOption,
     pub checksum: ZfsChecksumOption,
     pub copies: usize,
+    pub arc_max: usize,
     pub disk_size: f64,
     pub selected_disks: Vec<usize>,
 }
 
 impl ZfsBootdiskOptions {
-    /// This panics if the provided slice is empty.
-    pub fn defaults_from(disks: &[Disk]) -> Self {
-        let disk = &disks[0];
+    /// Panics if the disk list is empty.
+    pub fn defaults_from(runinfo: &RuntimeInfo, product_conf: &ProductConfig) -> Self {
+        let disk = &runinfo.disks[0];
         Self {
             ashift: 12,
             compress: ZfsCompressOption::default(),
             checksum: ZfsChecksumOption::default(),
             copies: 1,
+            arc_max: default_zfs_arc_max(product_conf.product, runinfo.total_memory),
             disk_size: disk.size,
-            selected_disks: (0..disks.len()).collect(),
+            selected_disks: (0..runinfo.disks.len()).collect(),
         }
     }
 }
 
+/// Calculates the default upper limit for the ZFS ARC size.
+/// See also <https://bugzilla.proxmox.com/show_bug.cgi?id=4829> and
+/// https://openzfs.github.io/openzfs-docs/Performance%20and%20Tuning/Module%20Parameters.html#zfs-arc-max
+///
+/// # Arguments
+/// * `product` - The product to be installed
+/// * `total_memory` - Total memory installed in the system, in MiB
+///
+/// # 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
+    } else {
+        ((total_memory as f64) / 10.)
+            .round()
+            .clamp(64., 16. * 1024.) as usize
+    }
+}
+
 #[derive(Clone, Debug)]
 pub enum AdvancedBootdiskOptions {
     Lvm(LvmBootdiskOptions),
@@ -385,3 +410,30 @@ impl NetworkOptions {
         })
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[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
+        ];
+
+        for (total_memory, expected) in TESTS {
+            assert_eq!(
+                default_zfs_arc_max(ProxmoxProduct::PVE, *total_memory),
+                *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-installer-common/src/setup.rs b/proxmox-installer-common/src/setup.rs
index 42a9e68..28a58f3 100644
--- a/proxmox-installer-common/src/setup.rs
+++ b/proxmox-installer-common/src/setup.rs
@@ -149,6 +149,7 @@ pub struct InstallZfsOption {
     #[serde(serialize_with = "serialize_as_display")]
     checksum: ZfsChecksumOption,
     copies: usize,
+    arc_max: usize,
 }
 
 impl From<ZfsBootdiskOptions> for InstallZfsOption {
@@ -158,6 +159,7 @@ impl From<ZfsBootdiskOptions> for InstallZfsOption {
             compress: opts.compress,
             checksum: opts.checksum,
             copies: opts.copies,
+            arc_max: opts.arc_max,
         }
     }
 }
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 48ccee9..3b2242a 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -22,7 +22,7 @@ use proxmox_installer_common::{
         AdvancedBootdiskOptions, BootdiskOptions, BtrfsBootdiskOptions, Disk, FsType,
         LvmBootdiskOptions, ZfsBootdiskOptions, ZFS_CHECKSUM_OPTIONS, ZFS_COMPRESS_OPTIONS,
     },
-    setup::{BootType, ProductConfig, ProxmoxProduct},
+    setup::{BootType, ProductConfig, ProxmoxProduct, RuntimeInfo},
 };
 
 pub struct BootdiskOptionsView {
@@ -155,6 +155,7 @@ impl AdvancedBootdiskOptionsView {
 
     fn fstype_on_submit(siv: &mut Cursive, disks: &[Disk], fstype: &FsType) {
         let state = siv.user_data::<InstallerState>().unwrap();
+        let runinfo = state.runtime_info.clone();
         let product_conf = state.setup_info.config.clone();
 
         siv.call_on_name("advanced-bootdisk-options-dialog", |view: &mut Dialog| {
@@ -166,9 +167,10 @@ impl AdvancedBootdiskOptionsView {
                     FsType::Ext4 | FsType::Xfs => view.add_child(
                         LvmBootdiskOptionsView::new_with_defaults(&disks[0], &product_conf),
                     ),
-                    FsType::Zfs(_) => {
-                        view.add_child(ZfsBootdiskOptionsView::new_with_defaults(disks))
-                    }
+                    FsType::Zfs(_) => view.add_child(ZfsBootdiskOptionsView::new_with_defaults(
+                        &runinfo,
+                        &product_conf,
+                    )),
                     FsType::Btrfs(_) => {
                         view.add_child(BtrfsBootdiskOptionsView::new_with_defaults(disks))
                     }
@@ -552,8 +554,11 @@ impl ZfsBootdiskOptionsView {
         Self { view }
     }
 
-    fn new_with_defaults(disks: &[Disk]) -> Self {
-        Self::new(disks, &ZfsBootdiskOptions::defaults_from(disks))
+    fn new_with_defaults(runinfo: &RuntimeInfo, product_conf: &ProductConfig) -> Self {
+        Self::new(
+            &runinfo.disks,
+            &ZfsBootdiskOptions::defaults_from(runinfo, product_conf),
+        )
     }
 
     fn get_values(&mut self) -> Option<(Vec<Disk>, ZfsBootdiskOptions)> {
@@ -573,6 +578,7 @@ impl ZfsBootdiskOptionsView {
                 compress,
                 checksum,
                 copies,
+                arc_max: 0, // use built-in ZFS default value
                 disk_size,
                 selected_disks,
             },
-- 
2.42.0

[pve-devel] [PATCH installer v4 6/6] tui: bootdisk: expose `arc_max` ZFS option for PVE installations

[Christoph Heiss]

To set the maximum value for arc_max accordingly, simply pass down
`RuntimeInfo` directly instead of the disks array to the views.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-tui-installer/src/main.rs           |  2 +-
 proxmox-tui-installer/src/views/bootdisk.rs | 68 ++++++++++++++-------
 proxmox-tui-installer/src/views/mod.rs      |  1 -
 3 files changed, 47 insertions(+), 24 deletions(-)

diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
index 2e5a194..e561d57 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -378,7 +378,7 @@ fn bootdisk_dialog(siv: &mut Cursive) -> InstallerView {
 
     InstallerView::new(
         &state,
-        BootdiskOptionsView::new(siv, &state.runtime_info.disks, &state.options.bootdisk)
+        BootdiskOptionsView::new(siv, &state.runtime_info, &state.options.bootdisk)
             .with_name("bootdisk-options"),
         Box::new(|siv| {
             let options = siv.call_on_name("bootdisk-options", BootdiskOptionsView::get_values);
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 3b2242a..5272258 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -25,6 +25,10 @@ use proxmox_installer_common::{
     setup::{BootType, ProductConfig, ProxmoxProduct, RuntimeInfo},
 };
 
+/// OpenZFS specifies 64 MiB as the absolute minimum:
+/// https://openzfs.github.io/openzfs-docs/Performance%20and%20Tuning/Module%20Parameters.html#zfs-arc-max
+const ZFS_ARC_MIN_SIZE_MIB: usize = 64; // MiB
+
 pub struct BootdiskOptionsView {
     view: LinearLayout,
     advanced_options: Rc<RefCell<BootdiskOptions>>,
@@ -32,13 +36,13 @@ pub struct BootdiskOptionsView {
 }
 
 impl BootdiskOptionsView {
-    pub fn new(siv: &mut Cursive, disks: &[Disk], options: &BootdiskOptions) -> Self {
+    pub fn new(siv: &mut Cursive, runinfo: &RuntimeInfo, options: &BootdiskOptions) -> Self {
         let bootdisk_form = FormView::new()
             .child(
                 "Target harddisk",
                 SelectView::new()
                     .popup()
-                    .with_all(disks.iter().map(|d| (d.to_string(), d.clone()))),
+                    .with_all(runinfo.disks.iter().map(|d| (d.to_string(), d.clone()))),
             )
             .with_name("bootdisk-options-target-disk");
 
@@ -52,11 +56,11 @@ impl BootdiskOptionsView {
         let advanced_button = LinearLayout::horizontal()
             .child(DummyView.full_width())
             .child(Button::new("Advanced options", {
-                let disks = disks.to_owned();
+                let runinfo = runinfo.clone();
                 let options = advanced_options.clone();
                 move |siv| {
                     siv.add_layer(advanced_options_view(
-                        &disks,
+                        &runinfo,
                         options.clone(),
                         product_conf.clone(),
                     ));
@@ -109,7 +113,7 @@ struct AdvancedBootdiskOptionsView {
 }
 
 impl AdvancedBootdiskOptionsView {
-    fn new(disks: &[Disk], options: &BootdiskOptions, product_conf: ProductConfig) -> Self {
+    fn new(runinfo: &RuntimeInfo, options: &BootdiskOptions, product_conf: ProductConfig) -> Self {
         let filter_btrfs =
             |fstype: &&FsType| -> bool { product_conf.enable_btrfs || !fstype.is_btrfs() };
 
@@ -128,10 +132,7 @@ impl AdvancedBootdiskOptionsView {
                     .position(|t| *t == options.fstype)
                     .unwrap_or_default(),
             )
-            .on_submit({
-                let disks = disks.to_owned();
-                move |siv, fstype| Self::fstype_on_submit(siv, &disks, fstype)
-            });
+            .on_submit(Self::fstype_on_submit);
 
         let mut view = LinearLayout::vertical()
             .child(DummyView.full_width())
@@ -143,17 +144,17 @@ impl AdvancedBootdiskOptionsView {
                 view.add_child(LvmBootdiskOptionsView::new(lvm, &product_conf))
             }
             AdvancedBootdiskOptions::Zfs(zfs) => {
-                view.add_child(ZfsBootdiskOptionsView::new(disks, zfs))
+                view.add_child(ZfsBootdiskOptionsView::new(runinfo, zfs, &product_conf))
             }
             AdvancedBootdiskOptions::Btrfs(btrfs) => {
-                view.add_child(BtrfsBootdiskOptionsView::new(disks, btrfs))
+                view.add_child(BtrfsBootdiskOptionsView::new(&runinfo.disks, btrfs))
             }
         };
 
         Self { view }
     }
 
-    fn fstype_on_submit(siv: &mut Cursive, disks: &[Disk], fstype: &FsType) {
+    fn fstype_on_submit(siv: &mut Cursive, fstype: &FsType) {
         let state = siv.user_data::<InstallerState>().unwrap();
         let runinfo = state.runtime_info.clone();
         let product_conf = state.setup_info.config.clone();
@@ -165,14 +166,14 @@ impl AdvancedBootdiskOptionsView {
                 view.remove_child(3);
                 match fstype {
                     FsType::Ext4 | FsType::Xfs => view.add_child(
-                        LvmBootdiskOptionsView::new_with_defaults(&disks[0], &product_conf),
+                        LvmBootdiskOptionsView::new_with_defaults(&runinfo.disks[0], &product_conf),
                     ),
                     FsType::Zfs(_) => view.add_child(ZfsBootdiskOptionsView::new_with_defaults(
                         &runinfo,
                         &product_conf,
                     )),
                     FsType::Btrfs(_) => {
-                        view.add_child(BtrfsBootdiskOptionsView::new_with_defaults(disks))
+                        view.add_child(BtrfsBootdiskOptionsView::new_with_defaults(&runinfo.disks))
                     }
                 }
             }
@@ -186,7 +187,7 @@ impl AdvancedBootdiskOptionsView {
                         0,
                         SelectView::new()
                             .popup()
-                            .with_all(disks.iter().map(|d| (d.to_string(), d.clone()))),
+                            .with_all(runinfo.disks.iter().map(|d| (d.to_string(), d.clone()))),
                     );
                 }
                 other => view.replace_child(0, TextView::new(other.to_string())),
@@ -516,7 +517,13 @@ struct ZfsBootdiskOptionsView {
 
 impl ZfsBootdiskOptionsView {
     // TODO: Re-apply previous disk selection from `options` correctly
-    fn new(disks: &[Disk], options: &ZfsBootdiskOptions) -> Self {
+    fn new(
+        runinfo: &RuntimeInfo,
+        options: &ZfsBootdiskOptions,
+        product_conf: &ProductConfig,
+    ) -> Self {
+        let is_pve = product_conf.product == ProxmoxProduct::PVE;
+
         let inner = FormView::new()
             .child("ashift", IntegerEditView::new().content(options.ashift))
             .child(
@@ -544,9 +551,16 @@ impl ZfsBootdiskOptionsView {
                     ),
             )
             .child("copies", IntegerEditView::new().content(options.copies))
+            .child_conditional(
+                is_pve,
+                "ARC max size",
+                IntegerEditView::new_with_suffix("MiB")
+                    .max_value(runinfo.total_memory)
+                    .content(options.arc_max),
+            )
             .child("hdsize", DiskSizeEditView::new().content(options.disk_size));
 
-        let view = MultiDiskOptionsView::new(disks, &options.selected_disks, inner)
+        let view = MultiDiskOptionsView::new(&runinfo.disks, &options.selected_disks, inner)
             .top_panel(TextView::new(
                 "ZFS is not compatible with hardware RAID controllers, for details see the documentation."
             ).center());
@@ -556,20 +570,30 @@ impl ZfsBootdiskOptionsView {
 
     fn new_with_defaults(runinfo: &RuntimeInfo, product_conf: &ProductConfig) -> Self {
         Self::new(
-            &runinfo.disks,
+            runinfo,
             &ZfsBootdiskOptions::defaults_from(runinfo, product_conf),
+            product_conf,
         )
     }
 
     fn get_values(&mut self) -> Option<(Vec<Disk>, ZfsBootdiskOptions)> {
         let (disks, selected_disks) = self.view.get_disks_and_selection()?;
         let view = self.view.inner_mut()?;
+        let has_arc_max = view.len() >= 6;
+        let disk_size_index = if has_arc_max { 5 } else { 4 };
 
         let ashift = view.get_value::<IntegerEditView, _>(0)?;
         let compress = view.get_value::<SelectView<_>, _>(1)?;
         let checksum = view.get_value::<SelectView<_>, _>(2)?;
         let copies = view.get_value::<IntegerEditView, _>(3)?;
-        let disk_size = view.get_value::<DiskSizeEditView, _>(4)?;
+        let disk_size = view.get_value::<DiskSizeEditView, _>(disk_size_index)?;
+
+        let arc_max = if has_arc_max {
+            view.get_value::<IntegerEditView, _>(4)?
+                .max(ZFS_ARC_MIN_SIZE_MIB)
+        } else {
+            0 // use built-in ZFS default value
+        };
 
         Some((
             disks,
@@ -578,7 +602,7 @@ impl ZfsBootdiskOptionsView {
                 compress,
                 checksum,
                 copies,
-                arc_max: 0, // use built-in ZFS default value
+                arc_max,
                 disk_size,
                 selected_disks,
             },
@@ -591,12 +615,12 @@ impl ViewWrapper for ZfsBootdiskOptionsView {
 }
 
 fn advanced_options_view(
-    disks: &[Disk],
+    runinfo: &RuntimeInfo,
     options: Rc<RefCell<BootdiskOptions>>,
     product_conf: ProductConfig,
 ) -> impl View {
     Dialog::around(AdvancedBootdiskOptionsView::new(
-        disks,
+        runinfo,
         &(*options).borrow(),
         product_conf,
     ))
diff --git a/proxmox-tui-installer/src/views/mod.rs b/proxmox-tui-installer/src/views/mod.rs
index 1777c09..4d27532 100644
--- a/proxmox-tui-installer/src/views/mod.rs
+++ b/proxmox-tui-installer/src/views/mod.rs
@@ -43,7 +43,6 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
     ///
     /// # Arguments
     /// * `suffix` - Content for the label to the right of it.
-    #[allow(unused)]
     pub fn new_with_suffix(suffix: &str) -> Self {
         let view = LinearLayout::horizontal()
             .child(EditView::new().content("0").full_width())
-- 
2.42.0

[pve-devel] applied: [PATCH installer v4 0/6] fix #4829: wire up `arc_max` ZFS option to GUI/TUI for PVE

[Thomas Lamprecht]

Am 07/11/2023 um 13:20 schrieb Christoph Heiss:
>   run env: remove debug print
>   install: use correct variable names in zfs_setup_module_conf()
>   proxinstall: expose `arc_max` ZFS option for PVE installations
>   test: add tests for zfs_arc_max calculations
>   common: add ZFS `arc_max` installer setup option
>   tui: bootdisk: expose `arc_max` ZFS option for PVE installations
> 

applied series, thanks!

Two thoughts though:

- we could add the ARC input field for PBS and PMG too, but not use the
  automatic-sizing heuristic there (i.e., still 50% memory by default)

- maybe GB with float would fit in better, but surely not perfect either,
  so just for the record.

Re: [pve-devel] applied: [PATCH installer v4 0/6] fix #4829: wire up `arc_max` ZFS option to GUI/TUI for PVE

[Christoph Heiss]

On Tue, Nov 07, 2023 at 04:50:31PM +0100, Thomas Lamprecht wrote:
>
> Am 07/11/2023 um 13:20 schrieb Christoph Heiss:
> >   run env: remove debug print
> >   install: use correct variable names in zfs_setup_module_conf()
> >   proxinstall: expose `arc_max` ZFS option for PVE installations
> >   test: add tests for zfs_arc_max calculations
> >   common: add ZFS `arc_max` installer setup option
> >   tui: bootdisk: expose `arc_max` ZFS option for PVE installations
> >
>
> applied series, thanks!

Thanks!

>
> Two thoughts though:
>
> - we could add the ARC input field for PBS and PMG too, but not use the
>   automatic-sizing heuristic there (i.e., still 50% memory by default)

Makes sense, esp. for PBS due to its nature. I'll look into it sometime!

>
> - maybe GB with float would fit in better, but surely not perfect either,
>   so just for the record.

Good point, but I agree, hard to judge what's better. As we are
defaulting to max. 16 GiB (for PVE), it's a rather smaller range, so MiB
makes sense IMO.
Although aligning it with all the other inputs, which are in GiB, makes
sense too. As well as inputting it as GiB float should be precise enough
for any case.
I will definitely keep it in mind for the future.