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

[pve-devel] [PATCH installer v3 0/8] fix #4829: set up lower default limit for ZFS ARC in 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, 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.

v1: https://lists.proxmox.com/pipermail/pve-devel/2023-August/058830.html
v2: https://lists.proxmox.com/pipermail/pve-devel/2023-October/059606.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

Christoph Heiss (8):
  run env: add comment for query_total_memory()
  tui: views: add optional suffix label for `NumericEditView`s
  tui: bootdisk: simplify product handling by passing the config
    directly
  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
  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                          |  14 +++
 Proxmox/Install/Config.pm                   |   1 +
 Proxmox/Install/RunEnv.pm                   |  47 ++++++++
 debian/control                              |   1 +
 proxinstall                                 |  15 +++
 proxmox-tui-installer/src/main.rs           |   2 +-
 proxmox-tui-installer/src/options.rs        |  57 +++++++++-
 proxmox-tui-installer/src/setup.rs          |   2 +
 proxmox-tui-installer/src/views/bootdisk.rs | 116 +++++++++++++-------
 proxmox-tui-installer/src/views/mod.rs      | 104 ++++++++++++++----
 test/Makefile                               |  10 ++
 test/zfs-arc-max.pl                         |  81 ++++++++++++++
 13 files changed, 386 insertions(+), 67 deletions(-)
 create mode 100644 test/Makefile
 create mode 100755 test/zfs-arc-max.pl

--
2.41.0

[pve-devel] [PATCH installer v3 1/8] run env: add comment for query_total_memory()

[Christoph Heiss]

This is mainly to explicitly document the unit of its return value.

No functional changes.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v2 -> v3:
  * new patch

 Proxmox/Install/RunEnv.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Proxmox/Install/RunEnv.pm b/Proxmox/Install/RunEnv.pm
index c9d788b..3e810b2 100644
--- a/Proxmox/Install/RunEnv.pm
+++ b/Proxmox/Install/RunEnv.pm
@@ -18,6 +18,8 @@ my sub fromjs : prototype($) {
 }

 my $mem_total = undef;
+# Returns the system memory size in MiB, and falls back to 512 MiB if it
+# could not be determined.
 sub query_total_memory : prototype() {
     return $mem_total if defined($mem_total);

--
2.42.0

[pve-devel] [PATCH installer v3 2/8] 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>
---
Changes v1 -> v2:
  * no changes

Changes v2 -> v3:
  * added #[allow(unused)] attribute to ::new_with_suffix(), to avoid
    warnings until it is actually used

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

diff --git a/proxmox-tui-installer/src/views/mod.rs b/proxmox-tui-installer/src/views/mod.rs
index aa24fa4..8882ce9 100644
--- a/proxmox-tui-installer/src/views/mod.rs
+++ b/proxmox-tui-installer/src/views/mod.rs
@@ -19,16 +19,38 @@ 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.
+    #[allow(unused)]
+    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 +64,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 +72,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 +106,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 +117,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 +175,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 +193,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 +216,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.42.0

[pve-devel] [PATCH installer v3 3/8] tui: bootdisk: simplify product handling by passing the config directly

[Christoph Heiss]

No functional changes.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v2 -> v3:
  * new patch

 proxmox-tui-installer/src/views/bootdisk.rs | 50 ++++++++++++---------
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index ba08c8b..3addd6c 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -134,10 +134,9 @@ impl AdvancedBootdiskOptionsView {
             .child(DummyView.full_width());

         match &options.advanced {
-            AdvancedBootdiskOptions::Lvm(lvm) => view.add_child(LvmBootdiskOptionsView::new(
-                lvm,
-                product_conf.product == ProxmoxProduct::PVE,
-            )),
+            AdvancedBootdiskOptions::Lvm(lvm) => {
+                view.add_child(LvmBootdiskOptionsView::new(lvm, &product_conf))
+            }
             AdvancedBootdiskOptions::Zfs(zfs) => {
                 view.add_child(ZfsBootdiskOptionsView::new(disks, zfs))
             }
@@ -150,10 +149,8 @@ impl AdvancedBootdiskOptionsView {
     }

     fn fstype_on_submit(siv: &mut Cursive, disks: &[Disk], fstype: &FsType) {
-        let is_pve = siv
-            .user_data::<InstallerState>()
-            .map(|state| state.setup_info.config.product == ProxmoxProduct::PVE)
-            .unwrap_or_default();
+        let state = siv.user_data::<InstallerState>().unwrap();
+        let product_conf = state.setup_info.config.clone();

         siv.call_on_name("advanced-bootdisk-options-dialog", |view: &mut Dialog| {
             if let Some(AdvancedBootdiskOptionsView { view }) =
@@ -161,18 +158,15 @@ impl AdvancedBootdiskOptionsView {
             {
                 view.remove_child(3);
                 match fstype {
-                    FsType::Ext4 | FsType::Xfs => view.add_child(LvmBootdiskOptionsView::new(
-                        &LvmBootdiskOptions::defaults_from(&disks[0]),
-                        is_pve,
-                    )),
-                    FsType::Zfs(_) => view.add_child(ZfsBootdiskOptionsView::new(
-                        disks,
-                        &ZfsBootdiskOptions::defaults_from(disks),
-                    )),
-                    FsType::Btrfs(_) => view.add_child(BtrfsBootdiskOptionsView::new(
-                        disks,
-                        &BtrfsBootdiskOptions::defaults_from(disks),
-                    )),
+                    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::Btrfs(_) => {
+                        view.add_child(BtrfsBootdiskOptionsView::new_with_defaults(disks))
+                    }
                 }
             }
         });
@@ -261,7 +255,9 @@ struct LvmBootdiskOptionsView {
 }

 impl LvmBootdiskOptionsView {
-    fn new(options: &LvmBootdiskOptions, show_extra_fields: bool) -> Self {
+    fn new(options: &LvmBootdiskOptions, product_conf: &ProductConfig) -> Self {
+        let show_extra_fields = product_conf.product == ProxmoxProduct::PVE;
+
         // TODO: Set maximum accordingly to disk size
         let view = FormView::new()
             .child(
@@ -295,6 +291,10 @@ impl LvmBootdiskOptionsView {
         }
     }

+    fn new_with_defaults(disk: &Disk, product_conf: &ProductConfig) -> Self {
+        Self::new(&LvmBootdiskOptions::defaults_from(disk), product_conf)
+    }
+
     fn get_values(&mut self) -> Option<LvmBootdiskOptions> {
         let min_lvm_free_id = if self.has_extra_fields { 4 } else { 2 };

@@ -481,6 +481,10 @@ impl BtrfsBootdiskOptionsView {
         Self { view }
     }

+    fn new_with_defaults(disks: &[Disk]) -> Self {
+        Self::new(disks, &BtrfsBootdiskOptions::defaults_from(disks))
+    }
+
     fn get_values(&mut self) -> Option<(Vec<Disk>, BtrfsBootdiskOptions)> {
         let (disks, selected_disks) = self.view.get_disks_and_selection()?;
         let disk_size = self.view.inner_mut()?.get_value::<DiskSizeEditView, _>(0)?;
@@ -543,6 +547,10 @@ impl ZfsBootdiskOptionsView {
         Self { view }
     }

+    fn new_with_defaults(disks: &[Disk]) -> Self {
+        Self::new(disks, &ZfsBootdiskOptions::defaults_from(disks))
+    }
+
     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()?;
--
2.42.0

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

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * no changes

Changes v2 -> v3:
  * better documented calculation and renamed some variables to reflect
    its unit (thanks Thomas!)
  * moved modprobe config setup into separate sub

 Proxmox/Install.pm        | 14 ++++++++++++
 Proxmox/Install/Config.pm |  1 +
 Proxmox/Install/RunEnv.pm | 45 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+)

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index 1117fc4..f371716 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -291,6 +291,19 @@ sub get_zfs_raid_setup {
     return ($devlist, $cmd);
 }

+# If the maximum ARC size for ZFS was explicitly changed by the user, applies
+# it to the new system by setting the `zfs_arc_max` module parameter in /etc/modprobe.d/zfs.conf
+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;
+
+    if ($arc_max > 0) {
+	file_write_all("$targetdir/etc/modprobe.d/zfs.conf", "options zfs zfs_arc_max=$arc_max\n")
+    }
+}
+
 sub get_btrfs_raid_setup {
     my $filesys = Proxmox::Install::Config::get_filesys();

@@ -1141,6 +1154,7 @@ _EOD

 	    file_write_all("$targetdir/etc/kernel/cmdline", "root=ZFS=$zfs_pool_name/ROOT/$zfs_root_volume_name boot=zfs\n");

+	    zfs_setup_module_conf($targetdir);
 	}

 	diversion_remove($targetdir, "/usr/sbin/update-grub");
diff --git a/Proxmox/Install/Config.pm b/Proxmox/Install/Config.pm
index 024f62a..f496d61 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(), # in MiB
 	},
 	# TODO: single disk selection config
 	target_hd => undef,
diff --git a/Proxmox/Install/RunEnv.pm b/Proxmox/Install/RunEnv.pm
index 3e810b2..9116397 100644
--- a/Proxmox/Install/RunEnv.pm
+++ b/Proxmox/Install/RunEnv.pm
@@ -303,6 +303,51 @@ sub query_installation_environment : prototype() {
     return $output;
 }

+# OpenZFS specifies 64 MiB as the absolute minimum:
+# https://openzfs.github.io/openzfs-docs/Performance%20and%20Tuning/Module%20Parameters.html#zfs-arc-max
+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
+
+# 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 $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;
+    } elsif ($rounded_mib < $ZFS_ARC_MIN_SIZE_MIB) {
+	return $ZFS_ARC_MIN_SIZE_MIB;
+    }
+
+    return $rounded_mib;
+}
+
+# Clamps the provided ZFS arc_max value (in MiB) to the accepted bounds. The
+# lower is specified by `$ZFS_ARC_MIN_SIZE_MIB`, the upper by the available system memory.
+# Returns the clamped value in MiB.
+sub clamp_zfs_arc_max {
+    my ($mib) = @_;
+
+    return $mib if $mib == 0;
+
+    my $total_mem_mib = get('total_memory');
+    if ($mib > $total_mem_mib) {
+	return $total_mem_mib;
+    } elsif ($mib < $ZFS_ARC_MIN_SIZE_MIB) {
+	return $ZFS_ARC_MIN_SIZE_MIB;
+    }
+
+    return $mib;
+}
+
 my $_env = undef;
 sub get {
     my ($k) = @_;
--
2.42.0

[pve-devel] [PATCH installer v3 5/8] fix #4829: proxinstall: expose new `arc_max` ZFS option for PVE installations

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * no changes

Changes v2 -> v3:
  * no changes

 proxinstall | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/proxinstall b/proxinstall
index 64c8bab..f1a3c02 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, $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 v3 6/8] fix #4829: test: add tests for new zfs_arc_max calculations

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * no changes

Changes v2 -> v3:
  * no changes

 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 111fe4b..13a2b81 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.42.0

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

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * updated comment for ZfsBootdiskOptions::defaults_from() accordingly

Changes v2 -> v2:
  * documented the unit of the return value of default_zfs_arc_max()

 proxmox-tui-installer/src/options.rs        | 57 +++++++++++++++++++--
 proxmox-tui-installer/src/setup.rs          |  2 +
 proxmox-tui-installer/src/views/bootdisk.rs | 31 ++++++-----
 3 files changed, 71 insertions(+), 19 deletions(-)

diff --git a/proxmox-tui-installer/src/options.rs b/proxmox-tui-installer/src/options.rs
index 85b39b8..c9b036d 100644
--- a/proxmox-tui-installer/src/options.rs
+++ b/proxmox-tui-installer/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};
 use crate::SummaryOption;

@@ -190,21 +192,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];
+    /// 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(),
         }
     }
 }
