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

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

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@proxmox.com>
---
Changes v4 -> v5:
  * rebased on latest master
  * fixed value not persisting across dialog open/close for non-PVE

Changes v3 -> v4:
  * rebased on latest master

Changes v2 -> v3:
  * no changes

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 9375ded..34dfadb 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -214,7 +214,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 1e22105..dfb1608 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -622,7 +622,24 @@ 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 {
+                let view = view.placeholder(runinfo.total_memory / 2);
+
+                if options.arc_max != 0 {
+                    view.content(options.arc_max)
+                } else {
+                    view
+                }
+            }
+        };
 
         let inner = FormView::new()
             .child("ashift", IntegerEditView::new().content(options.ashift))
@@ -654,13 +671,7 @@ impl ZfsBootdiskOptionsView {
                 "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("ARC max size", arc_max_view)
             .child("hdsize", DiskSizeEditView::new().content(options.disk_size));
 
         let view = MultiDiskOptionsView::new(&runinfo.disks, &options.selected_disks, inner)
@@ -682,21 +693,22 @@ 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.get_options_view()?;
-        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 occurred return `0`. The former simply means
+        // that the placeholder value is still there.
+        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 c7491fc..a41dac3 100644
--- a/proxmox-tui-installer/src/views/mod.rs
+++ b/proxmox-tui-installer/src/views/mod.rs
@@ -96,7 +96,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.45.2



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