[pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes"

[pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes"

[Aaron Lauterer]

https://github.com/zfsonlinux/zfs/issues/8541 is solved and part of
openzfs 2.0 with [0]. Since we ship only ZFS 2.0 with PVE 7 we should be
okay to remove our workaround

[0] https://github.com/openzfs/zfs/commit/47c9299fcc9e5fb91d0b1636bfacc03bd3e98439

This reverts commit cdef3abb25984c369571626b38f97f92a0a2fd15.
---
 PVE/Storage/ZFSPoolPlugin.pm | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
index 2e2abe3..f719f42 100644
--- a/PVE/Storage/ZFSPoolPlugin.pm
+++ b/PVE/Storage/ZFSPoolPlugin.pm
@@ -313,12 +313,7 @@ sub zfs_get_pool_stats {
 
 sub zfs_create_zvol {
     my ($class, $scfg, $zvol, $size) = @_;
-
-    # always align size to 1M as workaround until
-    # https://github.com/zfsonlinux/zfs/issues/8541 is solved
-    my $padding = (1024 - $size % 1024) % 1024;
-    $size = $size + $padding;
-
+    
     my $cmd = ['create'];
 
     push @$cmd, '-s' if $scfg->{sparse};
-- 
2.30.2

Re: [pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes"

[Fabian Ebner]

Works for me. A tiny issue is that the disk size in the VM config does 
not show the actual value afterwards, but that is nothing new.

Tested-by: Fabian Ebner <f.ebner@proxmox.com>

Am 09.06.21 um 15:54 schrieb Aaron Lauterer:
> https://github.com/zfsonlinux/zfs/issues/8541 is solved and part of
> openzfs 2.0 with [0]. Since we ship only ZFS 2.0 with PVE 7 we should be
> okay to remove our workaround
> 
> [0] https://github.com/openzfs/zfs/commit/47c9299fcc9e5fb91d0b1636bfacc03bd3e98439
> 
> This reverts commit cdef3abb25984c369571626b38f97f92a0a2fd15.
> ---
>   PVE/Storage/ZFSPoolPlugin.pm | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
> index 2e2abe3..f719f42 100644
> --- a/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/PVE/Storage/ZFSPoolPlugin.pm
> @@ -313,12 +313,7 @@ sub zfs_get_pool_stats {
>   
>   sub zfs_create_zvol {
>       my ($class, $scfg, $zvol, $size) = @_;
> -
> -    # always align size to 1M as workaround until
> -    # https://github.com/zfsonlinux/zfs/issues/8541 is solved
> -    my $padding = (1024 - $size % 1024) % 1024;
> -    $size = $size + $padding;
> -
> +
>       my $cmd = ['create'];
>   
>       push @$cmd, '-s' if $scfg->{sparse};
> 

Re: [pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes"

[Thomas Lamprecht]

On 09.06.21 15:54, Aaron Lauterer wrote:
> https://github.com/zfsonlinux/zfs/issues/8541 is solved and part of
> openzfs 2.0 with [0]. Since we ship only ZFS 2.0 with PVE 7 we should be
> okay to remove our workaround
> 
> [0] https://github.com/openzfs/zfs/commit/47c9299fcc9e5fb91d0b1636bfacc03bd3e98439
> 
> This reverts commit cdef3abb25984c369571626b38f97f92a0a2fd15.
> ---
>  PVE/Storage/ZFSPoolPlugin.pm | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
> index 2e2abe3..f719f42 100644
> --- a/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/PVE/Storage/ZFSPoolPlugin.pm
> @@ -313,12 +313,7 @@ sub zfs_get_pool_stats {
>  
>  sub zfs_create_zvol {
>      my ($class, $scfg, $zvol, $size) = @_;
> -
> -    # always align size to 1M as workaround until
> -    # https://github.com/zfsonlinux/zfs/issues/8541 is solved
> -    my $padding = (1024 - $size % 1024) % 1024;
> -    $size = $size + $padding;
> -
> +    

above + line introduces a trailing whitespace error, please avoid that.

Can you please resend with that fixed and Fabi's T-b tag added?

>      my $cmd = ['create'];
>  
>      push @$cmd, '-s' if $scfg->{sparse};
> 

[pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes"

[Aaron Lauterer]

This reverts commit cdef3abb25984c369571626b38f97f92a0a2fd15.

The bug should be fixed by now [0]. The reproducer doesn't cause any
issues in my tests.

[0] https://github.com/openzfs/zfs/issues/8541

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
AFAICT this has an affect on EFI disks which after this revert will be
528k and not 1M. Similar as if we would store it as a .raw file.

I don't know about other tiny volume datasets. The TPM is 4M already.

 src/PVE/Storage/ZFSPoolPlugin.pm | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/PVE/Storage/ZFSPoolPlugin.pm b/src/PVE/Storage/ZFSPoolPlugin.pm
index 951d027..9a11d48 100644
--- a/src/PVE/Storage/ZFSPoolPlugin.pm
+++ b/src/PVE/Storage/ZFSPoolPlugin.pm
@@ -312,12 +312,7 @@ sub zfs_get_pool_stats {
 
 sub zfs_create_zvol {
     my ($class, $scfg, $zvol, $size) = @_;
-
-    # always align size to 1M as workaround until
-    # https://github.com/zfsonlinux/zfs/issues/8541 is solved
-    my $padding = (1024 - $size % 1024) % 1024;
-    $size = $size + $padding;
-
+    
     my $cmd = ['create'];
 
     push @$cmd, '-s' if $scfg->{sparse};
-- 
2.39.2

Re: [pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes"

[Thomas Lamprecht]

Am 14/06/2023 um 13:28 schrieb Aaron Lauterer:
> This reverts commit cdef3abb25984c369571626b38f97f92a0a2fd15.
> 
> The bug should be fixed by now [0]. The reproducer doesn't cause any
> issues in my tests.
> 
> [0] https://github.com/openzfs/zfs/issues/8541

hmm, torn on this one; 1 MB aligned images sound not to bad for various things,
and the extra size is rather negligible most of the time so we can mostly lose
here, otoh. it should be callers decision if storage works fine now..

> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> AFAICT this has an affect on EFI disks which after this revert will be
> 528k and not 1M. Similar as if we would store it as a .raw file.
> 

that sounds like it _could_ break stuff..

@fiona: what was the state with local storage migration and those disk size
mismatches? Or anything else coming to your mind?

Re: [pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes"

[Aaron Lauterer]

On 6/14/23 13:38, Thomas Lamprecht wrote:
> Am 14/06/2023 um 13:28 schrieb Aaron Lauterer:
>> This reverts commit cdef3abb25984c369571626b38f97f92a0a2fd15.
>>
>> The bug should be fixed by now [0]. The reproducer doesn't cause any
>> issues in my tests.
>>
>> [0] https://github.com/openzfs/zfs/issues/8541
> 
> hmm, torn on this one; 1 MB aligned images sound not to bad for various things,
> and the extra size is rather negligible most of the time so we can mostly lose
> here, otoh. it should be callers decision if storage works fine now..
> 
>>
>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>> ---
>> AFAICT this has an affect on EFI disks which after this revert will be
>> 528k and not 1M. Similar as if we would store it as a .raw file.
>>
> 
> that sounds like it _could_ break stuff..
> 
> @fiona: what was the state with local storage migration and those disk size
> mismatches? Or anything else coming to your mind?

I did a few tests in the meantime. An EFI disk on a directory based storage will 
be 528 K and can be moved to a ZFS storage with this patch. Without it, it will 
fail, similar to RBD which needs a 1M min size IIRC.

Re: [pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes"

[Fiona Ebner]

Am 14.06.23 um 13:44 schrieb Aaron Lauterer:
> On 6/14/23 13:38, Thomas Lamprecht wrote:
>> Am 14/06/2023 um 13:28 schrieb Aaron Lauterer:
>>> This reverts commit cdef3abb25984c369571626b38f97f92a0a2fd15.
>>>
>>> The bug should be fixed by now [0]. The reproducer doesn't cause any
>>> issues in my tests.
>>>
>>> [0] https://github.com/openzfs/zfs/issues/8541
>>
>> hmm, torn on this one; 1 MB aligned images sound not to bad for
>> various things,
>> and the extra size is rather negligible most of the time so we can
>> mostly lose
>> here, otoh. it should be callers decision if storage works fine now..
>>
>>>
>>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>>> ---
>>> AFAICT this has an affect on EFI disks which after this revert will be
>>> 528k and not 1M. Similar as if we would store it as a .raw file.
>>>
>>
>> that sounds like it _could_ break stuff..
>>
>> @fiona: what was the state with local storage migration and those disk
>> size
>> mismatches? Or anything else coming to your mind?
> 
> I did a few tests in the meantime. An EFI disk on a directory based
> storage will be 528 K and can be moved to a ZFS storage with this patch.
> Without it, it will fail, similar to RBD which needs a 1M min size IIRC.

Yes, drive mirror will fail if the source and target volume don't have
the exact same size: https://bugzilla.proxmox.com/show_bug.cgi?id=3227
so this would be an improvement. Although the proper fix for that bug
would need to be made in drive mirror (e.g. by adding an option to allow
larger target image).

Offline storage export/import for ZFS is currently limited to ZFS<->ZFS
anyways.

I'm not aware of any issues, but the alignment has been there for a
while now ;)

There's also similar padding in volume_resize(). Does ZFS also round up
automatically there now or do we need to keep that?

Re: [pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes"

[Aaron Lauterer]

On 6/14/23 14:13, Fiona Ebner wrote:
> Am 14.06.23 um 13:44 schrieb Aaron Lauterer:
>> On 6/14/23 13:38, Thomas Lamprecht wrote:
>>> Am 14/06/2023 um 13:28 schrieb Aaron Lauterer:
>>>> This reverts commit cdef3abb25984c369571626b38f97f92a0a2fd15.
>>>>
>>>> The bug should be fixed by now [0]. The reproducer doesn't cause any
>>>> issues in my tests.
>>>>
>>>> [0] https://github.com/openzfs/zfs/issues/8541
>>>
>>> hmm, torn on this one; 1 MB aligned images sound not to bad for
>>> various things,
>>> and the extra size is rather negligible most of the time so we can
>>> mostly lose
>>> here, otoh. it should be callers decision if storage works fine now..
>>>
>>>>
>>>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>>>> ---
>>>> AFAICT this has an affect on EFI disks which after this revert will be
>>>> 528k and not 1M. Similar as if we would store it as a .raw file.
>>>>
>>>
>>> that sounds like it _could_ break stuff..
>>>
>>> @fiona: what was the state with local storage migration and those disk
>>> size
>>> mismatches? Or anything else coming to your mind?
>>
>> I did a few tests in the meantime. An EFI disk on a directory based
>> storage will be 528 K and can be moved to a ZFS storage with this patch.
>> Without it, it will fail, similar to RBD which needs a 1M min size IIRC.
> 
> Yes, drive mirror will fail if the source and target volume don't have
> the exact same size: https://bugzilla.proxmox.com/show_bug.cgi?id=3227
> so this would be an improvement. Although the proper fix for that bug
> would need to be made in drive mirror (e.g. by adding an option to allow
> larger target image).
> 
> Offline storage export/import for ZFS is currently limited to ZFS<->ZFS
> anyways.
> 
> I'm not aware of any issues, but the alignment has been there for a
> while now ;)
> 
> There's also similar padding in volume_resize(). Does ZFS also round up
> automatically there now or do we need to keep that?

Doesn't look like it:
zfs create tank01/sizetest -V 35K
zfs set volsize=73K tank01/sizetest
cannot set property for 'tank01/sizetest': 'volsize' must be a multiple of 
volume block size (8K)


With that in mind, it is probably better to let things be as they are?