Re: [pve-devel] [PATCH v3 qemu-server 03/11] blockdev : convert qemu_driveadd && qemu_drivedel

["DERUMIER, Alexandre via pve-devel" <pve-devel@lists.proxmox.com>]

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

From: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>, "f.gruenbichler@proxmox.com" <f.gruenbichler@proxmox.com>
Subject: Re: [pve-devel] [PATCH v3 qemu-server 03/11] blockdev : convert qemu_driveadd && qemu_drivedel
Date: Fri, 10 Jan 2025 14:08:20 +0000
Message-ID: <a26394024a9767a4d602c07e362e670808b17fbb.camel@groupe-cyllene.com>

-------- Message initial --------
De: Fabian Grünbichler <f.gruenbichler@proxmox.com>
À: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Cc: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
Objet: Re: [pve-devel] [PATCH v3 qemu-server 03/11] blockdev : convert
qemu_driveadd && qemu_drivedel
Date: 08/01/2025 15:26:37


> Alexandre Derumier via pve-devel <pve-devel@lists.proxmox.com> hat am
> 16.12.2024 10:12 CET geschrieben:
> fixme/testme :
> PVE/VZDump/QemuServer.pm:    eval {
> PVE::QemuServer::qemu_drivedel($vmid, "tpmstate0-backup"); };
> 
> Signed-off-by: Alexandre Derumier <alexandre.derumier@groupe-
> cyllene.com>
> ---
>  PVE/QemuServer.pm | 64 +++++++++++++++++++++++++++++++++------------
> --
>  1 file changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 2832ed09..baf78ec0 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -1582,6 +1582,42 @@ sub print_drive_throttle_group {
>     return $throttle_group;
>  }
>  
> +sub generate_throttle_group {
> +    my ($drive) = @_;
> +
> +    my $drive_id = get_drive_id($drive);
> +
> +    my $throttle_group = { id => "throttle-drive-$drive_id" };
> +    my $limits = {};
> +
> +    foreach my $type (['', '-total'], [_rd => '-read'], [_wr => '-
> write']) {
> + my ($dir, $qmpname) = @$type;
> +
> + if (my $v = $drive->{"mbps$dir"}) {
> +     $limits->{"bps$qmpname"} = int($v*1024*1024);
> + }
> + if (my $v = $drive->{"mbps${dir}_max"}) {
> +     $limits->{"bps$qmpname-max"} = int($v*1024*1024);
> + }
> + if (my $v = $drive->{"bps${dir}_max_length"}) {
> +     $limits->{"bps$qmpname-max-length"} = int($v)
> + }
> + if (my $v = $drive->{"iops${dir}"}) {
> +     $limits->{"iops$qmpname"} = int($v);
> + }
> + if (my $v = $drive->{"iops${dir}_max"}) {
> +     $limits->{"iops$qmpname-max"} = int($v);
> + }
> + if (my $v = $drive->{"iops${dir}_max_length"}) {
> +     $limits->{"iops$qmpname-max-length"} = int($v);
> + }
> +   }
> +
> +   $throttle_group->{limits} = $limits;
> +
> +   return $throttle_group;

>>this and the corresponding print sub are exactly the same, so the
>>print sub could call this and join the limits with the `x-` prefix
>>added? 
yes we could merge them.

Currently, the command line can't defined complex qom object (this
should be available soon, qemu devs are working on it). That's why it's
using a different syntax with x-.


>>how does this interact with the qemu_block_set_io_throttle helper
>>used when updating the limits at runtime?
It's still working with block_set_io_throttle, where you define the
device. (the throttling value are passed to the topnode attached to the
device)


> +}
> +
>  sub generate_file_blockdev {
>      my ($storecfg, $drive, $nodename) = @_;
>  
> @@ -4595,32 +4631,22 @@ sub qemu_iothread_del {
>  }
>  
>  sub qemu_driveadd {
> -    my ($storecfg, $vmid, $device) = @_;
> +    my ($storecfg, $vmid, $drive) = @_;
>  
> -    my $kvmver = get_running_qemu_version($vmid);
> -    my $io_uring = min_version($kvmver, 6, 0);
> -    my $drive = print_drive_commandline_full($storecfg, $vmid,
> $device, undef, $io_uring);
> -    $drive =~ s/\\/\\\\/g;
> -    my $ret = PVE::QemuServer::Monitor::hmp_cmd($vmid, "drive_add
> auto \"$drive\"", 60);
> -
> -    # If the command succeeds qemu prints: "OK"
> -    return 1 if $ret =~ m/OK/s;
> +    my $drive_id = get_drive_id($drive);
> +    my $throttle_group = generate_throttle_group($drive);

>>do we always need a throttle group? or would we benefit from only
>>adding it when limits are set, and skip that node when I/O is
>>unlimited?

It's adding a lot of complexity without it, because it's not always
possible to insert a new blockdev (here throttlegroup) between the
device and the drive blockdev, when the blockdev is already the top
node attached to the device

the other benefit is to have a stable name for top blocknode. 
(drive node names can change when you switch).  (less lookup for some
qmp action, like mirror/commit for example where you need to known the
top node nodename)


They a no performance impact to have a throttle group without limit




[-- 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