[pve-devel] [RFC PATCH 1/2] install: btrfs: fix raid level falling back to single mode

[pve-devel] [RFC PATCH 1/2] install: btrfs: fix raid level falling back to single mode

[Daniel Kral]

From a user's perspective, the BTRFS single mode has been removed since
d85180e ("tui: rename raid levels 0/1 to align with GUI installer"). The
user can now only select at least the "RAID0" level, but if a user
selects any raid level with only one disk configured on the system,
BTRFS will be setup in 'single' mode instead of the chosen raid level.

The TUI installer has a separate check for this, but the GUI installer
as well as the auto installer will silently fallback to single mode,
which could be confusing for and unwanted by the user.

Therefore, remove the BTRFS single mode from being selected when
configuring disks in the GUI installer and during the installation in
general, which makes the auto installer fail if the wrong amount of
disks are selected for the specified raid level. This makes btrfs' raid
disk count validation align with the one from zfs.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---

Discussion

BTRFS didn't allow a single-disk RAID0 configuration before kernel 5.15
and AFAIK also silently used the single profile for that case, but this
has changed since then.

If we want users to still be able to create a BTRFS filesystem in single
mode (which seems very reasonable), I can do a v2 or followup to add a
"btrfs (single)" entry to the disk setup.

Testing

I have tested this by the usual installer test procedure (debug shell,
patch files, launch target installer manually) and the GUI installer
disallows creating a btrfs raid1 or raid10 with only one disk, but
allows it with at least the correct amount of disks. The same applies to
the autoinstaller with the same test cases.

Unpatched a single-disk RAID0 install resulted in:

```
root@pve:~# btrfs filesystem usage -T .
[ ... ]
             Data   Metadata  System
Id Path      single single    single   Unallocated Total    Slack
-- --------- ------ --------- -------- ----------- -------- -------
 1 /dev/sda3 4.01GB 520.00MiB  4.00MiB    26.98GiB 31.50GiB 3.50KiB
[ ... ]
```

and patched a single-disk RAID0 install results in:

```
```
root@pve:~# btrfs filesystem usage -T .
[ ... ]
             Data   Metadata  System
Id Path      RAID0  RAID0     RAID0    Unallocated Total    Slack
-- --------- ------ --------- -------- ----------- -------- -------
 1 /dev/sda3 4.00GB 512.00MiB  8.00MiB    26.99GiB 31.50GiB 3.50KiB
