[pve-devel] [PATCH installer 0/6] fix #4829: set up a lower default limit for the ZFS ARC in the installer

[pve-devel] [PATCH installer 0/6] fix #4829: set up a lower default limit for the ZFS ARC in the installer

[Christoph Heiss]

Fixes #4829. Introduces a new ZFS install option `arc_max` (aptly named
after the corresponding module option `zfs_arc_max`).

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.

If the option is set to a non-zero value, a new file
/etc/modprobe.d/zfs.conf gets written during install, setting the
`zfs_arc_max` module option as appropriate.

Tested by installing PVE, PBS and PMG. 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.

Christoph Heiss (6):
  fix #4829: install: add new ZFS `arc_max` setup option
  fix #4829: proxinstall: expose new `arc_max` ZFS option for PVE
    installations
  fix #4829: test: add tests for new zfs_arc_max calculations
  tui: views: add optional suffix label for `NumericEditView`s
  fix #4829: tui: setup: add new ZFS `arc_max` option
  fix #4829: tui: bootdisk: expose new `arc_max` ZFS option for PVE
    installations

 Makefile                                    |   3 +
 Proxmox/Install.pm                          |   4 +
 Proxmox/Install/Config.pm                   |   1 +
 Proxmox/Install/RunEnv.pm                   |  38 +++++++
 debian/control                              |   1 +
 proxinstall                                 |  15 +++
 proxmox-tui-installer/src/main.rs           |   2 +-
 proxmox-tui-installer/src/options.rs        |  55 ++++++++++-
 proxmox-tui-installer/src/setup.rs          |   2 +
 proxmox-tui-installer/src/views/bootdisk.rs |  59 ++++++++---
 proxmox-tui-installer/src/views/mod.rs      | 104 +++++++++++++++-----
 test/Makefile                               |  10 ++
 test/zfs-arc-max.pl                         |  81 +++++++++++++++
 13 files changed, 333 insertions(+), 42 deletions(-)
 create mode 100644 test/Makefile
 create mode 100755 test/zfs-arc-max.pl

--
2.41.0

[pve-devel] [PATCH installer 1/6] fix #4829: install: add new ZFS `arc_max` setup option

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/Install.pm        |  4 ++++
 Proxmox/Install/Config.pm |  1 +
 Proxmox/Install/RunEnv.pm | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+)

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index 1c4811d..d23a04b 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -1137,6 +1137,10 @@ _EOD
 
 	    file_write_all("$targetdir/etc/kernel/cmdline", "root=ZFS=$zfs_pool_name/ROOT/$zfs_root_volume_name boot=zfs\n");
 
+	    my $arc_max = Proxmox::Install::RunEnv::clamp_zfs_arc_max(
+		Proxmox::Install::Config::get_zfs_opt('arc_max')) * 1024 * 1024;
+	    file_write_all("$targetdir/etc/modprobe.d/zfs.conf", "options zfs zfs_arc_max=$arc_max\n")
+		if $arc_max > 0;
 	}
 
 	diversion_remove($targetdir, "/usr/sbin/update-grub");
diff --git a/Proxmox/Install/Config.pm b/Proxmox/Install/Config.pm
index 024f62a..f12ae67 100644
--- a/Proxmox/Install/Config.pm
+++ b/Proxmox/Install/Config.pm
@@ -72,6 +72,7 @@ my sub init_cfg {
 	    compress => 'on',
 	    checksum => 'on',
 	    copies => 1,
+	    arc_max => Proxmox::Install::RunEnv::default_zfs_arc_max(),
 	},
 	# TODO: single disk selection config
 	target_hd => undef,
diff --git a/Proxmox/Install/RunEnv.pm b/Proxmox/Install/RunEnv.pm
index 9878f55..fded45a 100644
--- a/Proxmox/Install/RunEnv.pm
+++ b/Proxmox/Install/RunEnv.pm
@@ -296,6 +296,44 @@ sub query_installation_environment : prototype() {
     return $output;
 }
 
+our $ZFS_ARC_MIN_SIZE = 64; # 64 MiB
+
+# 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
+sub default_zfs_arc_max {
+    # Use ZFS default on non-PVE
+    return 0 if Proxmox::Install::ISOEnv::get('product') ne 'pve';
+
+    my $max = 16 * 1024; # 16 GiB
+
+    my $rounded = int(sprintf('%.0f', get('total_memory') / 10));
+    if ($rounded > $max) {
+	return $max;
+    } elsif ($rounded < $ZFS_ARC_MIN_SIZE) {
+	return $ZFS_ARC_MIN_SIZE;
+    }
+
+    return $rounded;
+}
+
+# Clamps the provided ZFS arc_max value to the accepted bounds. See also
+# https://openzfs.github.io/openzfs-docs/Performance%20and%20Tuning/Module%20Parameters.html#zfs-arc-max
+sub clamp_zfs_arc_max {
+    my ($value) = @_;
+
+    return $value if $value == 0;
+
+    my $total_mem = get('total_memory');
+    if ($value > $total_mem) {
+	return $total_mem;
+    } elsif ($value < $ZFS_ARC_MIN_SIZE) {
+	return $ZFS_ARC_MIN_SIZE;
+    }
+
+    return $value;
+}
+
 my $_env = undef;
 sub get {
     my ($k) = @_;
-- 
2.41.0

[pve-devel] [PATCH installer 2/6] fix #4829: proxinstall: expose new `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 d5b2565..76aec17 100755
--- a/proxinstall
+++ b/proxinstall
@@ -1144,6 +1144,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, $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.41.0

[pve-devel] [PATCH installer 3/6] fix #4829: test: add tests for new 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 dc180b2..979897d 100644
--- a/Makefile
+++ b/Makefile
@@ -44,6 +44,7 @@ $(BUILDDIR):
 	  proxmox-low-level-installer \
 	  proxmox-tui-installer/ \
 	  spice-vdagent.sh \
+	  test/ \
 	  unconfigured.sh \
 	  xinitrc \
 	  $@.tmp
@@ -75,7 +76,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.41.0

[pve-devel] [PATCH installer 4/6] fix #4829: tui: views: add optional suffix label for `NumericEditView`s

[Christoph Heiss]

Most of the churn here is due to changing the inner view from an
`EditView` to a `LinearLayout`. Also prompted the introduction of two
small helpers .inner() and .inner_mut() to simplify things everywhere
else in the view.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
This is also a precursory change to completely replace the
`DiskSizeEditView` component with just the `{Integer,Float}EditView` as
appropriate, the former essentially duplicating most of the latter
views.

 proxmox-tui-installer/src/views/mod.rs | 104 +++++++++++++++++++------
 1 file changed, 82 insertions(+), 22 deletions(-)

diff --git a/proxmox-tui-installer/src/views/mod.rs b/proxmox-tui-installer/src/views/mod.rs
index aa24fa4..e997968 100644
--- a/proxmox-tui-installer/src/views/mod.rs
+++ b/proxmox-tui-installer/src/views/mod.rs
@@ -19,16 +19,37 @@ mod timezone;
 pub use timezone::*;

 pub struct NumericEditView<T> {
-    view: EditView,
+    view: LinearLayout,
     max_value: Option<T>,
     max_content_width: Option<usize>,
     allow_empty: bool,
 }

 impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
+    /// Creates a new [`NumericEditView`], with the value set to `0`.
     pub fn new() -> Self {
+        let view = LinearLayout::horizontal().child(EditView::new().content("0").full_width());
+
+        Self {
+            view,
+            max_value: None,
+            max_content_width: None,
+            allow_empty: false,
+        }
+    }
+
+    /// Creates a new [`NumericEditView`], with the value set to `0` and a label to the right of it
+    /// with the given content, separated by a space.
+    ///
+    /// # Arguments
+    /// * `suffix` - Content for the label to the right of it.
+    pub fn new_with_suffix(suffix: &str) -> Self {
+        let view = LinearLayout::horizontal()
+            .child(EditView::new().content("0").full_width())
+            .child(TextView::new(format!(" {suffix}")));
+
         Self {
-            view: EditView::new().content("0"),
+            view,
             max_value: None,
             max_content_width: None,
             allow_empty: false,
@@ -42,7 +63,7 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {

     pub fn max_content_width(mut self, width: usize) -> Self {
         self.max_content_width = Some(width);
-        self.view.set_max_content_width(self.max_content_width);
+        self.inner_mut().set_max_content_width(Some(width));
         self
     }

@@ -50,24 +71,25 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
         self.allow_empty = value;

         if value {
-            self.view = EditView::new();
+            *self.inner_mut() = EditView::new();
         } else {
-            self.view = EditView::new().content("0");
+            *self.inner_mut() = EditView::new().content("0");
         }

-        self.view.set_max_content_width(self.max_content_width);
+        let max_content_width = self.max_content_width;
+        self.inner_mut().set_max_content_width(max_content_width);
         self
     }

     pub fn get_content(&self) -> Result<T, <T as FromStr>::Err> {
         assert!(!self.allow_empty);
-        self.view.get_content().parse()
+        self.inner().get_content().parse()
     }

     pub fn get_content_maybe(&self) -> Option<Result<T, <T as FromStr>::Err>> {
-        let content = self.view.get_content();
+        let content = self.inner().get_content();
         if !content.is_empty() {
-            Some(self.view.get_content().parse())
+            Some(self.inner().get_content().parse())
         } else {
             None
         }
@@ -83,7 +105,7 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
             if let Ok(val) = self.get_content() {
                 if result.is_consumed() && val > max {
                     // Restore the original value, before the insert
-                    let cb = self.view.set_content((*original).clone());
+                    let cb = self.inner_mut().set_content((*original).clone());
                     return EventResult::with_cb_once(move |siv| {
                         result.process(siv);
                         cb(siv);
@@ -94,16 +116,54 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {

         result
     }
+
+    /// Provides an immutable reference to the inner [`EditView`].
+    fn inner(&self) -> &EditView {
+        // Safety: Invariant; first child must always exist and be a `EditView`
+        self.view
+            .get_child(0)
+            .unwrap()
+            .downcast_ref::<ResizedView<EditView>>()
+            .unwrap()
+            .get_inner()
+    }
+
+    /// Provides a mutable reference to the inner [`EditView`].
+    fn inner_mut(&mut self) -> &mut EditView {
+        // Safety: Invariant; first child must always exist and be a `EditView`
+        self.view
+            .get_child_mut(0)
+            .unwrap()
+            .downcast_mut::<ResizedView<EditView>>()
+            .unwrap()
+            .get_inner_mut()
+    }
+
+    /// Sets the content of the inner [`EditView`]. This correctly swaps out the content without
+    /// modifying the [`EditView`] in any way.
+    ///
+    /// Chainable variant.
+    ///
+    /// # Arguments
+    /// * `content` - New, stringified content for the inner [`EditView`]. Must be a valid value
+    /// according to the containet type `T`.
+    fn content_inner(mut self, content: &str) -> Self {
+        let mut inner = EditView::new();
+        std::mem::swap(self.inner_mut(), &mut inner);
+        inner = inner.content(content);
+        std::mem::swap(self.inner_mut(), &mut inner);
+        self
+    }
 }

 pub type FloatEditView = NumericEditView<f64>;
 pub type IntegerEditView = NumericEditView<usize>;

 impl ViewWrapper for FloatEditView {
-    cursive::wrap_impl!(self.view: EditView);
+    cursive::wrap_impl!(self.view: LinearLayout);

     fn wrap_on_event(&mut self, event: Event) -> EventResult {
-        let original = self.view.get_content();
+        let original = self.inner_mut().get_content();

         let has_decimal_place = original.find('.').is_some();

@@ -114,13 +174,13 @@ impl ViewWrapper for FloatEditView {
         };

         let decimal_places = self
-            .view
+            .inner_mut()
             .get_content()
             .split_once('.')
             .map(|(_, s)| s.len())
             .unwrap_or_default();
         if decimal_places > 2 {
-            let cb = self.view.set_content((*original).clone());
+            let cb = self.inner_mut().set_content((*original).clone());
             return EventResult::with_cb_once(move |siv| {
                 result.process(siv);
                 cb(siv);
@@ -132,17 +192,17 @@ impl ViewWrapper for FloatEditView {
 }

 impl FloatEditView {
-    pub fn content(mut self, content: f64) -> Self {
-        self.view = self.view.content(format!("{:.2}", content));
-        self
+    /// Sets the value of the [`FloatEditView`].
+    pub fn content(self, content: f64) -> Self {
+        self.content_inner(&format!("{:.2}", content))
     }
 }

 impl ViewWrapper for IntegerEditView {
-    cursive::wrap_impl!(self.view: EditView);
+    cursive::wrap_impl!(self.view: LinearLayout);

     fn wrap_on_event(&mut self, event: Event) -> EventResult {
-        let original = self.view.get_content();
+        let original = self.inner_mut().get_content();

         let result = match event {
             // Drop all other characters than numbers; allow dots if not set to integer-only
@@ -155,9 +215,9 @@ impl ViewWrapper for IntegerEditView {
 }

 impl IntegerEditView {
-    pub fn content(mut self, content: usize) -> Self {
-        self.view = self.view.content(content.to_string());
-        self
+    /// Sets the value of the [`IntegerEditView`].
+    pub fn content(self, content: usize) -> Self {
+        self.content_inner(&content.to_string())
     }
 }

--
2.41.0

[pve-devel] [PATCH installer 5/6] fix #4829: tui: setup: add new ZFS `arc_max` option

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-tui-installer/src/options.rs        | 55 +++++++++++++++++++--
 proxmox-tui-installer/src/setup.rs          |  2 +
 proxmox-tui-installer/src/views/bootdisk.rs |  8 ++-
 3 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/proxmox-tui-installer/src/options.rs b/proxmox-tui-installer/src/options.rs
index dab1730..cf92f1e 100644
--- a/proxmox-tui-installer/src/options.rs
+++ b/proxmox-tui-installer/src/options.rs
@@ -1,7 +1,7 @@
 use std::net::{IpAddr, Ipv4Addr};
 use std::{cmp, fmt};
 
-use crate::setup::{LocaleInfo, NetworkInfo, RuntimeInfo};
+use crate::setup::{LocaleInfo, NetworkInfo, ProxmoxProduct, RuntimeInfo};
 use crate::utils::{CidrAddress, Fqdn};
 use crate::SummaryOption;
 
@@ -190,21 +190,23 @@ 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];
+    pub fn defaults_from(runinfo: &RuntimeInfo) -> Self {
+        let disk = &runinfo.disks[0];
         Self {
             ashift: 12,
             compress: ZfsCompressOption::default(),
             checksum: ZfsChecksumOption::default(),
             copies: 1,
+            arc_max: default_zfs_arc_max(crate::current_product(), runinfo.total_memory),
             disk_size: disk.size,
-            selected_disks: (0..disks.len()).collect(),
+            selected_disks: (0..runinfo.disks.len()).collect(),
         }
     }
 }
@@ -441,3 +443,48 @@ impl InstallerOptions {
         ]
     }
 }
+
+/// 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
+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
+    }
+}
+
+#[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-tui-installer/src/setup.rs b/proxmox-tui-installer/src/setup.rs
index e51bb4d..3f41db4 100644
--- a/proxmox-tui-installer/src/setup.rs
+++ b/proxmox-tui-installer/src/setup.rs
@@ -114,6 +114,7 @@ struct InstallZfsOption {
     #[serde(serialize_with = "serialize_as_display")]
     checksum: ZfsChecksumOption,
     copies: usize,
+    arc_max: usize,
 }
 
 impl From<ZfsBootdiskOptions> for InstallZfsOption {
@@ -123,6 +124,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 a018e71..5ea0cee 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -131,6 +131,11 @@ impl AdvancedBootdiskOptionsView {
     }
 
     fn fstype_on_submit(siv: &mut Cursive, disks: &[Disk], fstype: &FsType) {
+        let runinfo = siv
+            .user_data::<InstallerState>()
+            .map(|state| state.runtime_info.clone())
+            .unwrap();
+
         siv.call_on_name("advanced-bootdisk-options-dialog", |view: &mut Dialog| {
             if let Some(AdvancedBootdiskOptionsView { view }) =
                 view.get_content_mut().downcast_mut()
@@ -142,7 +147,7 @@ impl AdvancedBootdiskOptionsView {
                     )),
                     FsType::Zfs(_) => view.add_child(ZfsBootdiskOptionsView::new(
                         disks,
-                        &ZfsBootdiskOptions::defaults_from(disks),
+                        &ZfsBootdiskOptions::defaults_from(&runinfo),
                     )),
                     FsType::Btrfs(_) => view.add_child(BtrfsBootdiskOptionsView::new(
                         disks,
@@ -534,6 +539,7 @@ impl ZfsBootdiskOptionsView {
                 compress,
                 checksum,
                 copies,
+                arc_max: 0, // use built-in ZFS default value
                 disk_size,
                 selected_disks,
             },
-- 
2.41.0

[pve-devel] [PATCH installer 6/6] fix #4829: tui: bootdisk: expose new `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 - which
accounts for about half the churn here.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-tui-installer/src/main.rs           |  2 +-
 proxmox-tui-installer/src/views/bootdisk.rs | 53 +++++++++++++++------
 2 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
index 7bfaf9b..384a460 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -433,7 +433,7 @@ fn bootdisk_dialog(siv: &mut Cursive) -> InstallerView {

     InstallerView::new(
         &state,
-        BootdiskOptionsView::new(&state.runtime_info.disks, &state.options.bootdisk)
+        BootdiskOptionsView::new(&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 5ea0cee..b736111 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -20,19 +20,21 @@ use crate::{
 };
 use crate::{setup::ProxmoxProduct, InstallerState};

+const ZFS_ARC_MIN_SIZE: usize = 64 * 1024 * 1024; // 64 MiB
+
 pub struct BootdiskOptionsView {
     view: LinearLayout,
     advanced_options: Rc<RefCell<BootdiskOptions>>,
 }

 impl BootdiskOptionsView {
-    pub fn new(disks: &[Disk], options: &BootdiskOptions) -> Self {
+    pub fn new(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");

@@ -41,10 +43,10 @@ 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, options.clone()));
+                    siv.add_layer(advanced_options_view(&runinfo, options.clone()));
                 }
             }));

@@ -87,7 +89,7 @@ struct AdvancedBootdiskOptionsView {
 }

 impl AdvancedBootdiskOptionsView {
-    fn new(disks: &[Disk], options: &BootdiskOptions) -> Self {
+    fn new(runinfo: &RuntimeInfo, options: &BootdiskOptions) -> Self {
         let enable_btrfs = crate::setup_info().config.enable_btrfs;

         let filter_btrfs = |fstype: &&FsType| -> bool { enable_btrfs || !fstype.is_btrfs() };
@@ -108,7 +110,7 @@ impl AdvancedBootdiskOptionsView {
                     .unwrap_or_default(),
             )
             .on_submit({
-                let disks = disks.to_owned();
+                let disks = runinfo.disks.to_owned();
                 move |siv, fstype| Self::fstype_on_submit(siv, &disks, fstype)
             });

@@ -120,10 +122,10 @@ impl AdvancedBootdiskOptionsView {
         match &options.advanced {
             AdvancedBootdiskOptions::Lvm(lvm) => view.add_child(LvmBootdiskOptionsView::new(lvm)),
             AdvancedBootdiskOptions::Zfs(zfs) => {
-                view.add_child(ZfsBootdiskOptionsView::new(disks, zfs))
+                view.add_child(ZfsBootdiskOptionsView::new(runinfo, zfs))
             }
             AdvancedBootdiskOptions::Btrfs(btrfs) => {
-                view.add_child(BtrfsBootdiskOptionsView::new(disks, btrfs))
+                view.add_child(BtrfsBootdiskOptionsView::new(&runinfo.disks, btrfs))
             }
         };

@@ -146,7 +148,7 @@ impl AdvancedBootdiskOptionsView {
                         &LvmBootdiskOptions::defaults_from(&disks[0]),
                     )),
                     FsType::Zfs(_) => view.add_child(ZfsBootdiskOptionsView::new(
-                        disks,
+                        &runinfo,
                         &ZfsBootdiskOptions::defaults_from(&runinfo),
                     )),
                     FsType::Btrfs(_) => view.add_child(BtrfsBootdiskOptionsView::new(
@@ -484,7 +486,9 @@ 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) -> Self {
+        let is_pve = crate::setup_info().config.product == ProxmoxProduct::PVE;
+
         let inner = FormView::new()
             .child("ashift", IntegerEditView::new().content(options.ashift))
             .child(
@@ -512,9 +516,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());
@@ -525,12 +536,21 @@ impl ZfsBootdiskOptionsView {
     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)
+        } else {
+            0 // use built-in ZFS default value
+        };

         Some((
             disks,
@@ -539,7 +559,7 @@ impl ZfsBootdiskOptionsView {
                 compress,
                 checksum,
                 copies,
-                arc_max: 0, // use built-in ZFS default value
+                arc_max,
                 disk_size,
                 selected_disks,
             },
@@ -551,9 +571,12 @@ impl ViewWrapper for ZfsBootdiskOptionsView {
     cursive::wrap_impl!(self.view: MultiDiskOptionsView<FormView>);
 }

-fn advanced_options_view(disks: &[Disk], options: Rc<RefCell<BootdiskOptions>>) -> impl View {
+fn advanced_options_view(
+    runinfo: &RuntimeInfo,
+    options: Rc<RefCell<BootdiskOptions>>,
+) -> impl View {
     Dialog::around(AdvancedBootdiskOptionsView::new(
-        disks,
+        runinfo,
         &(*options).borrow(),
     ))
     .title("Advanced bootdisk options")
--
2.41.0