public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes"
@ 2021-06-09 13:54 Aaron Lauterer
  2021-06-10 12:33 ` Fabian Ebner
  2021-06-11  9:28 ` Thomas Lamprecht
  0 siblings, 2 replies; 8+ messages in thread
From: Aaron Lauterer @ 2021-06-09 13:54 UTC (permalink / raw)
  To: pve-devel

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





^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes"
  2021-06-09 13:54 [pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes" Aaron Lauterer
@ 2021-06-10 12:33 ` Fabian Ebner
  2021-06-11  9:28 ` Thomas Lamprecht
  1 sibling, 0 replies; 8+ messages in thread
From: Fabian Ebner @ 2021-06-10 12:33 UTC (permalink / raw)
  To: pve-devel

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};
> 




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes"
  2021-06-09 13:54 [pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes" Aaron Lauterer
  2021-06-10 12:33 ` Fabian Ebner
@ 2021-06-11  9:28 ` Thomas Lamprecht
  1 sibling, 0 replies; 8+ messages in thread
From: Thomas Lamprecht @ 2021-06-11  9:28 UTC (permalink / raw)
  To: Proxmox VE development discussion, Aaron Lauterer

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};
> 





^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes"
  2023-06-14 12:13     ` Fiona Ebner
@ 2023-06-14 12:26       ` Aaron Lauterer
  0 siblings, 0 replies; 8+ messages in thread
From: Aaron Lauterer @ 2023-06-14 12:26 UTC (permalink / raw)
  To: Fiona Ebner, Thomas Lamprecht, Proxmox VE development discussion



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?




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes"
  2023-06-14 11:44   ` Aaron Lauterer
@ 2023-06-14 12:13     ` Fiona Ebner
  2023-06-14 12:26       ` Aaron Lauterer
  0 siblings, 1 reply; 8+ messages in thread
From: Fiona Ebner @ 2023-06-14 12:13 UTC (permalink / raw)
  To: Aaron Lauterer, Thomas Lamprecht, Proxmox VE development discussion

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?




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes"
  2023-06-14 11:38 ` Thomas Lamprecht
@ 2023-06-14 11:44   ` Aaron Lauterer
  2023-06-14 12:13     ` Fiona Ebner
  0 siblings, 1 reply; 8+ messages in thread
From: Aaron Lauterer @ 2023-06-14 11:44 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox VE development discussion



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.




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes"
  2023-06-14 11:28 Aaron Lauterer
@ 2023-06-14 11:38 ` Thomas Lamprecht
  2023-06-14 11:44   ` Aaron Lauterer
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Lamprecht @ 2023-06-14 11:38 UTC (permalink / raw)
  To: Proxmox VE development discussion, Aaron Lauterer

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?




^ permalink raw reply	[flat|nested] 8+ messages in thread

* [pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes"
@ 2023-06-14 11:28 Aaron Lauterer
  2023-06-14 11:38 ` Thomas Lamprecht
  0 siblings, 1 reply; 8+ messages in thread
From: Aaron Lauterer @ 2023-06-14 11:28 UTC (permalink / raw)
  To: pve-devel

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





^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-06-14 12:26 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-09 13:54 [pve-devel] [PATCH storage] Revert "workaround zfs create -V error for unaligned sizes" Aaron Lauterer
2021-06-10 12:33 ` Fabian Ebner
2021-06-11  9:28 ` Thomas Lamprecht
2023-06-14 11:28 Aaron Lauterer
2023-06-14 11:38 ` Thomas Lamprecht
2023-06-14 11:44   ` Aaron Lauterer
2023-06-14 12:13     ` Fiona Ebner
2023-06-14 12:26       ` Aaron Lauterer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal