all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: "DERUMIER, Alexandre via pve-devel" <pve-devel@lists.proxmox.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>,
	"f.gruenbichler@proxmox.com" <f.gruenbichler@proxmox.com>
Cc: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.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	[thread overview]
Message-ID: <mailman.222.1736518103.441.pve-devel@lists.proxmox.com> (raw)
In-Reply-To: <357523617.8078.1736346397754@webmail.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

  reply	other threads:[~2025-01-10 14:08 UTC|newest]

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20241216091229.3142660-1-alexandre.derumier@groupe-cyllene.com>
2024-12-16  9:12 ` [pve-devel] [PATCH v1 pve-qemu 1/1] add block-commit-replaces option patch Alexandre Derumier via pve-devel
2025-01-08 13:27   ` Fabian Grünbichler
2025-01-10  7:55     ` DERUMIER, Alexandre via pve-devel
     [not found]     ` <34a164520eba035d1db5f70761b0f4aa59fecfa5.camel@groupe-cyllene.com>
2025-01-10  9:15       ` Fiona Ebner
2025-01-10  9:32         ` DERUMIER, Alexandre via pve-devel
     [not found]         ` <1e45e756801843dd46eb6ce2958d30885ad73bc2.camel@groupe-cyllene.com>
2025-01-13 14:28           ` Fiona Ebner
2025-01-14 10:10             ` DERUMIER, Alexandre via pve-devel
2024-12-16  9:12 ` [pve-devel] [PATCH v3 qemu-server 01/11] blockdev: cmdline: convert drive to blockdev syntax Alexandre Derumier via pve-devel
2025-01-08 14:17   ` Fabian Grünbichler
2025-01-10 13:50     ` DERUMIER, Alexandre via pve-devel
2024-12-16  9:12 ` [pve-devel] [PATCH v3 pve-storage 1/3] qcow2: add external snapshot support Alexandre Derumier via pve-devel
2025-01-09 12:36   ` Fabian Grünbichler
2025-01-10  9:10     ` DERUMIER, Alexandre via pve-devel
     [not found]     ` <f25028d41a9588e82889b3ef869a14f33cbd216e.camel@groupe-cyllene.com>
2025-01-10 11:02       ` Fabian Grünbichler
2025-01-10 11:51         ` DERUMIER, Alexandre via pve-devel
     [not found]         ` <1caecaa23e5d57030a9e31f2f0e67648f1930d6a.camel@groupe-cyllene.com>
2025-01-10 12:20           ` Fabian Grünbichler
2025-01-10 13:14             ` DERUMIER, Alexandre via pve-devel
2024-12-16  9:12 ` [pve-devel] [PATCH v3 qemu-server 02/11] blockdev: fix cfg2cmd tests Alexandre Derumier via pve-devel
2024-12-16  9:12 ` [pve-devel] [PATCH v3 pve-storage 2/3] lvmplugin: add qcow2 snapshot Alexandre Derumier via pve-devel
2025-01-09 13:55   ` Fabian Grünbichler
2025-01-10 10:16     ` DERUMIER, Alexandre via pve-devel
2024-12-16  9:12 ` [pve-devel] [PATCH v3 qemu-server 03/11] blockdev : convert qemu_driveadd && qemu_drivedel Alexandre Derumier via pve-devel
2025-01-08 14:26   ` Fabian Grünbichler
2025-01-10 14:08     ` DERUMIER, Alexandre via pve-devel [this message]
2024-12-16  9:12 ` [pve-devel] [PATCH v3 pve-storage 3/3] storage: vdisk_free: remove external snapshots Alexandre Derumier via pve-devel
2024-12-16  9:12 ` [pve-devel] [PATCH v3 qemu-server 04/11] blockdev: vm_devices_list : fix block-query Alexandre Derumier via pve-devel
2025-01-08 14:31   ` Fabian Grünbichler
2025-01-13  7:56     ` DERUMIER, Alexandre via pve-devel
2024-12-16  9:12 ` [pve-devel] [PATCH v3 qemu-server 05/11] blockdev: convert cdrom media eject/insert Alexandre Derumier via pve-devel
2025-01-08 14:34   ` Fabian Grünbichler
2024-12-16  9:12 ` [pve-devel] [PATCH v3 qemu-server 06/11] blockdev: block_resize: convert to blockdev Alexandre Derumier via pve-devel
2024-12-16  9:12 ` [pve-devel] [PATCH v3 qemu-server 07/11] blockdev: nbd_export: block-export-add : use drive-$id for nodename Alexandre Derumier via pve-devel
2024-12-16  9:12 ` [pve-devel] [PATCH v3 qemu-server 08/11] blockdev: convert drive_mirror to blockdev_mirror Alexandre Derumier via pve-devel
2025-01-08 15:19   ` Fabian Grünbichler
2025-01-13  8:27     ` DERUMIER, Alexandre via pve-devel
     [not found]     ` <0d0d4c4d73110cf0e692cae0ee65bf7f9a6ce93a.camel@groupe-cyllene.com>
