* [pve-devel] [PATCH installer v4 1/6] run env: remove debug print
2023-11-07 12:20 [pve-devel] [PATCH installer v4 0/6] fix #4829: wire up `arc_max` ZFS option to GUI/TUI for PVE Christoph Heiss
@ 2023-11-07 12:20 ` Christoph Heiss
2023-11-07 12:20 ` [pve-devel] [PATCH installer v4 2/6] install: use correct variable names in zfs_setup_module_conf() Christoph Heiss
` (5 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Christoph Heiss @ 2023-11-07 12:20 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Proxmox/Install/RunEnv.pm | 1 -
1 file changed, 1 deletion(-)
diff --git a/Proxmox/Install/RunEnv.pm b/Proxmox/Install/RunEnv.pm
index 9116397..5f68d82 100644
--- a/Proxmox/Install/RunEnv.pm
+++ b/Proxmox/Install/RunEnv.pm
@@ -319,7 +319,6 @@ sub default_zfs_arc_max {
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;
--
2.42.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [pve-devel] [PATCH installer v4 2/6] install: use correct variable names in zfs_setup_module_conf()
2023-11-07 12:20 [pve-devel] [PATCH installer v4 0/6] fix #4829: wire up `arc_max` ZFS option to GUI/TUI for PVE Christoph Heiss
2023-11-07 12:20 ` [pve-devel] [PATCH installer v4 1/6] run env: remove debug print Christoph Heiss
@ 2023-11-07 12:20 ` Christoph Heiss
2023-11-07 12:20 ` [pve-devel] [PATCH installer v4 3/6] proxinstall: expose `arc_max` ZFS option for PVE installations Christoph Heiss
` (4 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Christoph Heiss @ 2023-11-07 12:20 UTC (permalink / raw)
To: pve-devel
That's what happens when you do some last-minute variable renaming and
trust that nothing broke ..
Fixes: 42aa2fa ("fix #4829: install: add new ZFS `arc_max` setup option")
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Proxmox/Install.pm | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index 693ab15..c4b0700 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -296,8 +296,8 @@ sub get_zfs_raid_setup {
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;
+ my $arc_max_mib = Proxmox::Install::Config::get_zfs_opt('arc_max');
+ my $arc_max = Proxmox::Install::RunEnv::clamp_zfs_arc_max($arc_max_mib) * 1024 * 1024;
if ($arc_max > 0) {
file_write_all("$targetdir/etc/modprobe.d/zfs.conf", "options zfs zfs_arc_max=$arc_max\n")
--
2.42.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [pve-devel] [PATCH installer v4 3/6] proxinstall: expose `arc_max` ZFS option for PVE installations
2023-11-07 12:20 [pve-devel] [PATCH installer v4 0/6] fix #4829: wire up `arc_max` ZFS option to GUI/TUI for PVE Christoph Heiss
2023-11-07 12:20 ` [pve-devel] [PATCH installer v4 1/6] run env: remove debug print Christoph Heiss
2023-11-07 12:20 ` [pve-devel] [PATCH installer v4 2/6] install: use correct variable names in zfs_setup_module_conf() Christoph Heiss
@ 2023-11-07 12:20 ` Christoph Heiss
2023-11-07 12:20 ` [pve-devel] [PATCH installer v4 4/6] test: add tests for zfs_arc_max calculations Christoph Heiss
` (3 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Christoph Heiss @ 2023-11-07 12:20 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
proxinstall | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/proxinstall b/proxinstall
index 113bf37..857281d 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_MIB, $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
^ permalink raw reply [flat|nested] 9+ messages in thread* [pve-devel] [PATCH installer v4 4/6] test: add tests for zfs_arc_max calculations
2023-11-07 12:20 [pve-devel] [PATCH installer v4 0/6] fix #4829: wire up `arc_max` ZFS option to GUI/TUI for PVE Christoph Heiss
` (2 preceding siblings ...)
2023-11-07 12:20 ` [pve-devel] [PATCH installer v4 3/6] proxinstall: expose `arc_max` ZFS option for PVE installations Christoph Heiss
@ 2023-11-07 12:20 ` Christoph Heiss
2023-11-07 12:20 ` [pve-devel] [PATCH installer v4 5/6] common: add ZFS `arc_max` installer setup option Christoph Heiss
` (2 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Christoph Heiss @ 2023-11-07 12:20 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
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 e792e0e..5e94c29 100644
--- a/Makefile
+++ b/Makefile
@@ -45,6 +45,7 @@ $(BUILDDIR):
proxmox-tui-installer/ \
proxmox-installer-common/ \
spice-vdagent.sh \
+ test/ \
unconfigured.sh \
xinitrc \
$@.tmp
@@ -76,7 +77,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
^ permalink raw reply [flat|nested] 9+ messages in thread* [pve-devel] [PATCH installer v4 5/6] common: add ZFS `arc_max` installer setup option
2023-11-07 12:20 [pve-devel] [PATCH installer v4 0/6] fix #4829: wire up `arc_max` ZFS option to GUI/TUI for PVE Christoph Heiss
` (3 preceding siblings ...)
2023-11-07 12:20 ` [pve-devel] [PATCH installer v4 4/6] test: add tests for zfs_arc_max calculations Christoph Heiss
@ 2023-11-07 12:20 ` Christoph Heiss
2023-11-07 12:20 ` [pve-devel] [PATCH installer v4 6/6] tui: bootdisk: expose `arc_max` ZFS option for PVE installations Christoph Heiss
2023-11-07 15:50 ` [pve-devel] applied: [PATCH installer v4 0/6] fix #4829: wire up `arc_max` ZFS option to GUI/TUI for PVE Thomas Lamprecht
6 siblings, 0 replies; 9+ messages in thread
From: Christoph Heiss @ 2023-11-07 12:20 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
proxmox-installer-common/src/options.rs | 62 +++++++++++++++++++--
proxmox-installer-common/src/setup.rs | 2 +
proxmox-tui-installer/src/views/bootdisk.rs | 18 ++++--
3 files changed, 71 insertions(+), 11 deletions(-)
diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index 185be2e..afd12cd 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/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};
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
@@ -172,25 +174,48 @@ 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(),
}
}
}
+/// 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
+ }
+}
+
#[derive(Clone, Debug)]
pub enum AdvancedBootdiskOptions {
Lvm(LvmBootdiskOptions),
@@ -385,3 +410,30 @@ impl NetworkOptions {
})
}
}
+
+#[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);
+ }
+ }
+}
diff --git a/proxmox-installer-common/src/setup.rs b/proxmox-installer-common/src/setup.rs
index 42a9e68..28a58f3 100644
--- a/proxmox-installer-common/src/setup.rs
+++ b/proxmox-installer-common/src/setup.rs
@@ -149,6 +149,7 @@ pub struct InstallZfsOption {
#[serde(serialize_with = "serialize_as_display")]
checksum: ZfsChecksumOption,
copies: usize,
+ arc_max: usize,
}
impl From<ZfsBootdiskOptions> for InstallZfsOption {
@@ -158,6 +159,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 48ccee9..3b2242a 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -22,7 +22,7 @@ use proxmox_installer_common::{
AdvancedBootdiskOptions, BootdiskOptions, BtrfsBootdiskOptions, Disk, FsType,
LvmBootdiskOptions, ZfsBootdiskOptions, ZFS_CHECKSUM_OPTIONS, ZFS_COMPRESS_OPTIONS,
},
- setup::{BootType, ProductConfig, ProxmoxProduct},
+ setup::{BootType, ProductConfig, ProxmoxProduct, RuntimeInfo},
};
pub struct BootdiskOptionsView {
@@ -155,6 +155,7 @@ impl AdvancedBootdiskOptionsView {
fn fstype_on_submit(siv: &mut Cursive, disks: &[Disk], 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| {
@@ -166,9 +167,10 @@ impl AdvancedBootdiskOptionsView {
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::Zfs(_) => view.add_child(ZfsBootdiskOptionsView::new_with_defaults(
+ &runinfo,
+ &product_conf,
+ )),
FsType::Btrfs(_) => {
view.add_child(BtrfsBootdiskOptionsView::new_with_defaults(disks))
}
@@ -552,8 +554,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)> {
@@ -573,6 +578,7 @@ impl ZfsBootdiskOptionsView {
compress,
checksum,
copies,
+ arc_max: 0, // use built-in ZFS default value
disk_size,
selected_disks,
},
--
2.42.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [pve-devel] [PATCH installer v4 6/6] tui: bootdisk: expose `arc_max` ZFS option for PVE installations
2023-11-07 12:20 [pve-devel] [PATCH installer v4 0/6] fix #4829: wire up `arc_max` ZFS option to GUI/TUI for PVE Christoph Heiss
` (4 preceding siblings ...)
2023-11-07 12:20 ` [pve-devel] [PATCH installer v4 5/6] common: add ZFS `arc_max` installer setup option Christoph Heiss
@ 2023-11-07 12:20 ` Christoph Heiss
2023-11-07 15:50 ` [pve-devel] applied: [PATCH installer v4 0/6] fix #4829: wire up `arc_max` ZFS option to GUI/TUI for PVE Thomas Lamprecht
6 siblings, 0 replies; 9+ messages in thread
From: Christoph Heiss @ 2023-11-07 12:20 UTC (permalink / raw)
To: pve-devel
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>
---
proxmox-tui-installer/src/main.rs | 2 +-
proxmox-tui-installer/src/views/bootdisk.rs | 68 ++++++++++++++-------
proxmox-tui-installer/src/views/mod.rs | 1 -
3 files changed, 47 insertions(+), 24 deletions(-)
diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
index 2e5a194..e561d57 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -378,7 +378,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 3b2242a..5272258 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -25,6 +25,10 @@ use proxmox_installer_common::{
setup::{BootType, ProductConfig, ProxmoxProduct, RuntimeInfo},
};
+/// 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_MIB: usize = 64; // MiB
+
pub struct BootdiskOptionsView {
view: LinearLayout,
advanced_options: Rc<RefCell<BootdiskOptions>>,
@@ -32,13 +36,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");
@@ -52,11 +56,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(),
));
@@ -109,7 +113,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() };
@@ -128,10 +132,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(Self::fstype_on_submit);
let mut view = LinearLayout::vertical()
.child(DummyView.full_width())
@@ -143,17 +144,17 @@ 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))
}
};
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();
@@ -165,14 +166,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(
&runinfo,
&product_conf,
)),
FsType::Btrfs(_) => {
- view.add_child(BtrfsBootdiskOptionsView::new_with_defaults(disks))
+ view.add_child(BtrfsBootdiskOptionsView::new_with_defaults(&runinfo.disks))
}
}
}
@@ -186,7 +187,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())),
@@ -516,7 +517,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(
@@ -544,9 +551,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());
@@ -556,20 +570,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_MIB)
+ } else {
+ 0 // use built-in ZFS default value
+ };
Some((
disks,
@@ -578,7 +602,7 @@ impl ZfsBootdiskOptionsView {
compress,
checksum,
copies,
- arc_max: 0, // use built-in ZFS default value
+ arc_max,
disk_size,
selected_disks,
},
@@ -591,12 +615,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 1777c09..4d27532 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
^ permalink raw reply [flat|nested] 9+ messages in thread* [pve-devel] applied: [PATCH installer v4 0/6] fix #4829: wire up `arc_max` ZFS option to GUI/TUI for PVE
2023-11-07 12:20 [pve-devel] [PATCH installer v4 0/6] fix #4829: wire up `arc_max` ZFS option to GUI/TUI for PVE Christoph Heiss
` (5 preceding siblings ...)
2023-11-07 12:20 ` [pve-devel] [PATCH installer v4 6/6] tui: bootdisk: expose `arc_max` ZFS option for PVE installations Christoph Heiss
@ 2023-11-07 15:50 ` Thomas Lamprecht
2023-11-08 9:05 ` Christoph Heiss
6 siblings, 1 reply; 9+ messages in thread
From: Thomas Lamprecht @ 2023-11-07 15:50 UTC (permalink / raw)
To: Proxmox VE development discussion, Christoph Heiss
Am 07/11/2023 um 13:20 schrieb Christoph Heiss:
> run env: remove debug print
> install: use correct variable names in zfs_setup_module_conf()
> proxinstall: expose `arc_max` ZFS option for PVE installations
> test: add tests for zfs_arc_max calculations
> common: add ZFS `arc_max` installer setup option
> tui: bootdisk: expose `arc_max` ZFS option for PVE installations
>
applied series, thanks!
Two thoughts though:
- we could add the ARC input field for PBS and PMG too, but not use the
automatic-sizing heuristic there (i.e., still 50% memory by default)
- maybe GB with float would fit in better, but surely not perfect either,
so just for the record.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [pve-devel] applied: [PATCH installer v4 0/6] fix #4829: wire up `arc_max` ZFS option to GUI/TUI for PVE
2023-11-07 15:50 ` [pve-devel] applied: [PATCH installer v4 0/6] fix #4829: wire up `arc_max` ZFS option to GUI/TUI for PVE Thomas Lamprecht
@ 2023-11-08 9:05 ` Christoph Heiss
0 siblings, 0 replies; 9+ messages in thread
From: Christoph Heiss @ 2023-11-08 9:05 UTC (permalink / raw)
To: Thomas Lamprecht; +Cc: Proxmox VE development discussion
On Tue, Nov 07, 2023 at 04:50:31PM +0100, Thomas Lamprecht wrote:
>
> Am 07/11/2023 um 13:20 schrieb Christoph Heiss:
> > run env: remove debug print
> > install: use correct variable names in zfs_setup_module_conf()
> > proxinstall: expose `arc_max` ZFS option for PVE installations
> > test: add tests for zfs_arc_max calculations
> > common: add ZFS `arc_max` installer setup option
> > tui: bootdisk: expose `arc_max` ZFS option for PVE installations
> >
>
> applied series, thanks!
Thanks!
>
> Two thoughts though:
>
> - we could add the ARC input field for PBS and PMG too, but not use the
> automatic-sizing heuristic there (i.e., still 50% memory by default)
Makes sense, esp. for PBS due to its nature. I'll look into it sometime!
>
> - maybe GB with float would fit in better, but surely not perfect either,
> so just for the record.
Good point, but I agree, hard to judge what's better. As we are
defaulting to max. 16 GiB (for PVE), it's a rather smaller range, so MiB
makes sense IMO.
Although aligning it with all the other inputs, which are in GiB, makes
sense too. As well as inputting it as GiB float should be precise enough
for any case.
I will definitely keep it in mind for the future.
^ permalink raw reply [flat|nested] 9+ messages in thread