[ ... ]
```
```

 Proxmox/Install.pm | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index c0a17b2..b72a83e 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -359,20 +359,16 @@ sub get_btrfs_raid_setup {
 
     my $mode;
 
-    if ($diskcount == 1) {
-	$mode = 'single';
+    if ($filesys eq 'btrfs (RAID0)') {
+	$mode = 'raid0';
+    } elsif ($filesys eq 'btrfs (RAID1)') {
+	die "btrfs (RAID1) needs at least 2 devices\n" if $diskcount < 2;
+	$mode = 'raid1';
+    } elsif ($filesys eq 'btrfs (RAID10)') {
+	die "btrfs (RAID10) needs at least 4 devices\n" if $diskcount < 4;
+	$mode = 'raid10';
     } else {
-	if ($filesys eq 'btrfs (RAID0)') {
-	    $mode = 'raid0';
-	} elsif ($filesys eq 'btrfs (RAID1)') {
-	    die "btrfs (RAID1) needs at least 2 devices\n" if $diskcount < 2;
-	    $mode = 'raid1';
-	} elsif ($filesys eq 'btrfs (RAID10)') {
-	    die "btrfs (RAID10) needs at least 4 devices\n" if $diskcount < 4;
-	    $mode = 'raid10';
-	} else {
-	    die "unknown btrfs mode '$filesys'\n";
-	}
+	die "unknown btrfs mode '$filesys'\n";
     }
 
     return ($devlist, $mode);
-- 
2.39.5



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

[pve-devel] [RFC PATCH 2/2] common: btrfs: lower minimum amount of disks for raid10 to 2

[Daniel Kral]

As the installer allows single-disk RAID0 configurations and BTRFS
allows to create a filesystem with the RAID10 profile with only two
disks since kernel version 5.15 [0], lower the minimum amount of disks
the installer requires for a BTRFS RAID10 setup.

The motiviation for this is to allow users to create a BTRFS RAID10
configuration even though they do not have the necessary disks ready at
setup time itself without needing to convert the profile afterwards.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b2f78e88052bc0bee56bbf646d245fcfb431a873

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
Discussion

If this patch seems like something worthwile, I think it would be
necessary to have some sort of warning popup for 2 <= $diskcount < 4 in
RAID10, and maybe also the same for $diskcount == 1 in RAID0, that
there's no advantage to create a degenerate RAID0/10 without planning to
add at least 1/2 disks later. I would add this in a v2 or followup if
this gets ACKed.

 Proxmox/Install.pm                          | 2 +-
 proxmox-installer-common/src/disk_checks.rs | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index b72a83e..d52d17b 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -365,7 +365,7 @@ sub get_btrfs_raid_setup {
 	die "btrfs (RAID1) needs at least 2 devices\n" if $diskcount < 2;
 	$mode = 'raid1';
     } elsif ($filesys eq 'btrfs (RAID10)') {
-	die "btrfs (RAID10) needs at least 4 devices\n" if $diskcount < 4;
+	die "btrfs (RAID10) needs at least 2 devices\n" if $diskcount < 2;
 	$mode = 'raid10';
     } else {
 	die "unknown btrfs mode '$filesys'\n";
diff --git a/proxmox-installer-common/src/disk_checks.rs b/proxmox-installer-common/src/disk_checks.rs
index ecc43bd..bd1c54c 100644
--- a/proxmox-installer-common/src/disk_checks.rs
+++ b/proxmox-installer-common/src/disk_checks.rs
@@ -129,7 +129,7 @@ pub fn check_btrfs_raid_config(level: BtrfsRaidLevel, disks: &[Disk]) -> Result<
     match level {
         BtrfsRaidLevel::Raid0 => check_raid_min_disks(disks, 1)?,
         BtrfsRaidLevel::Raid1 => check_raid_min_disks(disks, 2)?,
-        BtrfsRaidLevel::Raid10 => check_raid_min_disks(disks, 4)?,
+        BtrfsRaidLevel::Raid10 => check_raid_min_disks(disks, 2)?,
     }
 
     Ok(())
@@ -204,8 +204,8 @@ mod tests {
         assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid1, &disks).is_ok());
 
         assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &[]).is_err());
-        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks[..3]).is_err());
-        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks[..4]).is_ok());
+        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks[..1]).is_err());
+        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks[..2]).is_ok());
         assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks).is_ok());
     }
 
-- 
2.39.5



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

Re: [pve-devel] [RFC PATCH 2/2] common: btrfs: lower minimum amount of disks for raid10 to 2

[Fabio Fantoni via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 9776 bytes --]

From: Fabio Fantoni <fabio.fantoni@m2r.biz>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>, Daniel Kral <d.kral@proxmox.com>
Subject: Re: [pve-devel] [RFC PATCH 2/2] common: btrfs: lower minimum amount of disks for raid10 to 2
Date: Mon, 13 Jan 2025 13:24:06 +0100
Message-ID: <0999b2e1-8b8b-4baf-84d6-32251a675338@m2r.biz>

Il 10/01/2025 18:00, Daniel Kral ha scritto:
> As the installer allows single-disk RAID0 configurations and BTRFS
> allows to create a filesystem with the RAID10 profile with only two
> disks since kernel version 5.15 [0], lower the minimum amount of disks
> the installer requires for a BTRFS RAID10 setup.
>
> The motiviation for this is to allow users to create a BTRFS RAID10
> configuration even though they do not have the necessary disks ready at
> setup time itself without needing to convert the profile afterwards.

btrfs profiles work differently but other hardware or software raids,
many users may not inform themselves well beforehand but even in the
case of informed users even if technically now btrfs allows lower limits
with the creation of raid 0 (and raid10) I think it would be better to
keep them at the base at the creation and then it must be the user who
consciously makes any subsequent conversions.

regarding btrfs profiles at creation, one thing that could be useful is
to always put duplicate metadata (dup with single disk or raid 1 in the
case of raid0), if you don't want it by default maybe put it as an
additional option, and if you don't want that either at least add it to
the documentation (as a suggestion if you want greater resilience of the
filesystem without consuming excessive space)