2025-01-13  9:52       ` Fabian Grünbichler
2025-01-13  9:55         ` Fabian Grünbichler
2025-01-13 10:47         ` DERUMIER, Alexandre via pve-devel
2025-01-13 13:42           ` Fiona Ebner
2025-01-14 10:03             ` DERUMIER, Alexandre via pve-devel
     [not found]             ` <fa38efbd95b57ba57a5628d6acfcda9d5875fa82.camel@groupe-cyllene.com>
2025-01-15  9:39               ` Fiona Ebner
2025-01-15  9:51                 ` Fabian Grünbichler
2025-01-15 10:06                   ` Fiona Ebner
2025-01-15 10:15                     ` Fabian Grünbichler
2025-01-15 10:46                       ` Fiona Ebner
2025-01-15 10:50                         ` Fabian Grünbichler
2025-01-15 11:01                           ` Fiona Ebner
2025-01-15 13:01                       ` DERUMIER, Alexandre via pve-devel
2025-01-16 14:56                     ` DERUMIER, Alexandre via pve-devel
2025-01-15 10:15                   ` DERUMIER, Alexandre via pve-devel
     [not found]         ` <c1559499319052d6cf10900efd5376c12389a60f.camel@groupe-cyllene.com>
2025-01-13 13:31           ` Fabian Grünbichler
2025-01-20 13:37             ` DERUMIER, Alexandre via pve-devel
2024-12-16  9:12 ` [pve-devel] [PATCH v3 qemu-server 09/11] blockdev: mirror: change aio on target if io_uring is not default Alexandre Derumier via pve-devel
2025-01-09  9:51   ` Fabian Grünbichler
2025-01-13  8:38     ` DERUMIER, Alexandre via pve-devel
2024-12-16  9:12 ` [pve-devel] [PATCH v3 qemu-server 10/11] blockdev: add backing_chain support Alexandre Derumier via pve-devel
2025-01-09 11:57   ` Fabian Grünbichler
2025-01-13  8:53     ` DERUMIER, Alexandre via pve-devel
2024-12-16  9:12 ` [pve-devel] [PATCH v3 qemu-server 11/11] qcow2: add external snapshot support Alexandre Derumier via pve-devel
2025-01-09 11:57   ` Fabian Grünbichler
2025-01-09 13:19     ` Fabio Fantoni via pve-devel
2025-01-20 13:44       ` DERUMIER, Alexandre via pve-devel
     [not found]       ` <3307ec388a763510ec78f97ed9f0de00c87d54b5.camel@groupe-cyllene.com>
2025-01-20 14:29         ` Fabio Fantoni via pve-devel
     [not found]         ` <6bdfe757-ae04-42e1-b197-c9ddb873e353@m2r.biz>
2025-01-20 14:41           ` DERUMIER, Alexandre via pve-devel
2025-01-13 10:08     ` DERUMIER, Alexandre via pve-devel
     [not found]     ` <0ae72889042e006d9202e837aac7ecf2b413e1b4.camel@groupe-cyllene.com>
2025-01-13 13:27       ` Fabian Grünbichler
2025-01-13 18:07         ` DERUMIER, Alexandre via pve-devel
2025-01-13 18:58           ` DERUMIER, Alexandre via pve-devel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=mailman.222.1736518103.441.pve-devel@lists.proxmox.com \
    --to=pve-devel@lists.proxmox.com \
    --cc=alexandre.derumier@groupe-cyllene.com \
    --cc=f.gruenbichler@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal