Discussion:
[pve-devel] [PATCH installer 0/2] fix hdsize oddities for ZFS
Stoiko Ivanov
2018-12-07 11:35:30 UTC
Permalink
The recent addtitions of honoring hdsize for ZFS, introduced a few glitches
with disks of different size (sorry for not testing this thoroughly!).

The first patch fixes #2009, where once a smaller disk was selected, the
hdsize_adjustment was restricted to that disks size, even if selecting a larger
disk later.

The second patch changes the check for similar sized disks in a mirror/RAIDZ
configuration, to take the hdsize setting into consideration.

Stoiko Ivanov (2):
Fix #2009: Recreate hdsize_adj with new hdsize
Consider hdsize setting when comparing raid disks

proxinstall | 47 ++++++++++++++++++++++++++++++-----------------
1 file changed, 30 insertions(+), 17 deletions(-)
--
2.11.0
Stoiko Ivanov
2018-12-07 11:35:31 UTC
Permalink
Creating $hdsize_size_adjustment once with a passed hdsize, breaks changing
the disk to a smaller/larger one, since the installer is stuck with the limits
set with the initial disk.

Signed-off-by: Stoiko Ivanov <***@proxmox.com>
---
proxinstall | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/proxinstall b/proxinstall
index 9945dd1..159c727 100755
--- a/proxinstall
+++ b/proxinstall
@@ -2501,11 +2501,13 @@ my $hdsize_entry_buffer;
my $get_hdsize_spinbtn = sub {
my $hdsize = shift;

- if (!defined($hdsize_size_adj)) {
- die "called get_hdsize_spinbtn with \$hdsize_size_adj not defined but did not pass hdsize!\n"
- if !defined($hdsize);
+ $hdsize_entry_buffer //= Gtk3::EntryBuffer->new(undef, 1);
+
+ if (defined($hdsize)) {
$hdsize_size_adj = Gtk3::Adjustment->new($config_options->{hdsize} || $hdsize, 0, $hdsize+1, 1, 1, 1);
- $hdsize_entry_buffer = Gtk3::EntryBuffer->new(undef, 1);
+ } else {
+ die "called get_hdsize_spinbtn with \$hdsize_size_adj not defined but did not pass hdsize!\n"
+ if !defined($hdsize_size_adj);
}

my $spinbutton_hdsize = Gtk3::SpinButton->new($hdsize_size_adj, 1, 1);
--
2.11.0
Thomas Lamprecht
2018-12-10 09:21:00 UTC
Permalink
Post by Stoiko Ivanov
Creating $hdsize_size_adjustment once with a passed hdsize, breaks changing
the disk to a smaller/larger one, since the installer is stuck with the limits
set with the initial disk.
---
proxinstall | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/proxinstall b/proxinstall
index 9945dd1..159c727 100755
--- a/proxinstall
+++ b/proxinstall
@@ -2501,11 +2501,13 @@ my $hdsize_entry_buffer;
my $get_hdsize_spinbtn = sub {
my $hdsize = shift;
- if (!defined($hdsize_size_adj)) {
- die "called get_hdsize_spinbtn with \$hdsize_size_adj not defined but did not pass hdsize!\n"
- if !defined($hdsize);
+ $hdsize_entry_buffer //= Gtk3::EntryBuffer->new(undef, 1);
+
+ if (defined($hdsize)) {
$hdsize_size_adj = Gtk3::Adjustment->new($config_options->{hdsize} || $hdsize, 0, $hdsize+1, 1, 1, 1);
- $hdsize_entry_buffer = Gtk3::EntryBuffer->new(undef, 1);
+ } else {
+ die "called get_hdsize_spinbtn with \$hdsize_size_adj not defined but did not pass hdsize!\n"
+ if !defined($hdsize_size_adj);
}
my $spinbutton_hdsize = Gtk3::SpinButton->new($hdsize_size_adj, 1, 1);
applied, thanks!

Stoiko Ivanov
2018-12-07 11:35:32 UTC
Permalink
Honoring hdsize for ZFS setups introduced the possibility to use differently
sized disks for a mirror-setup, by restricting hdsize to the smallest disk.

Signed-off-by: Stoiko Ivanov <***@proxmox.com>
---
proxinstall | 37 ++++++++++++++++++++++++-------------
1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/proxinstall b/proxinstall
index 159c727..5f83d3f 100755
--- a/proxinstall
+++ b/proxinstall
@@ -2791,10 +2791,15 @@ my $get_raid_devlist = sub {
};

sub zfs_mirror_size_check {
- my ($expected, $actual) = @_;
+ my ($expected, $actual, $restricted_size_gb) = @_;

- die "mirrored disks must have same size\n"
- if abs($expected - $actual) > $expected / 10;
+ if (defined($restricted_size_gb)) {
+ die "bootdisks must have at least hdsize: $restricted_size_gb GB\n"
+ if ($actual < ($restricted_size_gb * 1024 * 1024 * 2)); #$actual is in 512b
+ } else {
+ die "mirrored disks must have same size\n"
+ if abs($expected - $actual) > $expected / 10;
+ }
}

sub get_zfs_raid_setup {
@@ -2817,10 +2822,7 @@ sub get_zfs_raid_setup {
} elsif ($filesys eq 'zfs (RAID1)') {
die "zfs (RAID1) needs at least 2 device\n" if $diskcount < 2;
$cmd .= ' mirror ';
- my $hd = @$devlist[0];
- my $expected_size = @$hd[2]; # all disks need approximately same size
- foreach $hd (@$devlist) {
- zfs_mirror_size_check($expected_size, @$hd[2]);
+ foreach my $hd (@$devlist) {
$cmd .= " @$hd[1]";
push @$bootdevlist, $hd;
}
@@ -2828,9 +2830,12 @@ sub get_zfs_raid_setup {
die "zfs (RAID10) needs at least 4 device\n" if $diskcount < 4;
die "zfs (RAID10) needs an even number of devices\n" if $diskcount & 1;

- push @$bootdevlist, @$devlist[0], @$devlist[1];
+ my ($hd1, $hd2) = (@$devlist[0], @$devlist[1]);
+ push @$bootdevlist, $hd1, $hd2;
+ $cmd .= ' mirror ' . @$hd1[1] . ' ' . @$hd2[1];

- for (my $i = 0; $i < $diskcount; $i+=2) {
+ # first 2 (boot) disks get checked separately
+ for (my $i = 2; $i < $diskcount; $i+=2) {
my $hd1 = @$devlist[$i];
my $hd2 = @$devlist[$i+1];
zfs_mirror_size_check(@$hd1[2], @$hd2[2]); # pairs need approximately same size
@@ -2841,11 +2846,8 @@ sub get_zfs_raid_setup {
my $level = $1;
my $mindisks = 2 + $level;
die "zfs (RAIDZ-$level) needs at least $mindisks devices\n" if scalar(@$devlist) < $mindisks;
- my $hd = @$devlist[0];
- my $expected_size = @$hd[2]; # all disks need approximately same size
$cmd .= " raidz$level";
- foreach $hd (@$devlist) {
- zfs_mirror_size_check($expected_size, @$hd[2]);
+ foreach my $hd (@$devlist) {
$cmd .= " @$hd[1]";
push @$bootdevlist, $hd;
}
@@ -2853,6 +2855,15 @@ sub get_zfs_raid_setup {
die "unknown zfs mode '$filesys'\n";
}

+ #bootdisks honor hdsize setting if present
+ my $restricted_size_gb = $config_options->{hdsize};
+
+ my $hd = @$bootdevlist[0];
+ my $expected_size = @$hd[2];
+ foreach $hd (@$bootdevlist) {
+ zfs_mirror_size_check($expected_size, @$hd[2], $restricted_size_gb);
+ }
+
return ($devlist, $bootdevlist, $cmd);
}
--
2.11.0
Thomas Lamprecht
2018-12-10 09:51:11 UTC
Permalink
(resend as I previously forgot to include the mailing list ...)
Post by Stoiko Ivanov
Honoring hdsize for ZFS setups introduced the possibility to use differently
sized disks for a mirror-setup, by restricting hdsize to the smallest disk.
---
proxinstall | 37 ++++++++++++++++++++++++-------------
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/proxinstall b/proxinstall
index 159c727..5f83d3f 100755
--- a/proxinstall
+++ b/proxinstall
@@ -2791,10 +2791,15 @@ my $get_raid_devlist = sub {
};
sub zfs_mirror_size_check {
- die "mirrored disks must have same size\n"
- if abs($expected - $actual) > $expected / 10;
+ if (defined($restricted_size_gb)) {
+ die "bootdisks must have at least hdsize: $restricted_size_gb GB\n"
+ if ($actual < ($restricted_size_gb * 1024 * 1024 * 2)); #$actual is in 512b
I'd rather not have this in a post if, but a "normal" one. also the comment could
be something like # $actual is in multiple of 512 bytes

and are you sure the calculation is correct? It could be nicer to get both sizes to
the same "standard" unit, i.e., KiB in this case not 512b, e.g.:

if ($actual * 2 < ($restricted_size_gb * 1024 * 1024)) {
Post by Stoiko Ivanov
+ } else {
+ die "mirrored disks must have same size\n"
+ if abs($expected - $actual) > $expected / 10;
+ }
}
sub get_zfs_raid_setup {
@@ -2817,10 +2822,7 @@ sub get_zfs_raid_setup {
} elsif ($filesys eq 'zfs (RAID1)') {
die "zfs (RAID1) needs at least 2 device\n" if $diskcount < 2;
$cmd .= ' mirror ';
}
@@ -2828,9 +2830,12 @@ sub get_zfs_raid_setup {
die "zfs (RAID10) needs at least 4 device\n" if $diskcount < 4;
die "zfs (RAID10) needs an even number of devices\n" if $diskcount & 1;
you can use perl array slices for this, increases expressiveness, IMO:
my ($hd1, $hd2) = $devlist->[0,1];
obviously not tested and your call
Post by Stoiko Ivanov
- for (my $i = 0; $i < $diskcount; $i+=2) {
+ # first 2 (boot) disks get checked separately
+ for (my $i = 2; $i < $diskcount; $i+=2) {
you redefine $hd1 and $hd2 here, maybe just reuse above ones? I'd like to avoid
such things, if not necessary, they tend to complicate things and introduce harder
to understand behaviour.
Post by Stoiko Ivanov
@@ -2841,11 +2846,8 @@ sub get_zfs_raid_setup {
my $level = $1;
my $mindisks = 2 + $level;
$cmd .= " raidz$level";
maybe also a comment here that all idsks are boot disks and thus all get checked below?

looks OK, besides above, albeit I still need to think a bit about all possible
combinations ^^
Post by Stoiko Ivanov
}
@@ -2853,6 +2855,15 @@ sub get_zfs_raid_setup {
die "unknown zfs mode '$filesys'\n";
}
+ #bootdisks honor hdsize setting if present
+ my $restricted_size_gb = $config_options->{hdsize};
+
+ }
+
return ($devlist, $bootdevlist, $cmd);
}
Honoring hdsize for ZFS setups introduced the possibility to use differently
sized disks for a mirror-setup, by restricting hdsize to the smallest disk.
---
proxinstall | 37 ++++++++++++++++++++++++-------------
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/proxinstall b/proxinstall
index 159c727..5f83d3f 100755
--- a/proxinstall
+++ b/proxinstall
@@ -2791,10 +2791,15 @@ my $get_raid_devlist = sub {
};
sub zfs_mirror_size_check {
- die "mirrored disks must have same size\n"
- if abs($expected - $actual) > $expected / 10;
+ if (defined($restricted_size_gb)) {
+ die "bootdisks must have at least hdsize: $restricted_size_gb GB\n"
+ if ($actual < ($restricted_size_gb * 1024 * 1024 * 2)); #$actual is in 512b
I'd rather not have this in a post if, but a "normal" one. also the comment could
be something like # $actual is in multiple of 512 bytes

and are you sure the calculation is correct? It could be nicer to get both sizes to
the same "standard" unit, i.e., KiB in this case not 512b, e.g.:

if ($actual * 2 < ($restricted_size_gb * 1024 * 1024)) {
Post by Stoiko Ivanov
+ } else {
+ die "mirrored disks must have same size\n"
+ if abs($expected - $actual) > $expected / 10;
+ }
}
sub get_zfs_raid_setup {
@@ -2817,10 +2822,7 @@ sub get_zfs_raid_setup {
} elsif ($filesys eq 'zfs (RAID1)') {
die "zfs (RAID1) needs at least 2 device\n" if $diskcount < 2;
$cmd .= ' mirror ';
}
@@ -2828,9 +2830,12 @@ sub get_zfs_raid_setup {
die "zfs (RAID10) needs at least 4 device\n" if $diskcount < 4;
die "zfs (RAID10) needs an even number of devices\n" if $diskcount & 1;
you can use perl array slices for this, increases expressiveness, IMO:
my ($hd1, $hd2) = $devlist->[0,1];
obviously not tested and your call
Post by Stoiko Ivanov
- for (my $i = 0; $i < $diskcount; $i+=2) {
+ # first 2 (boot) disks get checked separately
+ for (my $i = 2; $i < $diskcount; $i+=2) {
you redefine $hd1 and $hd2 here, maybe just reuse above ones? I'd like to avoid
such things, if not necessary, they tend to complicate things and introduce harder
to understand behaviour.
Post by Stoiko Ivanov
@@ -2841,11 +2846,8 @@ sub get_zfs_raid_setup {
my $level = $1;
my $mindisks = 2 + $level;
$cmd .= " raidz$level";
maybe also a comment here that all idsks are boot disks and thus all get checked below?

looks OK, besides above, albeit I still need to think a bit about all possible
combinations ^^
Post by Stoiko Ivanov
}
@@ -2853,6 +2855,15 @@ sub get_zfs_raid_setup {
die "unknown zfs mode '$filesys'\n";
}
+ #bootdisks honor hdsize setting if present
+ my $restricted_size_gb = $config_options->{hdsize};
+
+ }
+
return ($devlist, $bootdevlist, $cmd);
}
Loading...