[pve-devel] [PATCH installer v2 2/3] tui: expose arc size setting for zfs bootdisks for all products

[Christoph Heiss]

For non-PVE products, simply use the ZFS defaults (aka. 50%) and leave
unset, if the user never touches that setting.

Signed-off-by: Christoph Heiss <c.heiss at proxmox.com>
---
Changes v1 -> v2:
  * use new placeholder functionality for non-PVE products

 proxmox-installer-common/src/options.rs     |  3 +-
 proxmox-tui-installer/src/views/bootdisk.rs | 48 +++++++++++++--------
 proxmox-tui-installer/src/views/mod.rs      |  1 -
 3 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index 1aa8f65..a210142 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -205,7 +205,8 @@ impl ZfsBootdiskOptions {
 /// 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
+        // 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.)
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 7e13e91..b856342 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -564,7 +564,18 @@ impl ZfsBootdiskOptionsView {
         options: &ZfsBootdiskOptions,
         product_conf: &ProductConfig,
     ) -> Self {
-        let is_pve = product_conf.product == ProxmoxProduct::PVE;
+        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 {
+                view.content(options.arc_max)
+            } else {
+                view.placeholder(runinfo.total_memory / 2)
+            }
+        };

         let inner = FormView::new()
             .child("ashift", IntegerEditView::new().content(options.ashift))
@@ -592,14 +603,11 @@ impl ZfsBootdiskOptionsView {
                             .unwrap_or_default(),
                     ),
             )
-            .child("copies", IntegerEditView::new().content(options.copies).max_value(3))
-            .child_conditional(
-                is_pve,
-                "ARC max size",
-                IntegerEditView::new_with_suffix("MiB")
-                    .max_value(runinfo.total_memory)
-                    .content(options.arc_max),
+            .child(
+                "copies",
+                IntegerEditView::new().content(options.copies).max_value(3),
             )
+            .child("ARC max size", arc_max_view)
             .child("hdsize", DiskSizeEditView::new().content(options.disk_size));

         let view = MultiDiskOptionsView::new(&runinfo.disks, &options.selected_disks, inner)
@@ -621,21 +629,25 @@ 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, _>(disk_size_index)?;
-
-        let arc_max = if has_arc_max {
-            view.get_value::<IntegerEditView, _>(4)?
-                .max(ZFS_ARC_MIN_SIZE_MIB)
-        } else {
-            0 // use built-in ZFS default value
-        };
+        let disk_size = view.get_value::<DiskSizeEditView, _>(5)?;
+
+        // If a value is set, return that and clamp it to at least [`ZFS_ARC_MIN_SIZE_MIB`].
+        //
+        // Otherwise, if no value was set or an error occured return `0`. The former simply means
+        // that the placeholder value is still there.
+        //
+        // TODO: Return an [`Result`] from here as well and propagate the [`Result::Err`] value
+        // instead of defaulting to `0`.
+        let arc_max = view
+            .get_child::<IntegerEditView>(4)?
+            .get_content_maybe()
+            .map_or(Ok(0), |v| v.map(|v| v.max(ZFS_ARC_MIN_SIZE_MIB)))
+            .unwrap_or(0);

         Some((
             disks,
diff --git a/proxmox-tui-installer/src/views/mod.rs b/proxmox-tui-installer/src/views/mod.rs
index 299517e..9e27c5f 100644
--- a/proxmox-tui-installer/src/views/mod.rs
+++ b/proxmox-tui-installer/src/views/mod.rs
@@ -93,7 +93,6 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
     ///
     /// # Arguments
     /// `placeholder` - The placeholder value to set for this view.
-    #[allow(unused)]
     pub fn placeholder(mut self, placeholder: T) -> Self {
         self.placeholder = Some(placeholder);
         self.allow_empty(true)
--
2.43.0