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

[pve-devel] [PATCH installer v2 0/6] 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. 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

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

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        |  50 +++++++++-
 proxmox-tui-installer/src/setup.rs          |   2 +
 proxmox-tui-installer/src/views/bootdisk.rs |  61 +++++++++---
 proxmox-tui-installer/src/views/mod.rs      | 104 +++++++++++++++-----
 test/Makefile                               |  10 ++
 test/zfs-arc-max.pl                         |  81 +++++++++++++++
 13 files changed, 329 insertions(+), 43 deletions(-)
 create mode 100644 test/Makefile
 create mode 100755 test/zfs-arc-max.pl

--
2.41.0

[pve-devel] [PATCH installer v2 1/6] 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

 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 1117fc4..0c3541f 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -1141,6 +1141,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 c9d788b..9236030 100644
--- a/Proxmox/Install/RunEnv.pm
+++ b/Proxmox/Install/RunEnv.pm
@@ -301,6 +301,44 @@ sub query_installation_environment : prototype() {
     return $output;
 }

+our $ZFS_ARC_MIN_SIZE = 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.42.0

[pve-devel] [PATCH installer v2 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>
---
Changes v1 -> v2:
  * 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 v2 3/6] 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

 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 v2 4/6] 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

 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.42.0

[pve-devel] [PATCH installer v2 5/6] 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

 proxmox-tui-installer/src/options.rs        | 52 +++++++++++++++++++--
 proxmox-tui-installer/src/setup.rs          |  2 +
 proxmox-tui-installer/src/views/bootdisk.rs |  8 +++-
 3 files changed, 56 insertions(+), 6 deletions(-)

diff --git a/proxmox-tui-installer/src/options.rs b/proxmox-tui-installer/src/options.rs
index abc8c7b..fbed8d9 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];
+    /// Panics if the disk list is empty.
+    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(),
         }
     }
 }
@@ -451,6 +453,24 @@ 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::*;
@@ -460,6 +480,28 @@ mod tests {
     };
     use std::{collections::HashMap, path::PathBuf};

+    #[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);
+        }
+    }
+
     fn fill_setup_info() {
         crate::init_setup_info(SetupInfo {
             config: ProductConfig {
diff --git a/proxmox-tui-installer/src/setup.rs b/proxmox-tui-installer/src/setup.rs
index 7238131..25504fe 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 dbd13ea..e8322db 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -139,6 +139,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()
@@ -150,7 +155,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,
@@ -541,6 +546,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 v2 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.

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

 proxmox-tui-installer/src/main.rs           |  2 +-
 proxmox-tui-installer/src/views/bootdisk.rs | 55 +++++++++++++++------
 2 files changed, 40 insertions(+), 17 deletions(-)

diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
index 0a61e39..44d44c0 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -431,7 +431,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 e8322db..f7fbea3 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -16,10 +16,12 @@ use crate::{
         FsType, LvmBootdiskOptions, ZfsBootdiskOptions, ZfsRaidLevel, FS_TYPES,
         ZFS_CHECKSUM_OPTIONS, ZFS_COMPRESS_OPTIONS,
     },
-    setup::BootType,
+    setup::{BootType, RuntimeInfo},
 };
 use crate::{setup::ProxmoxProduct, InstallerState};

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

@@ -42,10 +44,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()));
                 }
             }));

@@ -95,7 +97,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() };
@@ -116,7 +118,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)
             });

@@ -128,10 +130,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))
             }
         };

@@ -154,7 +156,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(
@@ -491,7 +493,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(
@@ -519,9 +523,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());
@@ -532,12 +543,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,
@@ -546,7 +566,7 @@ impl ZfsBootdiskOptionsView {
                 compress,
                 checksum,
                 copies,
-                arc_max: 0, // use built-in ZFS default value
+                arc_max,
                 disk_size,
                 selected_disks,
             },
@@ -558,9 +578,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.42.0

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

[Thomas Lamprecht]

Am 24/10/2023 um 13:55 schrieb Christoph Heiss:
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
> Changes v1 -> v2:
>   * No changes
> 
>  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 1117fc4..0c3541f 100644
> --- a/Proxmox/Install.pm
> +++ b/Proxmox/Install.pm
> @@ -1141,6 +1141,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;

Mixing multiplication and method call this way is a bit hard to read.

Maybe move the clamping into  out to its own local method, something like:

my sub setup_zfs_conf {
    my ($target_dir) = @_;

    my $arc_max_mb = Proxmox::Install::Config::get_zfs_opt('arc_max');
    my $arc_max = Proxmox::Install::RunEnv::clamp_zfs_arc_max($arc_max_mb) * 1024 * 1024;

    if ($arc_max > 0) {
        file_write_all("${target_dir}/etc/modprobe.d/zfs.conf", "options zfs zfs_arc_max=$arc_max\n");
    }
}

> +	    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 c9d788b..9236030 100644
> --- a/Proxmox/Install/RunEnv.pm
> +++ b/Proxmox/Install/RunEnv.pm
> @@ -301,6 +301,44 @@ sub query_installation_environment : prototype() {
>      return $output;
>  }
> 
> +our $ZFS_ARC_MIN_SIZE = 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));

knowing in what units this is calculating/returning would be great here
(via comment and/or variable name)

> +    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) = @_;

please at least comment, or better, name variables accordingly, in what
units your dealing here, I think MB?

When I worked on the partition sizing code it was a PITA to trace what
unit was used when.

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

[Thomas Lamprecht]

Am 24/10/2023 um 13:55 schrieb 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.
> 
> v1: https://lists.proxmox.com/pipermail/pve-devel/2023-August/058830.html
> 
> Notable changes v1 -> v2:
>   * Rebased on latest master
>   * Fix arc_max value set in TUI not being applied correctly
> 
> 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
> 

looks OK bis of a few comments in the first patch, I can fix those up
too, but maybe it's quicker if just send a v3, if you agree with them.

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

[Christoph Heiss]

Thanks for the review!

I will refactor all the points mentioned and send a v2 soon.

On Tue, Oct 24, 2023 at 05:05:15PM +0200, Thomas Lamprecht wrote:
>
> Am 24/10/2023 um 13:55 schrieb Christoph Heiss:
> > Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> > ---
> > Changes v1 -> v2:
> >   * No changes
> >
> >  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 1117fc4..0c3541f 100644
> > --- a/Proxmox/Install.pm
> > +++ b/Proxmox/Install.pm
> > @@ -1141,6 +1141,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;
>
> Mixing multiplication and method call this way is a bit hard to read.
>
> Maybe move the clamping into  out to its own local method, something like:
>
> my sub setup_zfs_conf {
>     my ($target_dir) = @_;
>
>     my $arc_max_mb = Proxmox::Install::Config::get_zfs_opt('arc_max');
>     my $arc_max = Proxmox::Install::RunEnv::clamp_zfs_arc_max($arc_max_mb) * 1024 * 1024;
>
>     if ($arc_max > 0) {
>         file_write_all("${target_dir}/etc/modprobe.d/zfs.conf", "options zfs zfs_arc_max=$arc_max\n");
>     }
> }

Seems like a good idea, the extract_data() sub is _way_ to overloaded
already anyway.

>
> > +	    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 c9d788b..9236030 100644
> > --- a/Proxmox/Install/RunEnv.pm
> > +++ b/Proxmox/Install/RunEnv.pm
> > @@ -301,6 +301,44 @@ sub query_installation_environment : prototype() {
> >      return $output;
> >  }
> >
> > +our $ZFS_ARC_MIN_SIZE = 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));
>
> knowing in what units this is calculating/returning would be great here
> (via comment and/or variable name)
Definitely sensible, I will rename + expand the comments a bit more.

>
> > +    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) = @_;
>
> please at least comment, or better, name variables accordingly, in what
> units your dealing here, I think MB?
>
> When I worked on the partition sizing code it was a PITA to trace what
> unit was used when.

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

[Christoph Heiss]

On Tue, Oct 24, 2023 at 08:59:36AM -0300, Gilberto Ferreira via pve-devel wrote:
> Date: Tue, 24 Oct 2023 08:59:36 -0300
> From: Gilberto Ferreira <gilberto.nunes32@gmail.com>
> To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
> Subject: Re: [pve-devel] [PATCH installer v2 1/6] fix #4829: install: add
>  new ZFS `arc_max` setup option
>
>
> Hi there.
> Now, that's a good option in the installer.
> I wonder if this option will be available post-intall, like some box in the
> ZFS manager from WEB GUI!
> That's would be nice.
>

Currently, this isn't planned, although - since that setting is exposed
after all in the installer in the future - it would be kind of sensible
to add it to the GUI as well, I guess.
But as a separate series from this one, of course.

CC @Thomas - what do you think?

If you think this is something worth doing, I'd create a proper bugzilla
ticket with some more details.

My thinking here (roughly) would be:

* Move the {default,clamp}_zfs_arc_max() stuff to pve-common
* As for the API, probably shove it under `/nodes/{node}/config`?
  It's a per-node configuration after all, it seems to fit there best
  without introducing another endpoint just for this
* GUI-wise; if we do the above, it probably should also go under node ->
  system -> options ..
  Otherwise, add a small, separate panel for options to node -> disks ->
  zfs maybe?

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

[Thomas Lamprecht]

Am 25/10/2023 um 10:28 schrieb Christoph Heiss:
> On Tue, Oct 24, 2023 at 08:59:36AM -0300, Gilberto Ferreira via pve-devel wrote:
>> Date: Tue, 24 Oct 2023 08:59:36 -0300
>> From: Gilberto Ferreira <gilberto.nunes32@gmail.com>
>> To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
>> Subject: Re: [pve-devel] [PATCH installer v2 1/6] fix #4829: install: add
>>  new ZFS `arc_max` setup option
>>
>>
>> Hi there.
>> Now, that's a good option in the installer.
>> I wonder if this option will be available post-intall, like some box in the
>> ZFS manager from WEB GUI!
>> That's would be nice.
>>
> 
> Currently, this isn't planned, although - since that setting is exposed
> after all in the installer in the future - it would be kind of sensible
> to add it to the GUI as well, I guess.
> But as a separate series from this one, of course.
> 
> CC @Thomas - what do you think?

We would need to either have a separate config entry, that can easily
get out of sync with the actual configured values, or parse all possible
ways to set this, e.g., modprobe configs, which isn't too nice.

With a few trade-offs/limitations one could probably work around that,
but IMO it can be a bit too much hassle for something that one normally
changes only once or twice (or once this change is in, probably never
for new setups), so for now I'd keep this manual.

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

[Christoph Heiss]

On Wed, Oct 25, 2023 at 07:09:00PM +0200, Thomas Lamprecht wrote:
>
> Am 25/10/2023 um 10:28 schrieb Christoph Heiss:
> > On Tue, Oct 24, 2023 at 08:59:36AM -0300, Gilberto Ferreira via pve-devel wrote:
> >
> > Currently, this isn't planned, although - since that setting is exposed
> > after all in the installer in the future - it would be kind of sensible
> > to add it to the GUI as well, I guess.
> > But as a separate series from this one, of course.
> >
> > CC @Thomas - what do you think?
>
> We would need to either have a separate config entry, that can easily
> get out of sync with the actual configured values, or parse all possible
> ways to set this, e.g., modprobe configs, which isn't too nice.
Yeah, that actually seems like a big problem.

>
> With a few trade-offs/limitations one could probably work around that,
> but IMO it can be a bit too much hassle for something that one normally
> changes only once or twice (or once this change is in, probably never
> for new setups), so for now I'd keep this manual.
Then I'd also leave it at this, just being able to change it in the
installer and afterwards only "manually".

It is more than sufficiently documented in our guides how to change
that, let alone searching for that topic on the internet.

Thanks for chiming in!