[pve-devel] [RFC PATCH installer] proxinstall, common: remove deprecated fletcher2 as zfs checksum algorithm

[pve-devel] [RFC PATCH installer] proxinstall, common: remove deprecated fletcher2 as zfs checksum algorithm

[Christoph Heiss]

Fletcher-2 has long been deprecated and should not be used anymore
[0][1], so we probably should not offer it anymore too. It's been
deprecated since at least over 3 years, beyond that it's hard to find an
exact date.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>

[0] https://openzfs.github.io/openzfs-docs/Basic%20Concepts/Checksums.html#checksum-algorithms
[1] https://people.freebsd.org/~asomers/fletcher.pdf
---
Sending this as RFC since it might warrant some discussions.

While digging into this a bit more, I reckon we should remove 'off' as
an option too. It's considered an "extraordinarily bad idea" [2] (for to
pretty obvious reason) and nobody should ever use it.

Might be a good idea, just so that users simply disable checksum "for
performance reasons" without knowing about the implications of this.

[2] https://openzfs.github.io/openzfs-docs/Basic%20Concepts/Checksums.html#disabling-checksums

 proxinstall                             | 2 +-
 proxmox-installer-common/src/options.rs | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/proxinstall b/proxinstall
index 01d4cfe..d6d5acb 100755
--- a/proxinstall
+++ b/proxinstall
@@ -1145,7 +1145,7 @@ my $create_raid_advanced_grid = sub {

     my $combo_checksum = Gtk3::ComboBoxText->new();
     $combo_checksum->set_tooltip_text("zfs checksum algorithm for rpool dataset");
-    my $csum_opts = ["on", "off","fletcher2", "fletcher4", "sha256"];
+    my $csum_opts = ["on", "off", "fletcher4", "sha256"];
     foreach my $opt (@$csum_opts) {
 	$combo_checksum->append($opt, $opt);
     }
diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index afd12cd..98cd907 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -146,7 +146,6 @@ pub enum ZfsChecksumOption {
     #[default]
     On,
     Off,
-    Fletcher2,
     Fletcher4,
     Sha256,
 }
@@ -165,7 +164,7 @@ impl From<&ZfsChecksumOption> for String {

 pub const ZFS_CHECKSUM_OPTIONS: &[ZfsChecksumOption] = {
     use ZfsChecksumOption::*;
-    &[On, Off, Fletcher2, Fletcher4, Sha256]
+    &[On, Off, Fletcher4, Sha256]
 };

 #[derive(Clone, Debug)]
--
2.42.0

[pve-devel] applied: [RFC PATCH installer] proxinstall, common: remove deprecated fletcher2 as zfs checksum algorithm

[Thomas Lamprecht]

Am 22/12/2023 um 11:52 schrieb Christoph Heiss:
> Fletcher-2 has long been deprecated and should not be used anymore
> [0][1], so we probably should not offer it anymore too. It's been
> deprecated since at least over 3 years, beyond that it's hard to find an
> exact date.
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> 
> [0] https://openzfs.github.io/openzfs-docs/Basic%20Concepts/Checksums.html#checksum-algorithms
> [1] https://people.freebsd.org/~asomers/fletcher.pdf

please keep the git-trailes, like your S-o-b, always at the bottom.
References like these above are part of the commit message, not the
Developer Certificate of Origin S-o-b or patch meta stuff like R-b
or T-b trailers.

> ---
> Sending this as RFC since it might warrant some discussions.
> 
> While digging into this a bit more, I reckon we should remove 'off' as
> an option too. It's considered an "extraordinarily bad idea" [2] (for to
> pretty obvious reason) and nobody should ever use it.
> 
> Might be a good idea, just so that users simply disable checksum "for
> performance reasons" without knowing about the implications of this.

yeah, would make sense, especially as anybody can still disable those
afterwards using the `zfs` CLI tool, for whatever reason that may be.

The alternative would be either showing a hint if `off` is selected,
or change the combobox entry label of it to something like:
`off (NOT recommended)`

But just to mention all options, I'm fine with this being dropped,
if lots of users complain with somewhat reasonable justification then
we could always re-add it with aforementioned hint.

> [2] https://openzfs.github.io/openzfs-docs/Basic%20Concepts/Checksums.html#disabling-checksums
> 
>  proxinstall                             | 2 +-
>  proxmox-installer-common/src/options.rs | 3 +--
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
>

applied, thanks! 

Re: [pve-devel] applied: [RFC PATCH installer] proxinstall, common: remove deprecated fletcher2 as zfs checksum algorithm

[Christoph Heiss]

On Tue, Feb 06, 2024 at 01:17:57PM +0100, Thomas Lamprecht wrote:
> Am 22/12/2023 um 11:52 schrieb Christoph Heiss:
> > Fletcher-2 has long been deprecated and should not be used anymore
> > [0][1], so we probably should not offer it anymore too. It's been
> > deprecated since at least over 3 years, beyond that it's hard to find an
> > exact date.
> >
> > Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> >
> > [0] https://openzfs.github.io/openzfs-docs/Basic%20Concepts/Checksums.html#checksum-algorithms
> > [1] https://people.freebsd.org/~asomers/fletcher.pdf
>
> please keep the git-trailes, like your S-o-b, always at the bottom.
> References like these above are part of the commit message, not the
> Developer Certificate of Origin S-o-b or patch meta stuff like R-b
> or T-b trailers.

Oh right, sorry for that.

>
> > ---
> > Sending this as RFC since it might warrant some discussions.
> >
> > While digging into this a bit more, I reckon we should remove 'off' as
> > an option too. It's considered an "extraordinarily bad idea" [2] (for to
> > pretty obvious reason) and nobody should ever use it.
> >
> > Might be a good idea, just so that users simply disable checksum "for
> > performance reasons" without knowing about the implications of this.
>
> yeah, would make sense, especially as anybody can still disable those
> afterwards using the `zfs` CLI tool, for whatever reason that may be.
>
> The alternative would be either showing a hint if `off` is selected,
> or change the combobox entry label of it to something like:
> `off (NOT recommended)`
>
> But just to mention all options, I'm fine with this being dropped,
> if lots of users complain with somewhat reasonable justification then
> we could always re-add it with aforementioned hint.

Sounds good. I'd then prefer to just drop it as well, since setting it
to "off" is definitely something that should only be done with _very_
good reasoning anyway.

I'll send a patch.

>
> > [2] https://openzfs.github.io/openzfs-docs/Basic%20Concepts/Checksums.html#disabling-checksums
> >
> >  proxinstall                             | 2 +-
> >  proxmox-installer-common/src/options.rs | 3 +--
> >  2 files changed, 2 insertions(+), 3 deletions(-)
> >
> >
>
> applied, thanks!