[pve-devel] [PATCH installer v2 0/5] fix #5250: add btrfs `compress` mount option support

[pve-devel] [PATCH installer v2 0/5] fix #5250: add btrfs `compress` mount option support

[Christoph Heiss]

Fixes #5250 [0].

Pretty much as it says on the tin, this adds the `compress` mount option
for Btrfs much in the same way as the ZFS equivalent.

W.r.t to the discussion in #5250 - `compress` is used. For a detailed
explanation, see patch #2.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=5250

History
=======
v1: https://lists.proxmox.com/pipermail/pve-devel/2024-May/063845.html

Notable changes v1 -> v2:
  * rebased on latest master
  * squashed auto-installer tests into feature patch

Diffstat
========

Christoph Heiss (5):
  fix #5250: install: config: add new `btrfs_opts` with `compress`
    config option
  fix #5250: install: write btrfs `compress` option to fstab
  fix #5250: proxinstall: expose new btrfs `compress` option
  fix #5250: tui: expose new btrfs `compress` option
  fix #5250: auto-installer: expose new btrfs `compress` option

 Proxmox/Install.pm                            | 11 +++++-
 Proxmox/Install/Config.pm                     | 15 ++++++++
 proxinstall                                   | 15 ++++++++
 proxmox-auto-installer/src/answer.rs          |  6 +++-
 proxmox-auto-installer/src/utils.rs           |  7 +++-
 .../tests/resources/parse_answer/btrfs.json   | 24 +++++++++++++
 .../tests/resources/parse_answer/btrfs.toml   | 17 ++++++++++
 proxmox-installer-common/src/options.rs       | 31 +++++++++++++++++
 proxmox-installer-common/src/setup.rs         | 21 ++++++++++--
 proxmox-tui-installer/src/setup.rs            |  2 ++
 proxmox-tui-installer/src/views/bootdisk.rs   | 34 +++++++++++++------
 11 files changed, 167 insertions(+), 16 deletions(-)
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer/btrfs.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer/btrfs.toml

--
2.44.0



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

[pve-devel] [PATCH installer v2 1/5] fix #5250: install: config: add new `btrfs_opts` with `compress` config option

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * no changes

 Proxmox/Install/Config.pm                   | 15 ++++++++++
 proxmox-auto-installer/src/utils.rs         |  1 +
 proxmox-installer-common/src/options.rs     | 31 +++++++++++++++++++++
 proxmox-installer-common/src/setup.rs       | 21 ++++++++++++--
 proxmox-tui-installer/src/setup.rs          |  2 ++
 proxmox-tui-installer/src/views/bootdisk.rs |  5 ++--
 6 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/Proxmox/Install/Config.pm b/Proxmox/Install/Config.pm
index ae70093..7e67f69 100644
--- a/Proxmox/Install/Config.pm
+++ b/Proxmox/Install/Config.pm
@@ -79,6 +79,9 @@ my sub init_cfg {
 	    copies => 1,
 	    arc_max => Proxmox::Install::RunEnv::default_zfs_arc_max(), # in MiB
 	},
+	btrfs_opts => {
+	    compress => 'off',
+	},
 	# TODO: single disk selection config
 	target_hd => undef,
 	disk_selection => {},
@@ -173,6 +176,18 @@ sub get_zfs_opt {
     return defined($k) ? $zfs_opts->{$k} : $zfs_opts;
 }
 
+sub set_btrfs_opt {
+    my ($k, $v) = @_;
+    my $opts = get('btrfs_opts');
+    croak "unknown btrfs opts key '$k'" if !exists($opts->{$k});
+    $opts->{$k} = $v;
+}
+sub get_btrfs_opt {
+    my ($k) = @_;
+    my $opts = get('btrfs_opts');
+    return defined($k) ? $opts->{$k} : $opts;
+}
+
 sub set_target_hd { set_key('target_hd', $_[0]); }
 sub get_target_hd { return get('target_hd'); }
 
diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index 45ad222..ae7dbbd 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -340,6 +340,7 @@ pub fn parse_answer(
         minfree: None,
         maxvz: None,
         zfs_opts: None,
+        btrfs_opts: None,
         target_hd: None,
         disk_selection: BTreeMap::new(),
         existing_storage_auto_rename: 1,
diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index 9375ded..d99e26d 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -102,10 +102,40 @@ impl LvmBootdiskOptions {
     }
 }
 
+/// See the accompanying mount option in btrfs(5).
+#[derive(Copy, Clone, Debug, Default, Deserialize, Eq, PartialEq)]
+#[serde(rename_all(deserialize = "lowercase"))]
+pub enum BtrfsCompressOption {
+    On,
+    #[default]
+    Off,
+    Zlib,
+    Lzo,
+    Zstd,
+}
+
+impl fmt::Display for BtrfsCompressOption {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{}", format!("{self:?}").to_lowercase())
+    }
+}
+
+impl From<&BtrfsCompressOption> for String {
+    fn from(value: &BtrfsCompressOption) -> Self {
+        value.to_string()
+    }
+}
+
+pub const BTRFS_COMPRESS_OPTIONS: &[BtrfsCompressOption] = {
+    use BtrfsCompressOption::*;
+    &[On, Off, Zlib, Lzo, Zstd]
+};
+
 #[derive(Clone, Debug)]
 pub struct BtrfsBootdiskOptions {
     pub disk_size: f64,
     pub selected_disks: Vec<usize>,
+    pub compress: BtrfsCompressOption,
 }
 
 impl BtrfsBootdiskOptions {
@@ -115,6 +145,7 @@ impl BtrfsBootdiskOptions {
         Self {
             disk_size: disk.size,
             selected_disks: (0..disks.len()).collect(),
+            compress: BtrfsCompressOption::default(),
         }
     }
 }
diff --git a/proxmox-installer-common/src/setup.rs b/proxmox-installer-common/src/setup.rs
index e4e609b..d7f1c47 100644
--- a/proxmox-installer-common/src/setup.rs
+++ b/proxmox-installer-common/src/setup.rs
@@ -13,8 +13,8 @@ use serde::{de, Deserialize, Deserializer, Serialize, Serializer};
 
 use crate::{
     options::{
-        BtrfsRaidLevel, Disk, FsType, ZfsBootdiskOptions, ZfsChecksumOption, ZfsCompressOption,
-        ZfsRaidLevel,
+        BtrfsBootdiskOptions, BtrfsCompressOption, BtrfsRaidLevel, Disk, FsType,
+        ZfsBootdiskOptions, ZfsChecksumOption, ZfsCompressOption, ZfsRaidLevel,
     },
     utils::CidrAddress,
 };
@@ -221,6 +221,20 @@ impl From<ZfsBootdiskOptions> for InstallZfsOption {
     }
 }
 
+#[derive(Debug, Deserialize, Serialize)]
+pub struct InstallBtrfsOption {
+    #[serde(serialize_with = "serialize_as_display")]
+    pub compress: BtrfsCompressOption,
+}
+
+impl From<BtrfsBootdiskOptions> for InstallBtrfsOption {
+    fn from(opts: BtrfsBootdiskOptions) -> Self {
+        InstallBtrfsOption {
+            compress: opts.compress,
+        }
+    }
+}
+
 pub fn read_json<T: for<'de> Deserialize<'de>, P: AsRef<Path>>(path: P) -> Result<T, String> {
     let file = File::open(path).map_err(|err| err.to_string())?;
     let reader = BufReader::new(file);
@@ -475,6 +489,9 @@ pub struct InstallConfig {
     #[serde(skip_serializing_if = "Option::is_none")]
     pub zfs_opts: Option<InstallZfsOption>,
 
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub btrfs_opts: Option<InstallBtrfsOption>,
+
     #[serde(
         serialize_with = "serialize_disk_opt",
         skip_serializing_if = "Option::is_none",
diff --git a/proxmox-tui-installer/src/setup.rs b/proxmox-tui-installer/src/setup.rs
index ee4e1c7..70b8e28 100644
--- a/proxmox-tui-installer/src/setup.rs
+++ b/proxmox-tui-installer/src/setup.rs
@@ -18,6 +18,7 @@ impl From<InstallerOptions> for InstallConfig {
             minfree: None,
             maxvz: None,
             zfs_opts: None,
+            btrfs_opts: None,
             target_hd: None,
             disk_selection: BTreeMap::new(),
             existing_storage_auto_rename: 0,
@@ -66,6 +67,7 @@ impl From<InstallerOptions> for InstallConfig {
             }
             AdvancedBootdiskOptions::Btrfs(btrfs) => {
                 config.hdsize = btrfs.disk_size;
+                config.btrfs_opts = Some(btrfs.clone().into());
 
                 for (i, disk) in options.bootdisk.disks.iter().enumerate() {
                     config
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 1e22105..8db33dd 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -19,8 +19,8 @@ use proxmox_installer_common::{
         check_zfs_raid_config,
     },
     options::{
-        AdvancedBootdiskOptions, BootdiskOptions, BtrfsBootdiskOptions, Disk, FsType,
-        LvmBootdiskOptions, ZfsBootdiskOptions, ZFS_CHECKSUM_OPTIONS, ZFS_COMPRESS_OPTIONS,
+        AdvancedBootdiskOptions, BootdiskOptions, BtrfsBootdiskOptions, BtrfsCompressOption, Disk,
+        FsType, LvmBootdiskOptions, ZfsBootdiskOptions, ZFS_CHECKSUM_OPTIONS, ZFS_COMPRESS_OPTIONS,
     },
     setup::{BootType, ProductConfig, ProxmoxProduct, RuntimeInfo},
 };
@@ -602,6 +602,7 @@ impl BtrfsBootdiskOptionsView {
             BtrfsBootdiskOptions {
                 disk_size,
                 selected_disks,
+                compress: BtrfsCompressOption::default(),
             },
         ))
     }
-- 
2.45.2



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

[pve-devel] [PATCH installer v2 2/5] fix #5250: install: write btrfs `compress` option to fstab

[Christoph Heiss]

`compress` instead of `compress-force` is used, as the latter can have
unindented (performance) implications, as the name implies. That would
be neither expected by users nor should such a decision made without the
user explicitly opting for it.

Others do the same, e.g. the installer for RedHat/Fedora systems (aka.
Anaconda) opts for `compress` too.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * no changes
  * moved some mount setup code here from next patch

 Proxmox/Install.pm | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index fa2702c..5c64c3d 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -1065,7 +1065,16 @@ sub extract_data {
 
 	    die "unable to detect FS UUID" if !defined($fsuuid);
 
-	    $fstab .= "UUID=$fsuuid / btrfs defaults 0 1\n";
+	    my $btrfs_opts = Proxmox::Install::Config::get_btrfs_opt();
+
+	    my $mountopts = 'defaults';
+	    if ($btrfs_opts->{compress} eq 'on') {
+		$mountopts .= ',compress';
+	    } elsif ($btrfs_opts->{compress} ne 'off') {
+		$mountopts .= ",compress=$btrfs_opts->{compress}";
+	    }
+
+	    $fstab .= "UUID=$fsuuid / btrfs $mountopts 0 1\n";
 	} else {
 	    my $root_mountopt = $fssetup->{$filesys}->{root_mountopt} || 'defaults';
 	    $fstab .= "$rootdev / $filesys ${root_mountopt} 0 1\n";
-- 
2.45.2



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

[pve-devel] [PATCH installer v2 3/5] fix #5250: proxinstall: expose new btrfs `compress` option

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * no changes
  * moved some mount option setup code to previous patch

 proxinstall | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/proxinstall b/proxinstall
index 12f3eaa..c7776f0 100755
--- a/proxinstall
+++ b/proxinstall
@@ -1160,6 +1160,21 @@ my $create_raid_advanced_grid = sub {
 my $create_btrfs_raid_advanced_grid = sub {
     my ($hdsize_btn) = @_;
     my $labeled_widgets = [];
+
+    my $combo_compress = Gtk3::ComboBoxText->new();
+    $combo_compress->set_tooltip_text("btrfs compression algorithm for boot volume");
+    my $comp_opts = ["on", "off", "zlib", "lzo", "zstd"];
+    foreach my $opt (@$comp_opts) {
+	$combo_compress->append($opt, $opt);
+    }
+    my $compress = Proxmox::Install::Config::get_btrfs_opt('compress') // 'off';
+    $combo_compress->set_active_id($compress);
+    $combo_compress->signal_connect (changed => sub {
+	my $w = shift;
+	Proxmox::Install::Config::set_btrfs_opt('compress', $w->get_active_text());
+    });
+    push @$labeled_widgets, ['compress', $combo_compress];
+
     push @$labeled_widgets, ['hdsize', $hdsize_btn, 'GB'];
     return $create_label_widget_grid->($labeled_widgets);;
 };
-- 
2.45.2



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

[pve-devel] [PATCH installer v2 4/5] fix #5250: tui: expose new btrfs `compress` option

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * rebased on master

 proxmox-tui-installer/src/views/bootdisk.rs | 37 +++++++++++++--------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 8db33dd..625374e 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -19,8 +19,9 @@ use proxmox_installer_common::{
         check_zfs_raid_config,
     },
     options::{
-        AdvancedBootdiskOptions, BootdiskOptions, BtrfsBootdiskOptions, BtrfsCompressOption, Disk,
-        FsType, LvmBootdiskOptions, ZfsBootdiskOptions, ZFS_CHECKSUM_OPTIONS, ZFS_COMPRESS_OPTIONS,
+        AdvancedBootdiskOptions, BootdiskOptions, BtrfsBootdiskOptions, Disk, FsType,
+        LvmBootdiskOptions, ZfsBootdiskOptions, BTRFS_COMPRESS_OPTIONS, ZFS_CHECKSUM_OPTIONS,
+        ZFS_COMPRESS_OPTIONS,
     },
     setup::{BootType, ProductConfig, ProxmoxProduct, RuntimeInfo},
 };
@@ -573,12 +574,23 @@ struct BtrfsBootdiskOptionsView {
 
 impl BtrfsBootdiskOptionsView {
     fn new(runinfo: &RuntimeInfo, options: &BtrfsBootdiskOptions) -> Self {
-        let view = MultiDiskOptionsView::new(
-            &runinfo.disks,
-            &options.selected_disks,
-            FormView::new().child("hdsize", DiskSizeEditView::new().content(options.disk_size)),
-        )
-        .top_panel(TextView::new("Btrfs integration is a technology preview!").center());
+        let inner = FormView::new()
+            .child(
+                "compress",
+                SelectView::new()
+                    .popup()
+                    .with_all(BTRFS_COMPRESS_OPTIONS.iter().map(|o| (o.to_string(), *o)))
+                    .selected(
+                        BTRFS_COMPRESS_OPTIONS
+                            .iter()
+                            .position(|o| *o == options.compress)
+                            .unwrap_or_default(),
+                    ),
+            )
+            .child("hdsize", DiskSizeEditView::new().content(options.disk_size));
+
+        let view = MultiDiskOptionsView::new(&runinfo.disks, &options.selected_disks, inner)
+            .top_panel(TextView::new("Btrfs integration is a technology preview!").center());
 
         Self { view }
     }
@@ -592,17 +604,16 @@ impl BtrfsBootdiskOptionsView {
 
     fn get_values(&mut self) -> Option<(Vec<Disk>, BtrfsBootdiskOptions)> {
         let (disks, selected_disks) = self.view.get_disks_and_selection()?;
-        let disk_size = self
-            .view
-            .get_options_view()?
-            .get_value::<DiskSizeEditView, _>(0)?;
+        let view = self.view.get_options_view()?;
+        let compress = view.get_value::<SelectView<_>, _>(0)?;
+        let disk_size = view.get_value::<DiskSizeEditView, _>(1)?;
 
         Some((
             disks,
             BtrfsBootdiskOptions {
                 disk_size,
                 selected_disks,
-                compress: BtrfsCompressOption::default(),
+                compress,
             },
         ))
     }
-- 
2.45.2



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

[pve-devel] [PATCH installer v2 5/5] fix #5250: auto-installer: expose new btrfs `compress` option

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * squashed in separate tests patch

 proxmox-auto-installer/src/answer.rs          |  6 ++++-
 proxmox-auto-installer/src/utils.rs           |  6 ++++-
 .../tests/resources/parse_answer/btrfs.json   | 24 +++++++++++++++++++
 .../tests/resources/parse_answer/btrfs.toml   | 17 +++++++++++++
 4 files changed, 51 insertions(+), 2 deletions(-)
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer/btrfs.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer/btrfs.toml

diff --git a/proxmox-auto-installer/src/answer.rs b/proxmox-auto-installer/src/answer.rs
index d691da1..fd0ed0d 100644
--- a/proxmox-auto-installer/src/answer.rs
+++ b/proxmox-auto-installer/src/answer.rs
@@ -1,6 +1,9 @@
 use clap::ValueEnum;
 use proxmox_installer_common::{
-    options::{BtrfsRaidLevel, FsType, ZfsChecksumOption, ZfsCompressOption, ZfsRaidLevel},
+    options::{
+        BtrfsCompressOption, BtrfsRaidLevel, FsType, ZfsChecksumOption, ZfsCompressOption,
+        ZfsRaidLevel,
+    },
     utils::{CidrAddress, Fqdn},
 };
 use serde::{Deserialize, Serialize};
@@ -270,6 +273,7 @@ pub struct LvmOptions {
 pub struct BtrfsOptions {
     pub hdsize: Option<f64>,
     pub raid: Option<BtrfsRaidLevel>,
+    pub compress: Option<BtrfsCompressOption>,
 }
 
 #[derive(Clone, Deserialize, Serialize, Debug, PartialEq)]
diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index ae7dbbd..9e86053 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -11,7 +11,8 @@ use crate::{
 use proxmox_installer_common::{
     options::{FsType, NetworkOptions, ZfsChecksumOption, ZfsCompressOption},
     setup::{
-        InstallConfig, InstallRootPassword, InstallZfsOption, LocaleInfo, RuntimeInfo, SetupInfo,
+        InstallBtrfsOption, InstallConfig, InstallRootPassword, InstallZfsOption, LocaleInfo,
+        RuntimeInfo, SetupInfo,
     },
 };
 use serde::{Deserialize, Serialize};
@@ -394,6 +395,9 @@ pub fn parse_answer(
             config.hdsize = btrfs
                 .hdsize
                 .unwrap_or(runtime_info.disks[first_selected_disk].size);
+            config.btrfs_opts = Some(InstallBtrfsOption {
+                compress: btrfs.compress.unwrap_or_default(),
+            })
         }
     }
     Ok(config)
diff --git a/proxmox-auto-installer/tests/resources/parse_answer/btrfs.json b/proxmox-auto-installer/tests/resources/parse_answer/btrfs.json
new file mode 100644
index 0000000..0330a38
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer/btrfs.json
@@ -0,0 +1,24 @@
+{
+  "autoreboot": 1,
+  "cidr": "192.168.1.114/24",
+  "country": "at",
+  "dns": "192.168.1.254",
+  "domain": "testinstall",
+  "disk_selection": {
+    "6": "6",
+    "7": "7"
+  },
+  "filesys": "btrfs (RAID1)",
+  "gateway": "192.168.1.1",
+  "hdsize": 80.0,
+  "existing_storage_auto_rename": 1,
+  "hostname": "pveauto",
+  "keymap": "de",
+  "mailto": "mail@no.invalid",
+  "mngmt_nic": "eno1",
+  "root_password": { "plain": "123456" },
+  "timezone": "Europe/Vienna",
+  "btrfs_opts": {
+    "compress": "zlib"
+  }
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer/btrfs.toml b/proxmox-auto-installer/tests/resources/parse_answer/btrfs.toml
new file mode 100644
index 0000000..8fcd27d
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer/btrfs.toml
@@ -0,0 +1,17 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "pveauto.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root_password = "123456"
+
+[network]
+source = "from-dhcp"
+
+[disk-setup]
+filesystem = "btrfs"
+btrfs.raid = "raid1"
+btrfs.compress = "zlib"
+btrfs.hdsize = 80
+disk_list = ["sda", "sdb"]
-- 
2.45.2



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