[pve-devel] [PATCH-SERIES qemu-server/storage] improve RBD resize

[pve-devel] [PATCH-SERIES qemu-server/storage] improve RBD resize

[Fiona Ebner]

Make the way the block_resize QMP command is used consistent with
what other block device backed storages like ZFS and LVM(thin) do.

Avoid the --allow-shrink flag that should never be required in our
code and avoid passing floating point numbers to the rbd resize
command.


qemu-server:

Fiona Ebner (1):
  block resize: avoid passing zero size to QMP command

 PVE/QemuServer.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


pve-storage:

Fiona Ebner (2):
  rbd: don't specify allow-shrink flag
  rbd: volume resize: avoid passing floating point value to rbd

 PVE/Storage/RBDPlugin.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.30.2

[pve-devel] [PATCH qemu-server 1/1] block resize: avoid passing zero size to QMP command

[Fiona Ebner]

Commit 7246e8f9 ("Set zero $size and continue if volume_resize()
returns false") mentions that this is needed for "some storages with
backing block devices to do online resize" and since this patch came
together [0] with pve-storage commit a4aee43 ("Fix RBD resize with
krbd option enabled."), it's safe to assume that RBD with krbd is
meant. But it should be the same situation for any external plugin
relying on the same behavior.

Other storages backed by block devices like LVM(-thin) and ZFS return
1 and the new size respectively, and the code is older than the above
mentioned commits. So really, the RBD plugin just should have returned
a positive value to be in-line with those and there should be no need
to pass 0 to the block_resize QMP command either.

Actually, it's a hack, because the block_resize QMP command does not
actually do special handling for the value 0. It's just that in the
case of a block device, QEMU won't try to resize it (and not fail for
shrinkage). But the size in the raw driver's BlockDriverState is
temporarily set to 0 (which is not nice), until the sector count is
refreshed, where raw_co_getlength is called, which queries the new
size and sets the size in the raw driver's BlockDriverState again as a
side effect. It's not known to cause any issues, but bdrv_getlength is
a coroutine wrapper starting from QEMU 8.0.0, and it's just better to
avoid setting a completely wrong value even temporarily. Just pass the
actually requested size like is done for LVM(thin) and ZFS.

[0]: https://lists.proxmox.com/pipermail/pve-devel/2017-January/025060.html

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index c1d0fd2d..86d88914 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4766,7 +4766,7 @@ sub qemu_block_resize {
 
     my $running = check_running($vmid);
 
-    $size = 0 if !PVE::Storage::volume_resize($storecfg, $volid, $size, $running);
+    PVE::Storage::volume_resize($storecfg, $volid, $size, $running);
 
     return if !$running;
 
-- 
2.30.2

[pve-devel] [PATCH storage 1/2] rbd: don't specify allow-shrink flag

[Fiona Ebner]

It was introduced by commit 4b7dd9d ("allow --allow-shrink on RBD
resize"), but doesn't give a rationale. A mail gives more[0],
indicating that the user also uses the function to shrink images.
However, the volume_resize function is only reachable via the resize
API endpoints for VMs and containers, which have an explicit check to
disallow shrinkage. If somebody really wants to shrink the image, just
let them use the storage's tools directly. Calling into Proxmox VE's
perl functions directly is not supported.

[0]: https://lists.proxmox.com/pipermail/pve-devel/2016-November/024077.html

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/Storage/RBDPlugin.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
index 73703fb..9d59a14 100644
--- a/PVE/Storage/RBDPlugin.pm
+++ b/PVE/Storage/RBDPlugin.pm
@@ -779,7 +779,7 @@ sub volume_resize {
 
     my ($vtype, $name, $vmid) = $class->parse_volname($volname);
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'resize', '--allow-shrink', '--size', ($size/1024/1024), $name);
+    my $cmd = $rbd_cmd->($scfg, $storeid, 'resize', '--size', ($size/1024/1024), $name);
     run_rbd_command($cmd, errmsg => "rbd resize '$volname' error");
     return undef;
 }
-- 
2.30.2

[pve-devel] [PATCH storage 2/2] rbd: volume resize: avoid passing floating point value to rbd

[Fiona Ebner]

which causes an error "the argument for option '--size' is invalid".
Just round up to the nearest integer to have at least the requested
size. This is similar to what is done for ZFS with d3e3e5d ("When
resizing a ZFS volume, align size to 1M") and makes commands like 'qm
resize 102 scsi1 +0.01G' work.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/Storage/RBDPlugin.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
index 9d59a14..c9e70a2 100644
--- a/PVE/Storage/RBDPlugin.pm
+++ b/PVE/Storage/RBDPlugin.pm
@@ -7,6 +7,7 @@ use Cwd qw(abs_path);
 use IO::File;
 use JSON;
 use Net::IP;
+use POSIX qw(ceil);
 
 use PVE::CephConfig;
 use PVE::Cluster qw(cfs_read_file);;
@@ -779,7 +780,7 @@ sub volume_resize {
 
     my ($vtype, $name, $vmid) = $class->parse_volname($volname);
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'resize', '--size', ($size/1024/1024), $name);
+    my $cmd = $rbd_cmd->($scfg, $storeid, 'resize', '--size', ceil($size/1024/1024), $name);
     run_rbd_command($cmd, errmsg => "rbd resize '$volname' error");
     return undef;
 }
-- 
2.30.2

Re: [pve-devel] [PATCH storage 2/2] rbd: volume resize: avoid passing floating point value to rbd

[Thomas Lamprecht]

Am 28/04/2023 um 14:32 schrieb Fiona Ebner:
> which causes an error "the argument for option '--size' is invalid".
> Just round up to the nearest integer to have at least the requested
> size. This is similar to what is done for ZFS with d3e3e5d ("When
> resizing a ZFS volume, align size to 1M") and makes commands like 'qm
> resize 102 scsi1 +0.01G' work.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  PVE/Storage/RBDPlugin.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index 9d59a14..c9e70a2 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -7,6 +7,7 @@ use Cwd qw(abs_path);
>  use IO::File;
>  use JSON;
>  use Net::IP;
> +use POSIX qw(ceil);
>  
>  use PVE::CephConfig;
>  use PVE::Cluster qw(cfs_read_file);;
> @@ -779,7 +780,7 @@ sub volume_resize {
>  
>      my ($vtype, $name, $vmid) = $class->parse_volname($volname);
>  
> -    my $cmd = $rbd_cmd->($scfg, $storeid, 'resize', '--size', ($size/1024/1024), $name);
> +    my $cmd = $rbd_cmd->($scfg, $storeid, 'resize', '--size', ceil($size/1024/1024), $name);


Hmm, but POSIX ceil is also returning a double `double ceil(double x)`, maybe wrap
that into an int(), hedging against (future) perl is often a relatively good idea ^^

FWIW: we often to something like int(($size + 1023)/1024/1024); (untested for this
specific case).


>      run_rbd_command($cmd, errmsg => "rbd resize '$volname' error");
>      return undef;
>  }

[pve-devel] applied-series: [PATCH-SERIES qemu-server/storage] improve RBD resize

[Thomas Lamprecht]

Am 28/04/2023 um 14:32 schrieb Fiona Ebner:
> Make the way the block_resize QMP command is used consistent with
> what other block device backed storages like ZFS and LVM(thin) do.
> 
> Avoid the --allow-shrink flag that should never be required in our
> code and avoid passing floating point numbers to the rbd resize
> command.
> 
> 
> qemu-server:
> 
> Fiona Ebner (1):
>   block resize: avoid passing zero size to QMP command
> 
>  PVE/QemuServer.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> pve-storage:
> 
> Fiona Ebner (2):
>   rbd: don't specify allow-shrink flag
>   rbd: volume resize: avoid passing floating point value to rbd
> 
>  PVE/Storage/RBDPlugin.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 


applied series, wrapping the ceil in an int(), just to be sure - thanks!

Re: [pve-devel] [PATCH storage 2/2] rbd: volume resize: avoid passing floating point value to rbd

[Fiona Ebner]

Am 06.06.23 um 19:37 schrieb Thomas Lamprecht:
> Am 28/04/2023 um 14:32 schrieb Fiona Ebner:
>>  
>> -    my $cmd = $rbd_cmd->($scfg, $storeid, 'resize', '--size', ($size/1024/1024), $name);
>> +    my $cmd = $rbd_cmd->($scfg, $storeid, 'resize', '--size', ceil($size/1024/1024), $name);
> 
> 
> Hmm, but POSIX ceil is also returning a double `double ceil(double x)`, maybe wrap
> that into an int(), hedging against (future) perl is often a relatively good idea ^^

Thanks! While testing worked, it does seem a bit brittle without the int().

> 
> FWIW: we often to something like int(($size + 1023)/1024/1024); (untested for this
> specific case).
> 

This would also have avoided the new "use" statement, will go for that
next time.