[pve-devel] [PATCH installer 2/2] Consider hdsize setting when comparing raid disks
Thomas Lamprecht
t.lamprecht at proxmox.com
Mon Dec 10 10:51:11 CET 2018
(resend as I previously forgot to include the mailing list ...)
On 12/7/18 12:35 PM, Stoiko Ivanov wrote:
> 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 <s.ivanov at 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
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)) {
> + } 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]);
you can use perl array slices for this, increases expressiveness, IMO:
my ($hd1, $hd2) = $devlist->[0,1];
obviously not tested and your call
> + 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];
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.
> 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]);
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 ^^
> + 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);
> }
>
>
On 12/7/18 12:35 PM, Stoiko Ivanov wrote:
> 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 <s.ivanov at 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
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)) {
> + } 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]);
you can use perl array slices for this, increases expressiveness, IMO:
my ($hd1, $hd2) = $devlist->[0,1];
obviously not tested and your call
> + 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];
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.
> 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]);
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 ^^
> + 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);
> }
>
>
More information about the pve-devel
mailing list