[pve-devel] [PATCH SERIES storage/manager/docs 0/3] add ZFS dRAID creation

[pve-devel] [PATCH SERIES storage/manager/docs 0/3] add ZFS dRAID creation

[Stefan Hrdlicka]

The patch series adds dRAID configuration to the API and WebGUI.
Besides that there is an update to the documenation adding some basic
info about dRAID.

--

 PVE/API2/Disks/ZFS.pm | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

--

 www/manager6/node/ZFS.js | 47 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

--

 local-zfs.adoc | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

--


-- 
2.30.2

[pve-devel] [PATCH pve-storage 1/3] fix #3967: enable ZFS dRAID creation via API

[Stefan Hrdlicka]

It is possible to set the number of spares and the size of
data stripes via draidspares & dreaddata parameters.

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 PVE/API2/Disks/ZFS.pm | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm
index eeb9f48..63946d2 100644
--- a/PVE/API2/Disks/ZFS.pm
+++ b/PVE/API2/Disks/ZFS.pm
@@ -299,12 +299,27 @@ __PACKAGE__->register_method ({
 	    raidlevel => {
 		type => 'string',
 		description => 'The RAID level to use.',
-		enum => ['single', 'mirror', 'raid10', 'raidz', 'raidz2', 'raidz3'],
+		enum => ['single', 'mirror',
+		    'raid10', 'raidz', 'raidz2', 'raidz3',
+		    'draid', 'draid2', 'draid3',
+		],
 	    },
 	    devices => {
 		type => 'string', format => 'string-list',
 		description => 'The block devices you want to create the zpool on.',
 	    },
+	    draiddata => {
+		type => 'integer',
+		minimum => 1,
+		optional => 1,
+		description => 'Number of dRAID data stripes.',
+	    },
+	    draidspares => {
+		type => 'integer',
+		minimum => 0,
+		optional => 1,
+		description => 'Number of dRAID spares.',
+	    },
 	    ashift => {
 		type => 'integer',
 		minimum => 9,
@@ -339,6 +354,8 @@ __PACKAGE__->register_method ({
 	my $devs = [PVE::Tools::split_list($param->{devices})];
 	my $raidlevel = $param->{raidlevel};
 	my $compression = $param->{compression} // 'on';
+	my $draid_data = $param->{draiddata};
+	my $draid_spares = $param->{draidspares};
 
 	for my $dev (@$devs) {
 	    $dev = PVE::Diskmanage::verify_blockdev_path($dev);
@@ -354,6 +371,9 @@ __PACKAGE__->register_method ({
 	    raidz => 3,
 	    raidz2 => 4,
 	    raidz3 => 5,
+	    draid => 3,
+	    draid2 => 4,
+	    draid3 => 5,
 	};
 
 	# sanity checks
@@ -366,6 +386,19 @@ __PACKAGE__->register_method ({
 	die "$raidlevel needs at least $mindisks->{$raidlevel} disks\n"
 	    if $numdisks < $mindisks->{$raidlevel};
 
+	# draid checks
+	if ($raidlevel =~ m/^draid/) {
+	    # bare minium would be two drives:
+	    # one parity & one data drive this code doesn't allow that because
+	    # it makes no sense, at least one spare disk should be used
+	    my $draidmin = $mindisks->{$raidlevel} - 2;
+	    $draidmin += $draid_data if $draid_data;
+	    $draidmin += $draid_spares if $draid_spares;
+
+	    die "At least $draidmin disks needed for current dRAID config\n"
+		if $numdisks < $draidmin;
+	}
+
 	my $code = sub {
 	    for my $dev (@$devs) {
 		PVE::Diskmanage::assert_disk_unused($dev);
@@ -402,6 +435,11 @@ __PACKAGE__->register_method ({
 		}
 	    } elsif ($raidlevel eq 'single') {
 		push @$cmd, $devs->[0];
+	    } elsif ($raidlevel =~ m/^draid/) {
+		my $draid_cmd = $raidlevel;
+		$draid_cmd .= ":${draid_data}d" if $draid_data;
+		$draid_cmd .= ":${draid_spares}s" if $draid_spares;
+		push @$cmd, $draid_cmd, @$devs;
 	    } else {
 		push @$cmd, $raidlevel, @$devs;
 	    }
-- 
2.30.2

[pve-devel] [PATCH pve-manager 2/3] fix #3967: enable ZFS dRAID creation in WebGUI

[Stefan Hrdlicka]

add fields for additional settings required by ZFS dRAID

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
requires the changes in pve-storageto work

 www/manager6/node/ZFS.js | 47 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/www/manager6/node/ZFS.js b/www/manager6/node/ZFS.js
index 5b3bdbda..5f3bbfef 100644
--- a/www/manager6/node/ZFS.js
+++ b/www/manager6/node/ZFS.js
@@ -42,6 +42,9 @@ Ext.define('PVE.node.CreateZFS', {
 			    fieldLabel: gettext('RAID Level'),
 			    name: 'raidlevel',
 			    value: 'single',
+			    listeners: {
+				change: 'onTargetChange',
+			    },
 			    comboItems: [
 				['single', gettext('Single Disk')],
 				['mirror', 'Mirror'],
@@ -49,8 +52,29 @@ Ext.define('PVE.node.CreateZFS', {
 				['raidz', 'RAIDZ'],
 				['raidz2', 'RAIDZ2'],
 				['raidz3', 'RAIDZ3'],
+				['draid', 'dRAID'],
+				['draid2', 'dRAID2'],
+				['draid3', 'dRAID3'],
 			    ],
 			},
+			{
+			    xtype: 'proxmoxintegerfield',
+			    fieldLabel: gettext('DRAID data devices'),
+			    minValue: 1,
+			    disabled: true,
+			    hidden: true,
+			    reference: 'draiddata',
+			    name: 'draiddata',
+			},
+			{
+			    xtype: 'proxmoxintegerfield',
+			    fieldLabel: gettext('DRAID spares'),
+			    minValue: 0,
+			    disabled: true,
+			    hidden: true,
+			    reference: 'draidspares',
+			    name: 'draidspares',
+			},
 			{
 			    xtype: 'proxmoxKVComboBox',
 			    fieldLabel: gettext('Compression'),
@@ -101,6 +125,29 @@ Ext.define('PVE.node.CreateZFS', {
 
         me.callParent();
     },
+
+    controller: {
+	xclass: 'Ext.app.ViewController',
+
+	onTargetChange: function(selection) {
+	    var me = this;
+	    var dataField = me.lookupReference('draiddata');
+	    var sparesField = me.lookupReference('draidspares');
+	    if (selection.value.startsWith("draid")) {
+		//show draid settings
+		dataField.setVisible(true);
+		dataField.setDisabled(false);
+		sparesField.setVisible(true);
+		sparesField.setDisabled(false);
+	    } else {
+		//hide draid settings
+		dataField.setVisible(false);
+		dataField.setDisabled(true);
+		sparesField.setVisible(false);
+		sparesField.setDisabled(true);
+	    }
+	},
+    },
 });
 
 Ext.define('PVE.node.ZFSList', {
-- 
2.30.2

[pve-devel] [PATCH pve-docs 3/3] fix #3967: add ZFS dRAID documentation

[Stefan Hrdlicka]

add some basic explanation how ZFS dRAID works including
links to openZFS for more details

add documentation for two dRAID parameters used in code

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 local-zfs.adoc | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/local-zfs.adoc b/local-zfs.adoc
index ab0f6ad..8eb681c 100644
--- a/local-zfs.adoc
+++ b/local-zfs.adoc
@@ -32,7 +32,8 @@ management.
 
 * Copy-on-write clone
 
-* Various raid levels: RAID0, RAID1, RAID10, RAIDZ-1, RAIDZ-2 and RAIDZ-3
+* Various raid levels: RAID0, RAID1, RAID10, RAIDZ-1, RAIDZ-2, RAIDZ-3,
+dRAID, dRAID2, dRAID3
 
 * Can use SSD for cache
 
@@ -244,6 +245,43 @@ them, unless your environment has specific needs and characteristics where
 RAIDZ performance characteristics are acceptable.
 
 
+ZFS dRAID
+~~~~~~~~~
+
+In a ZFS dRAID (declustered RAID) the hot spare drive(s) participate in the RAID.
+Their spare capacity is reservered and used for rebuilding when one drive fails.
+This provides depending on the configuration faster rebuilding compaired to a
+RAIDZ in case of drive failure. More information can be found in the official
+openZFS documenation. footnote:[OpenZFS dRAID
+https://openzfs.github.io/openzfs-docs/Basic%20Concepts/dRAID%20Howto.html]
+
+NOTE: dRAID is intended for more then 10-15 disks in a dRAID. A RAIDZ
+setup should be better for a lower amount of disks in most use cases.
+
+ * `dRAID1` or `dRAID`: requires at least 2 disks, one can fail before data is
+lost
+ * `dRAID2`: requires at least 3 disks, two can fail before data is lost
+ * `dRAID3`: requires at least 4 disks, three can fail before data is lost
+
+
+Additonal information can be found on manual page:
+
+----
+# man zpoolconcepts
+----
+
+spares and data
+^^^^^^^^^^^^^^^
+The number of `spares` tells the system how many disks it should keep ready in
+case of of a disk failure. The default value is 0 `spares`. Without spares
+rebuilding won't get any speed benefits.
+
+The number of `data` devices specifies the size of a parity group. The default
+is 8 if the number of `disks - parity - spares >= 8`. A higher number of `data`
+and parity drives increases the allocation size (e.g. for 4k sectors with
+default `data`=6 minimum allocation size is 24k) which can affect compression.
+
+
 Bootloader
 ~~~~~~~~~~
 
-- 
2.30.2

Re: [pve-devel] [PATCH pve-docs 3/3] fix #3967: add ZFS dRAID documentation

[Matthias Heiserer]

I found a few typos :)
8<---
> +ZFS dRAID
> +~~~~~~~~~
> +
> +In a ZFS dRAID (declustered RAID) the hot spare drive(s) participate in the RAID.
> +Their spare capacity is reservered and used for rebuilding when one drive fails.
typo: reservered -> reserved
> +This provides depending on the configuration faster rebuilding compaired to a
Sounds a bit off to me. Maybe something like "Depending on the 
configuration, this provides..." would be better?
> +RAIDZ in case of drive failure. More information can be found in the official
> +openZFS documenation. footnote:[OpenZFS dRAID
typo: documenation -> documentation
> +https://openzfs.github.io/openzfs-docs/Basic%20Concepts/dRAID%20Howto.html]
> +
> +NOTE: dRAID is intended for more then 10-15 disks in a dRAID. A RAIDZ
> +setup should be better for a lower amount of disks in most use cases.
> +
> + * `dRAID1` or `dRAID`: requires at least 2 disks, one can fail before data is
> +lost
> + * `dRAID2`: requires at least 3 disks, two can fail before data is lost
> + * `dRAID3`: requires at least 4 disks, three can fail before data is lost
> +
> +
> +Additonal information can be found on manual page:
typo: Additional
> +
> +----
> +# man zpoolconcepts
> +----
> +
> +spares and data
I think we consistently start headers with a capital letter, so "spares" 
should be "Spares"
> +^^^^^^^^^^^^^^^
> +The number of `spares` tells the system how many disks it should keep ready in
> +case of of a disk failure. The default value is 0 `spares`. Without spares
typo: of of -> of
> +rebuilding won't get any speed benefits.
> +
> +The number of `data` devices specifies the size of a parity group. The default
> +is 8 if the number of `disks - parity - spares >= 8`. A higher number of `data`
> +and parity drives increases the allocation size (e.g. for 4k sectors with
> +default `data`=6 minimum allocation size is 24k) which can affect compression.
> +
> +
>   Bootloader
>   ~~~~~~~~~~
>   

Re: [pve-devel] [PATCH pve-storage 1/3] fix #3967: enable ZFS dRAID creation via API

[Dominik Csapak]

some comments inline

On 6/2/22 13:22, Stefan Hrdlicka wrote:
> It is possible to set the number of spares and the size of
> data stripes via draidspares & dreaddata parameters.
> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>   PVE/API2/Disks/ZFS.pm | 40 +++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm
> index eeb9f48..63946d2 100644
> --- a/PVE/API2/Disks/ZFS.pm
> +++ b/PVE/API2/Disks/ZFS.pm
> @@ -299,12 +299,27 @@ __PACKAGE__->register_method ({
>   	    raidlevel => {
>   		type => 'string',
>   		description => 'The RAID level to use.',
> -		enum => ['single', 'mirror', 'raid10', 'raidz', 'raidz2', 'raidz3'],
> +		enum => ['single', 'mirror',
> +		    'raid10', 'raidz', 'raidz2', 'raidz3',
> +		    'draid', 'draid2', 'draid3',
> +		],

i'd rather have the first options also on a single line, so

enum => [
     'single', 'mirror',
     ...
],

makes it even easier to read

>   	    },
>   	    devices => {
>   		type => 'string', format => 'string-list',
>   		description => 'The block devices you want to create the zpool on.',
>   	    },
> +	    draiddata => {
> +		type => 'integer',
> +		minimum => 1,
> +		optional => 1,
> +		description => 'Number of dRAID data stripes.',

isn't that the 'number of data devices per redundancy group' ?
it sounds here like its the number of groups, not the number
of disks per group

> +	    },
> +	    draidspares => {
> +		type => 'integer',
> +		minimum => 0,
> +		optional => 1,
> +		description => 'Number of dRAID spares.',
> +	    },
>   	    ashift => {
>   		type => 'integer',
>   		minimum => 9,
> @@ -339,6 +354,8 @@ __PACKAGE__->register_method ({
>   	my $devs = [PVE::Tools::split_list($param->{devices})];
>   	my $raidlevel = $param->{raidlevel};
>   	my $compression = $param->{compression} // 'on';
> +	my $draid_data = $param->{draiddata};
> +	my $draid_spares = $param->{draidspares};
>   
>   	for my $dev (@$devs) {
>   	    $dev = PVE::Diskmanage::verify_blockdev_path($dev);
> @@ -354,6 +371,9 @@ __PACKAGE__->register_method ({
>   	    raidz => 3,
>   	    raidz2 => 4,
>   	    raidz3 => 5,
> +	    draid => 3,
> +	    draid2 => 4,
> +	    draid3 => 5,
>   	};
>   
>   	# sanity checks
> @@ -366,6 +386,19 @@ __PACKAGE__->register_method ({
>   	die "$raidlevel needs at least $mindisks->{$raidlevel} disks\n"
>   	    if $numdisks < $mindisks->{$raidlevel};
>   
> +	# draid checks
> +	if ($raidlevel =~ m/^draid/) {
> +	    # bare minium would be two drives:
> +	    # one parity & one data drive this code doesn't allow that because
> +	    # it makes no sense, at least one spare disk should be used
> +	    my $draidmin = $mindisks->{$raidlevel} - 2;
> +	    $draidmin += $draid_data if $draid_data;
> +	    $draidmin += $draid_spares if $draid_spares;
> +
> +	    die "At least $draidmin disks needed for current dRAID config\n"
> +		if $numdisks < $draidmin;
> +	}
> +

maybe check here in the else path if the draiddata/spares were set and
throw an error? since they don't make any sense
(i know the gui does not show them, but an api user might misunderstand)

>   	my $code = sub {
>   	    for my $dev (@$devs) {
>   		PVE::Diskmanage::assert_disk_unused($dev);
> @@ -402,6 +435,11 @@ __PACKAGE__->register_method ({
>   		}
>   	    } elsif ($raidlevel eq 'single') {
>   		push @$cmd, $devs->[0];
> +	    } elsif ($raidlevel =~ m/^draid/) {
> +		my $draid_cmd = $raidlevel;
> +		$draid_cmd .= ":${draid_data}d" if $draid_data;
> +		$draid_cmd .= ":${draid_spares}s" if $draid_spares;
> +		push @$cmd, $draid_cmd, @$devs;
>   	    } else {
>   		push @$cmd, $raidlevel, @$devs;
>   	    }

Re: [pve-devel] [PATCH pve-manager 2/3] fix #3967: enable ZFS dRAID creation in WebGUI

[Dominik Csapak]

comments inline

On 6/2/22 13:22, Stefan Hrdlicka wrote:
> add fields for additional settings required by ZFS dRAID
> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
> requires the changes in pve-storageto work
> 
>   www/manager6/node/ZFS.js | 47 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 47 insertions(+)
> 
> diff --git a/www/manager6/node/ZFS.js b/www/manager6/node/ZFS.js
> index 5b3bdbda..5f3bbfef 100644
> --- a/www/manager6/node/ZFS.js
> +++ b/www/manager6/node/ZFS.js
> @@ -42,6 +42,9 @@ Ext.define('PVE.node.CreateZFS', {
>   			    fieldLabel: gettext('RAID Level'),
>   			    name: 'raidlevel',
>   			    value: 'single',
> +			    listeners: {
> +				change: 'onTargetChange',
> +			    },
>   			    comboItems: [
>   				['single', gettext('Single Disk')],
>   				['mirror', 'Mirror'],
> @@ -49,8 +52,29 @@ Ext.define('PVE.node.CreateZFS', {
>   				['raidz', 'RAIDZ'],
>   				['raidz2', 'RAIDZ2'],
>   				['raidz3', 'RAIDZ3'],
> +				['draid', 'dRAID'],
> +				['draid2', 'dRAID2'],
> +				['draid3', 'dRAID3'],
>   			    ],
>   			},
> +			{
> +			    xtype: 'proxmoxintegerfield',
> +			    fieldLabel: gettext('DRAID data devices'),
> +			    minValue: 1,
> +			    disabled: true,
> +			    hidden: true,
> +			    reference: 'draiddata',
> +			    name: 'draiddata',
> +			},
> +			{
> +			    xtype: 'proxmoxintegerfield',
> +			    fieldLabel: gettext('DRAID spares'),
> +			    minValue: 0,
> +			    disabled: true,
> +			    hidden: true,
> +			    reference: 'draidspares',
> +			    name: 'draidspares',
> +			},

is that something someone always wants to configure?
or more an advanced thing? in the latter case, i'd
probably put them in the advanced options

>   			{
>   			    xtype: 'proxmoxKVComboBox',
>   			    fieldLabel: gettext('Compression'),
> @@ -101,6 +125,29 @@ Ext.define('PVE.node.CreateZFS', {
>   
>           me.callParent();
>       },
> +
> +    controller: {

nit: normally we put the controller  on top of the class, not at the
bottom

> +	xclass: 'Ext.app.ViewController',
> +
> +	onTargetChange: function(selection) {
> +	    var me = this;
> +	    var dataField = me.lookupReference('draiddata');
> +	    var sparesField = me.lookupReference('draidspares');
> +	    if (selection.value.startsWith("draid")) {
> +		//show draid settings
> +		dataField.setVisible(true);
> +		dataField.setDisabled(false);
> +		sparesField.setVisible(true);
> +		sparesField.setDisabled(false);
> +	    } else {
> +		//hide draid settings
> +		dataField.setVisible(false);
> +		dataField.setDisabled(true);
> +		sparesField.setVisible(false);
> +		sparesField.setDisabled(true);
> +	    }

this could be more elegantly solved in two other ways:
1. use a viewmodel with a formula that returns true/false
    depending on the startsWith('draid') and then use a
    bind on the hidden/disabled setting of the fields

2. put the bool into a variable and use that, like this

let isDraid = ...startsWith('draid');
dataField.setVisible(isDraid);
dataField.setDisabled(!isDraid);
...



> +	},
> +    },
>   });
>   
>   Ext.define('PVE.node.ZFSList', {

Re: [pve-devel] [PATCH pve-storage 1/3] fix #3967: enable ZFS dRAID creation via API

[Dominik Csapak]

just saw an additional thing, comment inline

On 6/2/22 13:22, Stefan Hrdlicka wrote:
> It is possible to set the number of spares and the size of
> data stripes via draidspares & dreaddata parameters.
> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>   PVE/API2/Disks/ZFS.pm | 40 +++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm
> index eeb9f48..63946d2 100644
> --- a/PVE/API2/Disks/ZFS.pm
> +++ b/PVE/API2/Disks/ZFS.pm
> @@ -299,12 +299,27 @@ __PACKAGE__->register_method ({
>   	    raidlevel => {
>   		type => 'string',
>   		description => 'The RAID level to use.',
> -		enum => ['single', 'mirror', 'raid10', 'raidz', 'raidz2', 'raidz3'],
> +		enum => ['single', 'mirror',
> +		    'raid10', 'raidz', 'raidz2', 'raidz3',
> +		    'draid', 'draid2', 'draid3',
> +		],
>   	    },
>   	    devices => {
>   		type => 'string', format => 'string-list',
>   		description => 'The block devices you want to create the zpool on.',
>   	    },
> +	    draiddata => {
> +		type => 'integer',
> +		minimum => 1,
> +		optional => 1,
> +		description => 'Number of dRAID data stripes.',
> +	    },
> +	    draidspares => {
> +		type => 'integer',
> +		minimum => 0,
> +		optional => 1,
> +		description => 'Number of dRAID spares.',
> +	    },
>   	    ashift => {
>   		type => 'integer',
>   		minimum => 9,
> @@ -339,6 +354,8 @@ __PACKAGE__->register_method ({
>   	my $devs = [PVE::Tools::split_list($param->{devices})];
>   	my $raidlevel = $param->{raidlevel};
>   	my $compression = $param->{compression} // 'on';
> +	my $draid_data = $param->{draiddata};
> +	my $draid_spares = $param->{draidspares};
>   
>   	for my $dev (@$devs) {
>   	    $dev = PVE::Diskmanage::verify_blockdev_path($dev);
> @@ -354,6 +371,9 @@ __PACKAGE__->register_method ({
>   	    raidz => 3,
>   	    raidz2 => 4,
>   	    raidz3 => 5,
> +	    draid => 3,
> +	    draid2 => 4,
> +	    draid3 => 5,
>   	};
>   
>   	# sanity checks
> @@ -366,6 +386,19 @@ __PACKAGE__->register_method ({
>   	die "$raidlevel needs at least $mindisks->{$raidlevel} disks\n"
>   	    if $numdisks < $mindisks->{$raidlevel};
>   
> +	# draid checks
> +	if ($raidlevel =~ m/^draid/) {
> +	    # bare minium would be two drives:
> +	    # one parity & one data drive this code doesn't allow that because
> +	    # it makes no sense, at least one spare disk should be used
> +	    my $draidmin = $mindisks->{$raidlevel} - 2;
> +	    $draidmin += $draid_data if $draid_data;
> +	    $draidmin += $draid_spares if $draid_spares;

isn't that calculation wrong?
if i set draid and no data/spares, i just have to give a single disk?
(so we should initialize draid_data with 1 in that case probably?)

also why do you set min draid => 3, subtract 2 then add some again?
why not have draid => 1, and just add the other things?


> +
> +	    die "At least $draidmin disks needed for current dRAID config\n"
> +		if $numdisks < $draidmin;
> +	}
> +
>   	my $code = sub {
>   	    for my $dev (@$devs) {
>   		PVE::Diskmanage::assert_disk_unused($dev);
> @@ -402,6 +435,11 @@ __PACKAGE__->register_method ({
>   		}
>   	    } elsif ($raidlevel eq 'single') {
>   		push @$cmd, $devs->[0];
> +	    } elsif ($raidlevel =~ m/^draid/) {
> +		my $draid_cmd = $raidlevel;
> +		$draid_cmd .= ":${draid_data}d" if $draid_data;
> +		$draid_cmd .= ":${draid_spares}s" if $draid_spares;
> +		push @$cmd, $draid_cmd, @$devs;
>   	    } else {
>   		push @$cmd, $raidlevel, @$devs;
>   	    }

Re: [pve-devel] [PATCH pve-docs 3/3] fix #3967: add ZFS dRAID documentation

[Dominik Csapak]

comments inline

On 6/2/22 13:22, Stefan Hrdlicka wrote:
> add some basic explanation how ZFS dRAID works including
> links to openZFS for more details
> 
> add documentation for two dRAID parameters used in code
> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>   local-zfs.adoc | 40 +++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/local-zfs.adoc b/local-zfs.adoc
> index ab0f6ad..8eb681c 100644
> --- a/local-zfs.adoc
> +++ b/local-zfs.adoc
> @@ -32,7 +32,8 @@ management.
>   
>   * Copy-on-write clone
>   
> -* Various raid levels: RAID0, RAID1, RAID10, RAIDZ-1, RAIDZ-2 and RAIDZ-3
> +* Various raid levels: RAID0, RAID1, RAID10, RAIDZ-1, RAIDZ-2, RAIDZ-3,
> +dRAID, dRAID2, dRAID3
>   
>   * Can use SSD for cache
>   
> @@ -244,6 +245,43 @@ them, unless your environment has specific needs and characteristics where
>   RAIDZ performance characteristics are acceptable.
>   
>   
> +ZFS dRAID
> +~~~~~~~~~
> +
> +In a ZFS dRAID (declustered RAID) the hot spare drive(s) participate in the RAID.
> +Their spare capacity is reservered and used for rebuilding when one drive fails.
> +This provides depending on the configuration faster rebuilding compaired to a
> +RAIDZ in case of drive failure. More information can be found in the official
> +openZFS documenation. footnote:[OpenZFS dRAID
> +https://openzfs.github.io/openzfs-docs/Basic%20Concepts/dRAID%20Howto.html]
> +
> +NOTE: dRAID is intended for more then 10-15 disks in a dRAID. A RAIDZ
> +setup should be better for a lower amount of disks in most use cases.
> +
> + * `dRAID1` or `dRAID`: requires at least 2 disks, one can fail before data is
> +lost
> + * `dRAID2`: requires at least 3 disks, two can fail before data is lost
> + * `dRAID3`: requires at least 4 disks, three can fail before data is lost
> +
> +
> +Additonal information can be found on manual page:
> +
> +----
> +# man zpoolconcepts
> +----
> +
> +spares and data
> +^^^^^^^^^^^^^^^
> +The number of `spares` tells the system how many disks it should keep ready in
> +case of of a disk failure. The default value is 0 `spares`. Without spares
> +rebuilding won't get any speed benefits.
> +
> +The number of `data` devices specifies the size of a parity group. The default
> +is 8 if the number of `disks - parity - spares >= 8`. A higher number of `data`
> +and parity drives increases the allocation size (e.g. for 4k sectors with
> +default `data`=6 minimum allocation size is 24k) which can affect compression.

i found this block a bit confusing, because among other things, you talk about
'parity groups' but neither this, nor the offical docs mention a 'parity group'
rather a 'redundancy group', so i'd rename that

also i'd spell it out more clearly that the default is `disks - parity - spares` until
it's greater, then it's clamped at 8 (by default)

also i'd somehow mention the things from this sentence from the offical docs:
 > In general a smaller value of D will increase IOPS, improve the compression ratio, and speed up 
resilvering at the expense of total usable capacity. Defaults to 8, unless N-P-S is less than 8.


> +
> +
>   Bootloader
>   ~~~~~~~~~~
>   

Re: [pve-devel] [PATCH pve-storage 1/3] fix #3967: enable ZFS dRAID creation via API

[Dominik Csapak]

On 6/3/22 14:31, Dominik Csapak wrote:
> just saw an additional thing, comment inline
> 
> On 6/2/22 13:22, Stefan Hrdlicka wrote:
>> It is possible to set the number of spares and the size of
>> data stripes via draidspares & dreaddata parameters.
>>
>> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
>> ---
>>   PVE/API2/Disks/ZFS.pm | 40 +++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm
>> index eeb9f48..63946d2 100644
>> --- a/PVE/API2/Disks/ZFS.pm
>> +++ b/PVE/API2/Disks/ZFS.pm
>> @@ -299,12 +299,27 @@ __PACKAGE__->register_method ({
>>           raidlevel => {
>>           type => 'string',
>>           description => 'The RAID level to use.',
>> -        enum => ['single', 'mirror', 'raid10', 'raidz', 'raidz2', 'raidz3'],
>> +        enum => ['single', 'mirror',
>> +            'raid10', 'raidz', 'raidz2', 'raidz3',
>> +            'draid', 'draid2', 'draid3',
>> +        ],
>>           },
>>           devices => {
>>           type => 'string', format => 'string-list',
>>           description => 'The block devices you want to create the zpool on.',
>>           },
>> +        draiddata => {
>> +        type => 'integer',
>> +        minimum => 1,
>> +        optional => 1,
>> +        description => 'Number of dRAID data stripes.',
>> +        },
>> +        draidspares => {
>> +        type => 'integer',
>> +        minimum => 0,
>> +        optional => 1,
>> +        description => 'Number of dRAID spares.',
>> +        },
>>           ashift => {
>>           type => 'integer',
>>           minimum => 9,
>> @@ -339,6 +354,8 @@ __PACKAGE__->register_method ({
>>       my $devs = [PVE::Tools::split_list($param->{devices})];
>>       my $raidlevel = $param->{raidlevel};
>>       my $compression = $param->{compression} // 'on';
>> +    my $draid_data = $param->{draiddata};
>> +    my $draid_spares = $param->{draidspares};
>>       for my $dev (@$devs) {
>>           $dev = PVE::Diskmanage::verify_blockdev_path($dev);
>> @@ -354,6 +371,9 @@ __PACKAGE__->register_method ({
>>           raidz => 3,
>>           raidz2 => 4,
>>           raidz3 => 5,
>> +        draid => 3,
>> +        draid2 => 4,
>> +        draid3 => 5,
>>       };
>>       # sanity checks
>> @@ -366,6 +386,19 @@ __PACKAGE__->register_method ({
>>       die "$raidlevel needs at least $mindisks->{$raidlevel} disks\n"
>>           if $numdisks < $mindisks->{$raidlevel};
>> +    # draid checks
>> +    if ($raidlevel =~ m/^draid/) {
>> +        # bare minium would be two drives:
>> +        # one parity & one data drive this code doesn't allow that because
>> +        # it makes no sense, at least one spare disk should be used
>> +        my $draidmin = $mindisks->{$raidlevel} - 2;
>> +        $draidmin += $draid_data if $draid_data;
>> +        $draidmin += $draid_spares if $draid_spares;
> 
> isn't that calculation wrong?
> if i set draid and no data/spares, i just have to give a single disk?
> (so we should initialize draid_data with 1 in that case probably?)
> 
> also why do you set min draid => 3, subtract 2 then add some again?
> why not have draid => 1, and just add the other things?

thats wrong of course, since we check the amount of disks additionally
at the beginning, sorry for the noise

> 
> 
>> +
>> +        die "At least $draidmin disks needed for current dRAID config\n"
>> +        if $numdisks < $draidmin;
>> +    }
>> +
>>       my $code = sub {
>>           for my $dev (@$devs) {
>>           PVE::Diskmanage::assert_disk_unused($dev);
>> @@ -402,6 +435,11 @@ __PACKAGE__->register_method ({
>>           }
>>           } elsif ($raidlevel eq 'single') {
>>           push @$cmd, $devs->[0];
>> +        } elsif ($raidlevel =~ m/^draid/) {
>> +        my $draid_cmd = $raidlevel;
>> +        $draid_cmd .= ":${draid_data}d" if $draid_data;
>> +        $draid_cmd .= ":${draid_spares}s" if $draid_spares;
>> +        push @$cmd, $draid_cmd, @$devs;
>>           } else {
>>           push @$cmd, $raidlevel, @$devs;
>>           }
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

Re: [pve-devel] [PATCH pve-docs 3/3] fix #3967: add ZFS dRAID documentation

[Thomas Lamprecht]

Am 02/06/2022 um 14:47 schrieb Matthias Heiserer:
>>
>> +spares and data
> I think we consistently start headers with a capital letter, so "spares" should be "Spares"

We actually use Title Case[0] for headings in all docs and also wiki articles,
so it should be "Spares and Data"


[0]: https://en.wikipedia.org/wiki/Title_case

Re: [pve-devel] [PATCH pve-manager 2/3] fix #3967: enable ZFS dRAID creation in WebGUI

[Stefan Hrdlicka]

On 6/3/22 14:24, Dominik Csapak wrote:
> comments inline
> 
> On 6/2/22 13:22, Stefan Hrdlicka wrote:
>> add fields for additional settings required by ZFS dRAID
>>
>> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
>> ---
>> requires the changes in pve-storageto work
>>
>>   www/manager6/node/ZFS.js | 47 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 47 insertions(+)
>>
>> diff --git a/www/manager6/node/ZFS.js b/www/manager6/node/ZFS.js
>> index 5b3bdbda..5f3bbfef 100644
>> --- a/www/manager6/node/ZFS.js
>> +++ b/www/manager6/node/ZFS.js
>> @@ -42,6 +42,9 @@ Ext.define('PVE.node.CreateZFS', {
>>                   fieldLabel: gettext('RAID Level'),
>>                   name: 'raidlevel',
>>                   value: 'single',
>> +                listeners: {
>> +                change: 'onTargetChange',
>> +                },
>>                   comboItems: [
>>                   ['single', gettext('Single Disk')],
>>                   ['mirror', 'Mirror'],
>> @@ -49,8 +52,29 @@ Ext.define('PVE.node.CreateZFS', {
>>                   ['raidz', 'RAIDZ'],
>>                   ['raidz2', 'RAIDZ2'],
>>                   ['raidz3', 'RAIDZ3'],
>> +                ['draid', 'dRAID'],
>> +                ['draid2', 'dRAID2'],
>> +                ['draid3', 'dRAID3'],
>>                   ],
>>               },
>> +            {
>> +                xtype: 'proxmoxintegerfield',
>> +                fieldLabel: gettext('DRAID data devices'),
>> +                minValue: 1,
>> +                disabled: true,
>> +                hidden: true,
>> +                reference: 'draiddata',
>> +                name: 'draiddata',
>> +            },
>> +            {
>> +                xtype: 'proxmoxintegerfield',
>> +                fieldLabel: gettext('DRAID spares'),
>> +                minValue: 0,
>> +                disabled: true,
>> +                hidden: true,
>> +                reference: 'draidspares',
>> +                name: 'draidspares',
>> +            },
> 
> is that something someone always wants to configure?
> or more an advanced thing? in the latter case, i'd
> probably put them in the advanced options
> 

I would say you nearly always want to change at least the number of 
spares. The default values for creating a dRAID without setting anything 
are 0 spares and 8 devices per redundancy group (if disks >=8). The 
advantage of dRAID is fast rebuilding of a broken disk which you will 
only get (as far as I read) when you have spares configured.

The data devices per redundancy group would be possible to hide. But 
they have an impact on performance depending on the number chosen as 
well as storage usage. I'm not sure that it makes sense hiding one value 
of the two. I will think about it ;).


>>               {
>>                   xtype: 'proxmoxKVComboBox',
>>                   fieldLabel: gettext('Compression'),
>> @@ -101,6 +125,29 @@ Ext.define('PVE.node.CreateZFS', {
>>           me.callParent();
>>       },
>> +
>> +    controller: {
> 
> nit: normally we put the controller  on top of the class, not at the
> bottom
> 
>> +    xclass: 'Ext.app.ViewController',
>> +
>> +    onTargetChange: function(selection) {
>> +        var me = this;
>> +        var dataField = me.lookupReference('draiddata');
>> +        var sparesField = me.lookupReference('draidspares');
>> +        if (selection.value.startsWith("draid")) {
>> +        //show draid settings
>> +        dataField.setVisible(true);
>> +        dataField.setDisabled(false);
>> +        sparesField.setVisible(true);
>> +        sparesField.setDisabled(false);
>> +        } else {
>> +        //hide draid settings
>> +        dataField.setVisible(false);
>> +        dataField.setDisabled(true);
>> +        sparesField.setVisible(false);
>> +        sparesField.setDisabled(true);
>> +        }
> 
> this could be more elegantly solved in two other ways:
> 1. use a viewmodel with a formula that returns true/false
>     depending on the startsWith('draid') and then use a
>     bind on the hidden/disabled setting of the fields
> 
> 2. put the bool into a variable and use that, like this
> 
> let isDraid = ...startsWith('draid');
> dataField.setVisible(isDraid);
> dataField.setDisabled(!isDraid);
> ...
> 
> 
> 
>> +    },
>> +    },
>>   });
>>   Ext.define('PVE.node.ZFSList', {
> 
>