@@ -444,6 +448,27 @@ 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
+///
+/// # 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
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -470,6 +495,28 @@ 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
+        ];
+
+        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);
+        }
+    }
+
     #[test]
     fn network_options_from_setup_network_info() {
         let setup = dummy_setup_info();
diff --git a/proxmox-tui-installer/src/setup.rs b/proxmox-tui-installer/src/setup.rs
index 5575759..e9fe70d 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 3addd6c..07ca5b7 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -16,7 +16,7 @@ use crate::{
         FsType, LvmBootdiskOptions, ZfsBootdiskOptions, ZfsRaidLevel, FS_TYPES,
         ZFS_CHECKSUM_OPTIONS, ZFS_COMPRESS_OPTIONS,
     },
-    setup::{BootType, ProductConfig},
+    setup::{BootType, ProductConfig, RuntimeInfo},
 };
 use crate::{setup::ProxmoxProduct, InstallerState};

@@ -123,10 +123,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(move |siv, fstype| Self::fstype_on_submit(siv, fstype));

         let mut view = LinearLayout::vertical()
             .child(DummyView.full_width())
@@ -148,8 +145,9 @@ impl AdvancedBootdiskOptionsView {
         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();

         siv.call_on_name("advanced-bootdisk-options-dialog", |view: &mut Dialog| {
@@ -159,13 +157,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(disks))
-                    }
+                    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))
                     }
                 }
             }
@@ -179,7 +178,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())),
@@ -547,8 +546,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)> {
@@ -568,6 +570,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 v3 8/8] 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.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * fix ZFS_ARC_MIN_SIZE to be MiB rather than bytes

Changes v2 -> v3:
  * renamed `ZFS_ARC_MIN_SIZE` -> `ZFS_ARC_MIN_SIZE_MIB` and documented
    the origin of its value

 proxmox-tui-installer/src/main.rs           |  2 +-
 proxmox-tui-installer/src/views/bootdisk.rs | 55 +++++++++++++++------
 proxmox-tui-installer/src/views/mod.rs      |  1 -
 3 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
index 81fe3ca..5365d51 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -413,7 +413,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 07ca5b7..da4706b 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -20,6 +20,10 @@ use crate::{
 };
 use crate::{setup::ProxmoxProduct, InstallerState};

+/// 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: usize = 64; // MiB
+
 pub struct BootdiskOptionsView {
     view: LinearLayout,
     advanced_options: Rc<RefCell<BootdiskOptions>>,
@@ -27,13 +31,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");

@@ -47,11 +51,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(),
                     ));
@@ -104,7 +108,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() };

@@ -135,10 +139,10 @@ 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))
             }
         };

@@ -508,7 +512,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(
@@ -536,9 +546,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());
@@ -548,20 +565,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)
+        } else {
+            0 // use built-in ZFS default value
+        };

         Some((
             disks,
@@ -570,7 +597,7 @@ impl ZfsBootdiskOptionsView {
                 compress,
                 checksum,
                 copies,
-                arc_max: 0, // use built-in ZFS default value
+                arc_max,
                 disk_size,
                 selected_disks,
             },
@@ -583,12 +610,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 8882ce9..e997968 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] partially-applied: [PATCH installer v3 0/8] fix #4829: set up lower default limit for ZFS ARC in installer

[Thomas Lamprecht]

Am 31/10/2023 um 13:10 schrieb Christoph Heiss:
> Christoph Heiss (8):
>   run env: add comment for query_total_memory()
>   tui: views: add optional suffix label for `NumericEditView`s
>   tui: bootdisk: simplify product handling by passing the config
>     directly
>   fix #4829: install: add new ZFS `arc_max` setup option

applied until here, most of the rest needs rebase (not the next one,
but I wanted to avoid exposing this in the GTK UI, but not TUI)

>   fix #4829: proxinstall: expose new `arc_max` ZFS option for PVE
>     installations
>   fix #4829: test: add tests for new zfs_arc_max calculations
>   fix #4829: tui: setup: add new ZFS `arc_max` option
>   fix #4829: tui: bootdisk: expose new `arc_max` ZFS option for PVE
>     installations

I think you can drop some of those "fix #num" prefixes ;-)