>
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b2f78e88052bc0bee56bbf646d245fcfb431a873
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
> Discussion
>
> If this patch seems like something worthwile, I think it would be
> necessary to have some sort of warning popup for 2 <= $diskcount < 4 in
> RAID10, and maybe also the same for $diskcount == 1 in RAID0, that
> there's no advantage to create a degenerate RAID0/10 without planning to
> add at least 1/2 disks later. I would add this in a v2 or followup if
> this gets ACKed.
>
>   Proxmox/Install.pm                          | 2 +-
>   proxmox-installer-common/src/disk_checks.rs | 6 +++---
>   2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
> index b72a83e..d52d17b 100644
> --- a/Proxmox/Install.pm
> +++ b/Proxmox/Install.pm
> @@ -365,7 +365,7 @@ sub get_btrfs_raid_setup {
>   	die "btrfs (RAID1) needs at least 2 devices\n" if $diskcount < 2;
>   	$mode = 'raid1';
>       } elsif ($filesys eq 'btrfs (RAID10)') {
> -	die "btrfs (RAID10) needs at least 4 devices\n" if $diskcount < 4;
> +	die "btrfs (RAID10) needs at least 2 devices\n" if $diskcount < 2;
>   	$mode = 'raid10';
>       } else {
>   	die "unknown btrfs mode '$filesys'\n";
> diff --git a/proxmox-installer-common/src/disk_checks.rs b/proxmox-installer-common/src/disk_checks.rs
> index ecc43bd..bd1c54c 100644
> --- a/proxmox-installer-common/src/disk_checks.rs
> +++ b/proxmox-installer-common/src/disk_checks.rs
> @@ -129,7 +129,7 @@ pub fn check_btrfs_raid_config(level: BtrfsRaidLevel, disks: &[Disk]) -> Result<
>       match level {
>           BtrfsRaidLevel::Raid0 => check_raid_min_disks(disks, 1)?,
>           BtrfsRaidLevel::Raid1 => check_raid_min_disks(disks, 2)?,
> -        BtrfsRaidLevel::Raid10 => check_raid_min_disks(disks, 4)?,
> +        BtrfsRaidLevel::Raid10 => check_raid_min_disks(disks, 2)?,
>       }
>
>       Ok(())
> @@ -204,8 +204,8 @@ mod tests {
>           assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid1, &disks).is_ok());
>
>           assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &[]).is_err());
> -        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks[..3]).is_err());
> -        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks[..4]).is_ok());
> +        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks[..1]).is_err());
> +        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks[..2]).is_ok());
>           assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks).is_ok());
>       }
>



--
Questa email è stata esaminata alla ricerca di virus dal software antivirus Avast.
www.avast.com


[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [pve-devel] [RFC PATCH 2/2] common: btrfs: lower minimum amount of disks for raid10 to 2

[Daniel Kral]

On 1/13/25 13:24, Fabio Fantoni wrote:
> btrfs profiles work differently but other hardware or software raids, 
> many users may not inform themselves well beforehand but even in the 
> case of informed users even if technically now btrfs allows lower limits 
> with the creation of raid 0 (and raid10) I think it would be better to 
> keep them at the base at the creation and then it must be the user who 
> consciously makes any subsequent conversions.

Hm, I'm still unsure about this, because AFAIK we already allow creating 
ZFS RAID0 with a single disk, which technically also isn't a "real" 
RAID0 setup itself. But fair point for RAID10, it could be irritating 
for users to have a discrepancy between the minimum disk amount of ZFS 
and BTRFS RAID10 and it'd be a bit harder to communicate that in a 
understandable manner.

> 
> regarding btrfs profiles at creation, one thing that could be useful is 
> to always put duplicate metadata (dup with single disk or raid 1 in the 
> case of raid0), if you don't want it by default maybe put it as an 
> additional option, and if you don't want that either at least add it to 
> the documentation (as a suggestion if you want greater resilience of the 
> filesystem without consuming excessive space)

Currently, the installer creates the BTRFS filesystem with the data and 
metadata both using the same profile. I also think it could be valuable 
to have an "advanced" option, which allows to set a separate profile for 
the metadata.

Feel free to send either a RFC for it (even if I can't tell you whether 
it will be accepted as it adds some complexity to the fs setup) or 
create a Bugzilla so also other users and developers can discuss it.


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

Re: [pve-devel] [RFC PATCH 2/2] common: btrfs: lower minimum amount of disks for raid10 to 2

[Fabio Fantoni via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 9177 bytes --]

From: Fabio Fantoni <fabio.fantoni@m2r.biz>
To: Daniel Kral <d.kral@proxmox.com>, Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [RFC PATCH 2/2] common: btrfs: lower minimum amount of disks for raid10 to 2
Date: Wed, 15 Jan 2025 17:14:05 +0100
Message-ID: <65b62a24-fd78-4c4b-a7a9-5955237e872b@m2r.biz>

Il 15/01/2025 10:00, Daniel Kral ha scritto:
> On 1/13/25 13:24, Fabio Fantoni wrote:
>> btrfs profiles work differently but other hardware or software raids,
>> many users may not inform themselves well beforehand but even in the
>> case of informed users even if technically now btrfs allows lower
>> limits with the creation of raid 0 (and raid10) I think it would be
>> better to keep them at the base at the creation and then it must be
>> the user who consciously makes any subsequent conversions.
>
> Hm, I'm still unsure about this, because AFAIK we already allow
> creating ZFS RAID0 with a single disk, which technically also isn't a
> "real" RAID0 setup itself. But fair point for RAID10, it could be
> irritating for users to have a discrepancy between the minimum disk
> amount of ZFS and BTRFS RAID10 and it'd be a bit harder to communicate
> that in a understandable manner.

I'm not really sure if the difference with zfs (which they might not use
at all by btrfs users) is more impactful, or at least how proxmox
implements it, but I think it's more what you would expect in general
and even if it recently allows the use of the raid0 profile in
"fictitious" cases without requirements, or the possible removal of
disks without needing to convert the profile, it still seems strange to
me to have a "particular" situation by default, perhaps confusing even
more any less expert users.

I could be wrong, it would be useful to have more people's opinions.

>
>>
>> regarding btrfs profiles at creation, one thing that could be useful
>> is to always put duplicate metadata (dup with single disk or raid 1
>> in the case of raid0), if you don't want it by default maybe put it
>> as an additional option, and if you don't want that either at least
>> add it to the documentation (as a suggestion if you want greater
>> resilience of the filesystem without consuming excessive space)
>
> Currently, the installer creates the BTRFS filesystem with the data
> and metadata both using the same profile. I also think it could be
> valuable to have an "advanced" option, which allows to set a separate
> profile for the metadata.
>
> Feel free to send either a RFC for it (even if I can't tell you
> whether it will be accepted as it adds some complexity to the fs
> setup) or create a Bugzilla so also other users and developers can
> discuss it.
>
By default recently duplicate metadata is for all disk types, forcing
them to single is a proxmox specific thing. so it seemed good to me to
have at least the optional possibility to have them duplicated in the
installation. eventually could at least inform about this thing in the
documentation so that users are more aware and if they want they can
convert the metadata profile easily and quickly (dup for single disk and
raid1 in case of multiple disks in raid0).

regarding opening discussions I tried for some things even if there were
few or no answers.
bugzilla seems to me little used, the forum is very much used and there
is a minimum of participation but unfortunately given the enormous
quantity of topics that are continuously created the topics quickly
disappear from view.
I don't know what is best to do, eventually I will try to write again on
the forum.


--
Questa email è stata esaminata alla ricerca di virus dal software antivirus Avast.
www.avast.com


[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [pve-devel] [RFC PATCH 1/2] install: btrfs: fix raid level falling back to single mode

[Fabio Fantoni via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 9349 bytes --]

From: Fabio Fantoni <fabio.fantoni@m2r.biz>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>, Daniel Kral <d.kral@proxmox.com>
Subject: Re: [pve-devel] [RFC PATCH 1/2] install: btrfs: fix raid level falling back to single mode
Date: Mon, 13 Jan 2025 13:15:23 +0100
Message-ID: <a249f449-e9f8-4e41-b1c5-408e768c41fd@m2r.biz>

Il 10/01/2025 18:00, Daniel Kral ha scritto:
>  From a user's perspective, the BTRFS single mode has been removed since
> d85180e ("tui: rename raid levels 0/1 to align with GUI installer"). The
> user can now only select at least the "RAID0" level, but if a user
> selects any raid level with only one disk configured on the system,
> BTRFS will be setup in 'single' mode instead of the chosen raid level.
>
> The TUI installer has a separate check for this, but the GUI installer
> as well as the auto installer will silently fallback to single mode,
> which could be confusing for and unwanted by the user.
>
> Therefore, remove the BTRFS single mode from being selected when
> configuring disks in the GUI installer and during the installation in
> general, which makes the auto installer fail if the wrong amount of
> disks are selected for the specified raid level. This makes btrfs' raid
> disk count validation align with the one from zfs.
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
>
> Discussion
>
> BTRFS didn't allow a single-disk RAID0 configuration before kernel 5.15
> and AFAIK also silently used the single profile for that case, but this
> has changed since then.
>
> If we want users to still be able to create a BTRFS filesystem in single
> mode (which seems very reasonable), I can do a v2 or followup to add a
> "btrfs (single)" entry to the disk setup.
Hi, I think is better to add also "single" mode
>
> Testing
>
> I have tested this by the usual installer test procedure (debug shell,
> patch files, launch target installer manually) and the GUI installer
> disallows creating a btrfs raid1 or raid10 with only one disk, but
> allows it with at least the correct amount of disks. The same applies to
> the autoinstaller with the same test cases.
>
> Unpatched a single-disk RAID0 install resulted in:
>
> ```
> root@pve:~# btrfs filesystem usage -T .
> [ ... ]
>               Data   Metadata  System
> Id Path      single single    single   Unallocated Total    Slack
> -- --------- ------ --------- -------- ----------- -------- -------
>   1 /dev/sda3 4.01GB 520.00MiB  4.00MiB    26.98GiB 31.50GiB 3.50KiB
> [ ... ]
> ```
>
> and patched a single-disk RAID0 install results in:
>
> ```
> ```
> root@pve:~# btrfs filesystem usage -T .
> [ ... ]
>               Data   Metadata  System
> Id Path      RAID0  RAID0     RAID0    Unallocated Total    Slack
> -- --------- ------ --------- -------- ----------- -------- -------
>   1 /dev/sda3 4.00GB 512.00MiB  8.00MiB    26.99GiB 31.50GiB 3.50KiB
> [ ... ]
> ```
> ```
>
>   Proxmox/Install.pm | 22 +++++++++-------------
>   1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
> index c0a17b2..b72a83e 100644
> --- a/Proxmox/Install.pm
> +++ b/Proxmox/Install.pm
> @@ -359,20 +359,16 @@ sub get_btrfs_raid_setup {
>
>       my $mode;
>
> -    if ($diskcount == 1) {
> -	$mode = 'single';
> +    if ($filesys eq 'btrfs (RAID0)') {
> +	$mode = 'raid0';
> +    } elsif ($filesys eq 'btrfs (RAID1)') {
> +	die "btrfs (RAID1) needs at least 2 devices\n" if $diskcount < 2;
> +	$mode = 'raid1';
> +    } elsif ($filesys eq 'btrfs (RAID10)') {
> +	die "btrfs (RAID10) needs at least 4 devices\n" if $diskcount < 4;
> +	$mode = 'raid10';
>       } else {
> -	if ($filesys eq 'btrfs (RAID0)') {
> -	    $mode = 'raid0';
> -	} elsif ($filesys eq 'btrfs (RAID1)') {
> -	    die "btrfs (RAID1) needs at least 2 devices\n" if $diskcount < 2;
> -	    $mode = 'raid1';
> -	} elsif ($filesys eq 'btrfs (RAID10)') {
> -	    die "btrfs (RAID10) needs at least 4 devices\n" if $diskcount < 4;
> -	    $mode = 'raid10';
> -	} else {
> -	    die "unknown btrfs mode '$filesys'\n";
> -	}
> +	die "unknown btrfs mode '$filesys'\n";
>       }
>
>       return ($devlist, $mode);



--
Questa email è stata esaminata alla ricerca di virus dal software antivirus Avast.
www.avast.com


[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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