[pve-devel] [PATCH installer 2/4] tui: use default ZFS ARC maximum size from runtime enviroment

[Christoph Heiss <c.heiss@proxmox.com>]

Now that the value is pre-calculated in the low-level installer and
written to `run-env.json`, use it from there instead of calculating it
separately - thus having a single source of truth.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-installer-common/src/options.rs     | 58 ++-------------------
 proxmox-installer-common/src/setup.rs       |  3 ++
 proxmox-tui-installer/src/views/bootdisk.rs | 42 ++++++---------
 3 files changed, 21 insertions(+), 82 deletions(-)

diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index 0fd3e43..40100d8 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -6,9 +6,7 @@ use std::str::FromStr;
 use std::sync::OnceLock;
 use std::{cmp, fmt};
 
-use crate::setup::{
-    LocaleInfo, NetworkInfo, ProductConfig, ProxmoxProduct, RuntimeInfo, SetupInfo,
-};
+use crate::setup::{LocaleInfo, NetworkInfo, RuntimeInfo, SetupInfo};
 use crate::utils::{CidrAddress, Fqdn};
 
 #[derive(Copy, Clone, Debug, Deserialize, Serialize, Eq, PartialEq)]
@@ -256,42 +254,20 @@ pub struct ZfsBootdiskOptions {
 
 impl ZfsBootdiskOptions {
     /// Panics if the disk list is empty.
-    pub fn defaults_from(runinfo: &RuntimeInfo, product_conf: &ProductConfig) -> Self {
+    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(product_conf.product, runinfo.total_memory),
+            arc_max: runinfo.default_zfs_arc_max,
             disk_size: disk.size,
             selected_disks: (0..runinfo.disks.len()).collect(),
         }
     }
 }
 
-/// Calculates the default upper limit for the ZFS ARC size.
-/// See also <https://bugzilla.proxmox.com/show_bug.cgi?id=4829> and
-/// https://openzfs.github.io/openzfs-docs/Performance%20and%20Tuning/Module%20Parameters.html#zfs-arc-max
-///
-/// # Arguments
-/// * `product` - The product to be installed
-/// * `total_memory` - Total memory installed in the system, in MiB
-///
-/// # Returns
-/// The default ZFS maximum ARC size in MiB for this system.
-fn default_zfs_arc_max(product: ProxmoxProduct, total_memory: usize) -> usize {
-    if product != ProxmoxProduct::PVE {
-        // For products other the PVE, just let ZFS decide on its own. Setting `0`
-        // causes the installer to skip writing the `zfs_arc_max` module parameter.
-        0
-    } else {
-        ((total_memory as f64) / 10.)
-            .round()
-            .clamp(64., 16. * 1024.) as usize
-    }
-}
-
 #[derive(Clone, Debug)]
 pub enum AdvancedBootdiskOptions {
     Lvm(LvmBootdiskOptions),
@@ -493,31 +469,3 @@ pub fn email_validate(email: &str) -> Result<()> {
 
     Ok(())
 }
-
-#[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);
-            assert_eq!(default_zfs_arc_max(ProxmoxProduct::PDM, *total_memory), 0);
-        }
-    }
-}
diff --git a/proxmox-installer-common/src/setup.rs b/proxmox-installer-common/src/setup.rs
index 93818f3..6b033e1 100644
--- a/proxmox-installer-common/src/setup.rs
+++ b/proxmox-installer-common/src/setup.rs
@@ -390,6 +390,9 @@ pub struct RuntimeInfo {
     /// Whether the system was booted with SecureBoot enabled
     #[serde(default, deserialize_with = "deserialize_bool_from_int_maybe")]
     pub secure_boot: Option<bool>,
+
+    /// Default upper limit for the ZFS ARC size, in MiB.
+    pub default_zfs_arc_max: usize,
 }
 
 #[derive(Copy, Clone, Eq, Deserialize, PartialEq, Serialize)]
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index fffb05e..2e2019d 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -162,7 +162,7 @@ impl AdvancedBootdiskOptionsView {
                 &product_conf,
             )),
             AdvancedBootdiskOptions::Zfs(zfs) => {
-                view.add_child(ZfsBootdiskOptionsView::new(runinfo, zfs, &product_conf))
+                view.add_child(ZfsBootdiskOptionsView::new(runinfo, zfs))
             }
             AdvancedBootdiskOptions::Btrfs(btrfs) => {
                 view.add_child(BtrfsBootdiskOptionsView::new(runinfo, btrfs))
@@ -213,10 +213,9 @@ impl AdvancedBootdiskOptionsView {
                             &product_conf,
                         ))
                     }
-                    FsType::Zfs(_) => view.add_child(ZfsBootdiskOptionsView::new_with_defaults(
-                        &runinfo,
-                        &product_conf,
-                    )),
+                    FsType::Zfs(_) => {
+                        view.add_child(ZfsBootdiskOptionsView::new_with_defaults(&runinfo))
+                    }
                     FsType::Btrfs(_) => {
                         view.add_child(BtrfsBootdiskOptionsView::new_with_defaults(&runinfo))
                     }
@@ -631,27 +630,20 @@ struct ZfsBootdiskOptionsView {
 
 impl ZfsBootdiskOptionsView {
     // TODO: Re-apply previous disk selection from `options` correctly
-    fn new(
-        runinfo: &RuntimeInfo,
-        options: &ZfsBootdiskOptions,
-        product_conf: &ProductConfig,
-    ) -> Self {
+    fn new(runinfo: &RuntimeInfo, options: &ZfsBootdiskOptions) -> Self {
         let arc_max_view = {
             let view = IntegerEditView::new_with_suffix("MiB").max_value(runinfo.total_memory);
 
-            // For PVE "force" the default value, for other products place the recommended value
-            // only in the placeholder. This causes for the latter to not write the module option
-            // if the value is never modified by the user.
-            if product_conf.product == ProxmoxProduct::PVE {
+            // If the runtime environment provides a non-zero value, that is
+            // also not the built-in ZFS default of half the system memory, use
+            // that as default.
+            // Otherwise, just place the ZFS default into the placeholder.
+            if runinfo.default_zfs_arc_max > 0
+                && runinfo.default_zfs_arc_max != runinfo.total_memory / 2
+            {
                 view.content(options.arc_max)
             } else {
-                let view = view.placeholder(runinfo.total_memory / 2);
-
-                if options.arc_max != 0 {
-                    view.content(options.arc_max)
-                } else {
-                    view
-                }
+                view.placeholder(runinfo.total_memory / 2)
             }
         };
 
@@ -696,12 +688,8 @@ impl ZfsBootdiskOptionsView {
         Self { view }
     }
 
-    fn new_with_defaults(runinfo: &RuntimeInfo, product_conf: &ProductConfig) -> Self {
-        Self::new(
-            runinfo,
-            &ZfsBootdiskOptions::defaults_from(runinfo, product_conf),
-            product_conf,
-        )
+    fn new_with_defaults(runinfo: &RuntimeInfo) -> Self {
+        Self::new(runinfo, &ZfsBootdiskOptions::defaults_from(runinfo))
     }
 
     fn get_values(&mut self) -> Option<(Vec<Disk>, ZfsBootdiskOptions)> {
-- 
2.47.1



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel