[pve-devel] [PATCH-SERIES qemu-server 00/22] preparation for switch to blockdev

[pve-devel] [PATCH-SERIES qemu-server 00/22] preparation for switch to blockdev

[Fiona Ebner]

This is a preparatory series for the switch to -blockdev with Proxmox
VE 9, QEMU machine version 10.0, based in part on Alexandre's series
here [0].

While the last patch actually does the switch, many operations are not
yet supported. It is included to show what changes I made there. It
should not yet be applied and supporting everything is the goal for a
following series based on the rest of Alexandre's patches. It was time
to send this however, as it's already gotten large enough (but most
patches are quite small, so don't worry reviewers ;)).

There are mostly cleanups, moving and defining helpers, as well as
doing non-intrusive preparation for the switch with machine version
10.0. Quite a large chunk of the series is for having
activate_volumes() be called before config_to_command() with proper
cleanup handling.

A breaking change is the removal of the gone-since-QEMU-3.1 drive
geometry options. It was not possible to even start or backup a VM
with those options set since then. Still, restore of old backups with
such an option is made explicitly supported.

[0]: https://lore.proxmox.com/pve-devel/mailman.347.1749728041.395.pve-devel@lists.proxmox.com/T/

Fiona Ebner (22):
  drive: code cleanup: drop unused $vmid parameter from
    get_path_and_format()
  cfg2cmd: require at least QEMU binary version 6.0
  drive: parse: use hash argument for optional parameters
  drive: parse drive: support parsing with additional properties
  restore: parse drive with additional properties
  drive: remove geometry options gone since QEMU 3.1
  clone disk: io uring check: fix call to determine cache direct
  drive: move storage_allows_io_uring_default() and
    drive_uses_cache_direct() helpers to drive module
  drive: introduce aio_cmdline_option() helper
  drive: introduce detect_zeroes_cmdline_option() helper
  introduce StateFile module for state file related helpers
  vm start: move state file handling to dedicated module
  vm start: move config_to_command() call further down
  vm start/commandline: also clean up pci reservation when
    config_to_command() fails
  vm start/commandline: activate volumes before config_to_command()
  print drive device: explicitly set write-cache starting with machine
    version 10.0
  print drive device: set {r,w}error front-end properties starting with
    machine version 10.0
  print drive device: don't reference any drive for 'none' starting with
    machine version 10.0
  drive: create a throttle group for each drive starting with machine
    version 10.0
  blockdev: add helpers to generate blockdev commandline
  blockdev: add support for NBD paths
  command line: switch to blockdev starting with machine version 10.0

 PVE/API2/Qemu.pm                 |  12 +-
 PVE/QemuServer.pm                | 480 +++++++++++++++++--------------
 PVE/QemuServer/Blockdev.pm       | 229 +++++++++++++++
 PVE/QemuServer/Drive.pm          | 110 +++++--
 PVE/QemuServer/Makefile          |   3 +
 PVE/QemuServer/StateFile.pm      |  88 ++++++
 test/cfg2cmd/aio.conf.cmd        |  16 +-
 test/cfg2cmd/old-qemu.conf       |   4 +-
 test/run_config2command_tests.pl |  10 +
 9 files changed, 689 insertions(+), 263 deletions(-)
 create mode 100644 PVE/QemuServer/Blockdev.pm
 create mode 100644 PVE/QemuServer/StateFile.pm

-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 01/22] drive: code cleanup: drop unused $vmid parameter from get_path_and_format()

[Fiona Ebner]

The parameter is unused since commit f97b3da1 ("drive: code cleanup:
remove unused $vmid argument from get_iso_path() helper").

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm       | 6 +++---
 PVE/QemuServer/Drive.pm | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 577959a4..bf0c8bfe 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1452,7 +1452,7 @@ sub print_drive_commandline_full {
     my $vtype = $storeid ? (PVE::Storage::parse_volname($storecfg, $drive->{file}))[0] : undef;
 
     my ($path, $format) = PVE::QemuServer::Drive::get_path_and_format(
-	$storecfg, $vmid, $drive, $live_restore_name);
+	$storecfg, $drive, $live_restore_name);
 
     my $is_rbd = $path =~ m/^rbd:/;
 
@@ -5410,7 +5410,7 @@ sub vmconfig_update_disk {
 		}
 	    } else {
 		my ($path, $format) = PVE::QemuServer::Drive::get_path_and_format(
-		    $storecfg, $vmid, $drive);
+		    $storecfg, $drive);
 
 		# force eject if locked
 		mon_cmd($vmid, "eject", force => JSON::true, id => "$opt");
@@ -5460,7 +5460,7 @@ sub vmconfig_update_cloudinit_drive {
 
     if ($running) {
 	my ($path, $format) = PVE::QemuServer::Drive::get_path_and_format(
-	    $storecfg, $vmid, $cloudinit_drive);
+	    $storecfg, $cloudinit_drive);
 	if ($path) {
 	    mon_cmd($vmid, "eject", force => JSON::true, id => "$cloudinit_ds");
 	    mon_cmd(
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index 473970c1..878e1faa 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -103,7 +103,7 @@ sub get_iso_path {
 # Returns the path that can be used on the QEMU commandline and in QMP commands as well as the
 # checked format of the drive.
 sub get_path_and_format {
-    my ($storecfg, $vmid, $drive, $live_restore_name) = @_;
+    my ($storecfg, $drive, $live_restore_name) = @_;
 
     my $path;
     my $volid = $drive->{file};
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 02/22] cfg2cmd: require at least QEMU binary version 6.0

[Fiona Ebner]

The minimum supported binary version for Proxmox VE 9 is QEMU 10.0, so
there still is more than enough room for eventual regression testing
with older binaries.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm          | 15 ++++++---------
 test/cfg2cmd/old-qemu.conf |  4 ++--
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index bf0c8bfe..58a5cfa8 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1443,7 +1443,7 @@ my sub drive_uses_cache_direct {
 }
 
 sub print_drive_commandline_full {
-    my ($storecfg, $vmid, $drive, $live_restore_name, $io_uring) = @_;
+    my ($storecfg, $vmid, $drive, $live_restore_name) = @_;
 
     my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
 
@@ -1508,7 +1508,7 @@ sub print_drive_commandline_full {
     $opts .= ",cache=none" if !$drive->{cache} && $cache_direct;
 
     if (!$drive->{aio}) {
-	if ($io_uring && storage_allows_io_uring_default($scfg, $cache_direct)) {
+	if (storage_allows_io_uring_default($scfg, $cache_direct)) {
 	    # io_uring supports all cache modes
 	    $opts .= ",aio=io_uring";
 	} else {
@@ -3478,9 +3478,9 @@ sub config_to_command {
     my $kvm_binary = PVE::QemuServer::Helpers::get_command_for_arch($arch);
     my $kvmver = kvm_user_version($kvm_binary);
 
-    if (!$kvmver || $kvmver !~ m/^(\d+)\.(\d+)/ || $1 < 5) {
+    if (!$kvmver || $kvmver !~ m/^(\d+)\.(\d+)/ || $1 < 6) {
 	$kvmver //= "undefined";
-	die "Detected old QEMU binary ('$kvmver', at least 5.0 is required)\n";
+	die "Detected old QEMU binary ('$kvmver', at least 6.0 is required)\n";
     }
 
     my $machine_type = PVE::QemuServer::Machine::get_vm_machine($conf, $forcemachine, $arch);
@@ -3960,8 +3960,7 @@ sub config_to_command {
 	    push @$devices, '-blockdev', $live_restore->{blockdev};
 	}
 
-	my $drive_cmd = print_drive_commandline_full(
-	    $storecfg, $vmid, $drive, $live_blockdev_name, min_version($kvmver, 6, 0));
+	my $drive_cmd = print_drive_commandline_full($storecfg, $vmid, $drive, $live_blockdev_name);
 
 	# extra protection for templates, but SATA and IDE don't support it..
 	$drive_cmd .= ',readonly=on' if drive_is_read_only($conf, $drive);
@@ -4334,9 +4333,7 @@ sub qemu_iothread_del {
 sub qemu_driveadd {
     my ($storecfg, $vmid, $device) = @_;
 
-    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);
+    my $drive = print_drive_commandline_full($storecfg, $vmid, $device, undef);
     $drive =~ s/\\/\\\\/g;
     my $ret = PVE::QemuServer::Monitor::hmp_cmd($vmid, "drive_add auto \"$drive\"", 60);
 
diff --git a/test/cfg2cmd/old-qemu.conf b/test/cfg2cmd/old-qemu.conf
index e8129cf6..e560cac7 100644
--- a/test/cfg2cmd/old-qemu.conf
+++ b/test/cfg2cmd/old-qemu.conf
@@ -1,4 +1,4 @@
 # TEST: Test QEMU version detection and expect fail on old version
-# QEMU_VERSION: 4.0.1
-# EXPECT_ERROR: Detected old QEMU binary ('4.0.1', at least 5.0 is required)
+# QEMU_VERSION: 5.0.1
+# EXPECT_ERROR: Detected old QEMU binary ('5.0.1', at least 6.0 is required)
 smbios1: uuid=7b10d7af-b932-4c66-b2c3-3996152ec465
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 03/22] drive: parse: use hash argument for optional parameters

[Fiona Ebner]

In preparation to add a new one.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu.pm        | 12 +++++++-----
 PVE/QemuServer/Drive.pm |  6 ++++--
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 626cce45..003934ef 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -88,7 +88,8 @@ my $foreach_volume_with_alloc = sub {
     for my $opt (sort keys $param->%*) {
 	next if !PVE::QemuServer::is_valid_drivename($opt);
 
-	my $drive = PVE::QemuServer::Drive::parse_drive($opt, $param->{$opt}, 1);
+	my $drive = PVE::QemuServer::Drive::parse_drive(
+	    $opt, $param->{$opt}, { 'with-alloc' => 1 });
 	next if !$drive;
 
 	$func->($opt, $drive);
@@ -101,7 +102,7 @@ my $check_drive_param = sub {
     for my $opt (sort keys $param->%*) {
 	next if !PVE::QemuServer::is_valid_drivename($opt);
 
-	my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
+	my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, { 'with-alloc' => 1 });
 	raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive;
 
 	if ($drive->{'import-from'}) {
@@ -917,7 +918,7 @@ my $check_vm_modify_config_perm = sub {
 sub assert_scsi_feature_compatibility {
     my ($opt, $conf, $storecfg, $drive_attributes) = @_;
 
-    my $drive = PVE::QemuServer::Drive::parse_drive($opt, $drive_attributes, 1);
+    my $drive = PVE::QemuServer::Drive::parse_drive($opt, $drive_attributes, { 'with-alloc' => 1 });
 
     my $machine_type = PVE::QemuServer::Machine::get_vm_machine($conf, undef, $conf->{arch});
     my $machine_version = PVE::QemuServer::Machine::extract_version(
@@ -2028,7 +2029,7 @@ my $update_vm_api  = sub {
 
 	    my $check_drive_perms = sub {
 		my ($opt, $val) = @_;
-		my $drive = PVE::QemuServer::parse_drive($opt, $val, 1);
+		my $drive = PVE::QemuServer::parse_drive($opt, $val, { 'with-alloc' => 1 });
 		if (PVE::QemuServer::drive_is_cloudinit($drive)) {
 		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Cloudinit', 'VM.Config.CDROM']);
 		} elsif (PVE::QemuServer::drive_is_cdrom($drive, 1)) { # CDROM
@@ -2165,7 +2166,8 @@ my $update_vm_api  = sub {
 		    # default legacy boot order implies all cdroms anyway
 		    if (@bootorder) {
 			# append new CD drives to bootorder to mark them bootable
-			my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
+			my $drive = PVE::QemuServer::parse_drive(
+			    $opt, $param->{$opt}, { 'with-alloc' => 1 });
 			if (PVE::QemuServer::drive_is_cdrom($drive, 1) && !grep(/^$opt$/, @bootorder)) {
 			    push @bootorder, $opt;
 			    $conf->{pending}->{boot} = PVE::QemuServer::print_bootorder(\@bootorder);
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index 878e1faa..eaa77234 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -707,7 +707,9 @@ sub drive_is_read_only {
 #        [,iothread=on][,serial=serial][,model=model]
 
 sub parse_drive {
-    my ($key, $data, $with_alloc) = @_;
+    my ($key, $data, $parse_opts) = @_;
+
+    $parse_opts //= {};
 
     my ($interface, $index);
 
@@ -718,7 +720,7 @@ sub parse_drive {
 	return;
     }
 
-    my $desc_hash = $with_alloc ? $drivedesc_hash_with_alloc : $drivedesc_hash;
+    my $desc_hash = $parse_opts->{'with-alloc'} ? $drivedesc_hash_with_alloc : $drivedesc_hash;
 
     if (!defined($desc_hash->{$key})) {
 	warn "invalid drive key: $key\n";
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 04/22] drive: parse drive: support parsing with additional properties

[Fiona Ebner]

This will be useful for backwards-compat for restore to allow dropping
some drive properties that are long gone from QEMU.

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

diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index eaa77234..297f6bef 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -728,7 +728,10 @@ sub parse_drive {
     }
 
     my $desc = $desc_hash->{$key}->{format};
-    my $res = eval { PVE::JSONSchema::parse_property_string($desc, $data) };
+    my $res = eval {
+	PVE::JSONSchema::parse_property_string(
+	    $desc, $data, undef, $parse_opts->{'additional-properties'});
+    };
     return if !$res;
     $res->{interface} = $interface;
     $res->{index} = $index;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 05/22] restore: parse drive with additional properties

[Fiona Ebner]

For backwards-compat, to allow dropping some drive properties that are
long gone from QEMU.

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 58a5cfa8..5408ba8a 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6818,7 +6818,7 @@ sub restore_update_config_line {
     } elsif ($line =~ m/^((ide|scsi|virtio|sata|efidisk|tpmstate)\d+):\s*(\S+)\s*$/) {
 	my $virtdev = $1;
 	my $value = $3;
-	my $di = parse_drive($virtdev, $value);
+	my $di = parse_drive($virtdev, $value, { 'additional-properties' => 1 });
 	if (defined($di->{backup}) && !$di->{backup}) {
 	    $res .= "#$line";
 	} elsif ($map->{$virtdev}) {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 06/22] drive: remove geometry options gone since QEMU 3.1

[Fiona Ebner]

It was not possible to start a QEMU instance with these options set
since QEMU version 3.1, QEMU commit b24ec3c462 ("block: Remove
deprecated -drive geometry options") and thus also not to take a
backup. It is still possible to restore an ancient backup with these
options set and they will be automatically dropped.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm       |  2 +-
 PVE/QemuServer/Drive.pm | 30 +++++-------------------------
 2 files changed, 6 insertions(+), 26 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 5408ba8a..3803e64e 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1457,7 +1457,7 @@ sub print_drive_commandline_full {
     my $is_rbd = $path =~ m/^rbd:/;
 
     my $opts = '';
-    my @qemu_drive_options = qw(heads secs cyls trans media cache rerror werror aio discard);
+    my @qemu_drive_options = qw(media cache rerror werror aio discard);
     foreach my $o (@qemu_drive_options) {
 	$opts .= ",$o=$drive->{$o}" if defined($drive->{$o});
     }
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index 297f6bef..c7fd29e3 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -170,27 +170,6 @@ my %drivedesc_base = (
 	default => 'disk',
 	optional => 1
     },
-    cyls => {
-	type => 'integer',
-	description => "Force the drive's physical geometry to have a specific cylinder count.",
-	optional => 1
-    },
-    heads => {
-	type => 'integer',
-	description => "Force the drive's physical geometry to have a specific head count.",
-	optional => 1
-    },
-    secs => {
-	type => 'integer',
-	description => "Force the drive's physical geometry to have a specific sector count.",
-	optional => 1
-    },
-    trans => {
-	type => 'string',
-	enum => [qw(none lba auto)],
-	description => "Force disk geometry bios translation mode.",
-	optional => 1,
-    },
     snapshot => {
 	type => 'boolean',
 	description => "Controls qemu's snapshot mode feature."
@@ -700,7 +679,7 @@ sub drive_is_read_only {
     return $drive->{interface} ne 'sata' && $drive->{interface} ne 'ide';
 }
 
-# ideX = [volume=]volume-id[,media=d][,cyls=c,heads=h,secs=s[,trans=t]]
+# ideX = [volume=]volume-id[,media=d]
 #        [,snapshot=on|off][,cache=on|off][,format=f][,backup=yes|no]
 #        [,rerror=ignore|report|stop][,werror=enospc|ignore|report|stop]
 #        [,aio=native|threads][,discard=ignore|on][,detect_zeroes=on|off]
@@ -777,8 +756,7 @@ sub parse_drive {
     return if $res->{iops_wr} && $res->{iops};
 
     if ($res->{media} && ($res->{media} eq 'cdrom')) {
-	return if $res->{snapshot} || $res->{trans} || $res->{format};
-	return if $res->{heads} || $res->{secs} || $res->{cyls};
+	return if $res->{snapshot} || $res->{format};
 	return if $res->{interface} eq 'virtio';
     }
 
@@ -791,7 +769,9 @@ sub parse_drive {
 
 sub print_drive {
     my ($drive, $with_alloc) = @_;
-    my $skip = [ 'index', 'interface' ];
+    # clys, heads, secs and trans were removed in QEMU 3.1, but they could still appear in old
+    # backups, so skip them for restore
+    my $skip = [ 'index', 'interface', 'cyls', 'heads', 'secs', 'trans' ];
     my $fmt = $with_alloc ? $alldrive_fmt_with_alloc : $alldrive_fmt;
     return PVE::JSONSchema::print_property_string($drive, $fmt, $skip);
 }
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 07/22] clone disk: io uring check: fix call to determine cache direct

[Fiona Ebner]

The $scfg parameter for drive_uses_cache_direct() is only relevant for
BTRFS and the io_uring default does not depend on the cache setting
for BTRFS, so it currently doesn't matter if the parameter is passed
along or not here. Still, fix it for the future.

Fixes: 8fbae1dc ("fix #4525: clone disk: disallow mirror if it might cause problems with io_uring")
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 3803e64e..616afd7c 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -8415,7 +8415,7 @@ my sub clone_disk_check_io_uring {
     my $src_scfg = PVE::Storage::storage_config($storecfg, $src_storeid);
     my $dst_scfg = PVE::Storage::storage_config($storecfg, $dst_storeid);
 
-    my $cache_direct = drive_uses_cache_direct($src_drive);
+    my $cache_direct = drive_uses_cache_direct($src_drive, $src_scfg);
 
     my $src_uses_io_uring;
     if ($src_drive->{aio}) {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 08/22] drive: move storage_allows_io_uring_default() and drive_uses_cache_direct() helpers to drive module

[Fiona Ebner]

Suggested-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm       | 46 +++++++++++------------------------------
 PVE/QemuServer/Drive.pm | 33 +++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 34 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 616afd7c..24b791e8 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -55,7 +55,16 @@ use PVE::QemuServer::Helpers qw(config_aware_timeout min_version kvm_user_versio
 use PVE::QemuServer::Cloudinit;
 use PVE::QemuServer::CGroup;
 use PVE::QemuServer::CPUConfig qw(print_cpu_device get_cpu_options get_cpu_bitness is_native_arch get_amd_sev_object get_amd_sev_type);
-use PVE::QemuServer::Drive qw(is_valid_drivename checked_volume_format drive_is_cloudinit drive_is_cdrom drive_is_read_only parse_drive print_drive);
+use PVE::QemuServer::Drive qw(
+is_valid_drivename
+checked_volume_format
+drive_is_cloudinit
+drive_is_cdrom
+drive_is_read_only
+parse_drive
+print_drive
+storage_allows_io_uring_default
+);
 use PVE::QemuServer::Machine;
 use PVE::QemuServer::Memory qw(get_current_memory);
 use PVE::QemuServer::MetaInfo;
@@ -1411,37 +1420,6 @@ sub get_initiator_name {
     return $initiator;
 }
 
-my sub storage_allows_io_uring_default {
-    my ($scfg, $cache_direct) = @_;
-
-    # io_uring with cache mode writeback or writethrough on krbd will hang...
-    return if $scfg && $scfg->{type} eq 'rbd' && $scfg->{krbd} && !$cache_direct;
-
-    # io_uring with cache mode writeback or writethrough on LVM will hang, without cache only
-    # sometimes, just plain disable...
-    return if $scfg && $scfg->{type} eq 'lvm';
-
-    # io_uring causes problems when used with CIFS since kernel 5.15
-    # Some discussion: https://www.spinics.net/lists/linux-cifs/msg26734.html
-    return if $scfg && $scfg->{type} eq 'cifs';
-
-    return 1;
-}
-
-my sub drive_uses_cache_direct {
-    my ($drive, $scfg) = @_;
-
-    my $cache_direct = 0;
-
-    if (my $cache = $drive->{cache}) {
-	$cache_direct = $cache =~ /^(?:off|none|directsync)$/;
-    } elsif (!drive_is_cdrom($drive) && !($scfg && $scfg->{type} eq 'btrfs' && !$scfg->{nocow})) {
-	$cache_direct = 1;
-    }
-
-    return $cache_direct;
-}
-
 sub print_drive_commandline_full {
     my ($storecfg, $vmid, $drive, $live_restore_name) = @_;
 
@@ -1503,7 +1481,7 @@ sub print_drive_commandline_full {
 	$opts .= ",format=$format";
     }
 
-    my $cache_direct = drive_uses_cache_direct($drive, $scfg);
+    my $cache_direct = PVE::QemuServer::Drive::drive_uses_cache_direct($drive, $scfg);
 
     $opts .= ",cache=none" if !$drive->{cache} && $cache_direct;
 
@@ -8415,7 +8393,7 @@ my sub clone_disk_check_io_uring {
     my $src_scfg = PVE::Storage::storage_config($storecfg, $src_storeid);
     my $dst_scfg = PVE::Storage::storage_config($storecfg, $dst_storeid);
 
-    my $cache_direct = drive_uses_cache_direct($src_drive, $src_scfg);
+    my $cache_direct = PVE::QemuServer::Drive::drive_uses_cache_direct($src_drive, $src_scfg);
 
     my $src_uses_io_uring;
     if ($src_drive->{aio}) {
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index c7fd29e3..7caa5502 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -24,6 +24,7 @@ drive_is_read_only
 get_scsi_devicetype
 parse_drive
 print_drive
+storage_allows_io_uring_default
 );
 
 our $QEMU_FORMAT_RE = qr/raw|qcow|qcow2|qed|vmdk|cloop/;
@@ -983,4 +984,36 @@ sub get_scsi_device_type {
 
     return $devicetype;
 }
+
+sub storage_allows_io_uring_default {
+    my ($scfg, $cache_direct) = @_;
+
+    # io_uring with cache mode writeback or writethrough on krbd will hang...
+    return if $scfg && $scfg->{type} eq 'rbd' && $scfg->{krbd} && !$cache_direct;
+
+    # io_uring with cache mode writeback or writethrough on LVM will hang, without cache only
+    # sometimes, just plain disable...
+    return if $scfg && $scfg->{type} eq 'lvm';
+
+    # io_uring causes problems when used with CIFS since kernel 5.15
+    # Some discussion: https://www.spinics.net/lists/linux-cifs/msg26734.html
+    return if $scfg && $scfg->{type} eq 'cifs';
+
+    return 1;
+}
+
+sub drive_uses_cache_direct {
+    my ($drive, $scfg) = @_;
+
+    my $cache_direct = 0;
+
+    if (my $cache = $drive->{cache}) {
+	$cache_direct = $cache =~ /^(?:off|none|directsync)$/;
+    } elsif (!drive_is_cdrom($drive) && !($scfg && $scfg->{type} eq 'btrfs' && !$scfg->{nocow})) {
+	$cache_direct = 1;
+    }
+
+    return $cache_direct;
+}
+
 1;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 09/22] drive: introduce aio_cmdline_option() helper

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm         | 17 +++--------------
 PVE/QemuServer/Drive.pm   | 18 ++++++++++++++++++
 test/cfg2cmd/aio.conf.cmd | 16 ++++++++--------
 3 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 24b791e8..a2e51849 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1435,7 +1435,7 @@ sub print_drive_commandline_full {
     my $is_rbd = $path =~ m/^rbd:/;
 
     my $opts = '';
-    my @qemu_drive_options = qw(media cache rerror werror aio discard);
+    my @qemu_drive_options = qw(media cache rerror werror discard);
     foreach my $o (@qemu_drive_options) {
 	$opts .= ",$o=$drive->{$o}" if defined($drive->{$o});
     }
@@ -1485,19 +1485,8 @@ sub print_drive_commandline_full {
 
     $opts .= ",cache=none" if !$drive->{cache} && $cache_direct;
 
-    if (!$drive->{aio}) {
-	if (storage_allows_io_uring_default($scfg, $cache_direct)) {
-	    # io_uring supports all cache modes
-	    $opts .= ",aio=io_uring";
-	} else {
-	    # aio native works only with O_DIRECT
-	    if($cache_direct) {
-		$opts .= ",aio=native";
-	    } else {
-		$opts .= ",aio=threads";
-	    }
-	}
-    }
+    my $aio = PVE::QemuServer::Drive::aio_cmdline_option($scfg, $drive, $cache_direct);
+    $opts .= ",aio=$aio";
 
     die "$drive_id: explicit media parameter is required for iso images\n"
 	if !defined($drive->{media}) && defined($vtype) && $vtype eq 'iso';
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index 7caa5502..f7611662 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -1016,4 +1016,22 @@ sub drive_uses_cache_direct {
     return $cache_direct;
 }
 
+sub aio_cmdline_option {
+    my ($scfg, $drive, $cache_direct) = @_;
+
+    return $drive->{aio} if $drive->{aio};
+
+    if (storage_allows_io_uring_default($scfg, $cache_direct)) {
+	# io_uring supports all cache modes
+	return 'io_uring';
+    } else {
+	# aio native works only with O_DIRECT
+	if ($cache_direct) {
+	    return 'native';
+	} else {
+	    return 'threads';
+	}
+    }
+}
+
 1;
diff --git a/test/cfg2cmd/aio.conf.cmd b/test/cfg2cmd/aio.conf.cmd
index 851cb90b..9d29a34f 100644
--- a/test/cfg2cmd/aio.conf.cmd
+++ b/test/cfg2cmd/aio.conf.cmd
@@ -24,33 +24,33 @@
   -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3,free-page-reporting=on' \
   -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
   -device 'lsi,id=scsihw0,bus=pci.0,addr=0x5' \
-  -drive 'file=/var/lib/vz/images/8006/vm-8006-disk-0.raw,if=none,id=drive-scsi0,aio=threads,discard=on,format=raw,cache=none,detect-zeroes=unmap' \
+  -drive 'file=/var/lib/vz/images/8006/vm-8006-disk-0.raw,if=none,id=drive-scsi0,discard=on,format=raw,cache=none,aio=threads,detect-zeroes=unmap' \
   -device 'scsi-hd,bus=scsihw0.0,scsi-id=0,drive=drive-scsi0,id=scsi0' \
-  -drive 'file=/var/lib/vz/images/8006/vm-8006-disk-1.raw,if=none,id=drive-scsi1,aio=native,discard=on,format=raw,cache=none,detect-zeroes=unmap' \
+  -drive 'file=/var/lib/vz/images/8006/vm-8006-disk-1.raw,if=none,id=drive-scsi1,discard=on,format=raw,cache=none,aio=native,detect-zeroes=unmap' \
   -device 'scsi-hd,bus=scsihw0.0,scsi-id=1,drive=drive-scsi1,id=scsi1' \
-  -drive 'file=/var/lib/vz/images/8006/vm-8006-disk-2.raw,if=none,id=drive-scsi2,aio=io_uring,discard=on,format=raw,cache=none,detect-zeroes=unmap' \
+  -drive 'file=/var/lib/vz/images/8006/vm-8006-disk-2.raw,if=none,id=drive-scsi2,discard=on,format=raw,cache=none,aio=io_uring,detect-zeroes=unmap' \
   -device 'scsi-hd,bus=scsihw0.0,scsi-id=2,drive=drive-scsi2,id=scsi2' \
   -drive 'file=/var/lib/vz/images/8006/vm-8006-disk-3.raw,if=none,id=drive-scsi3,discard=on,format=raw,cache=none,aio=io_uring,detect-zeroes=unmap' \
   -device 'scsi-hd,bus=scsihw0.0,scsi-id=3,drive=drive-scsi3,id=scsi3' \
   -drive 'file=/mnt/pve/cifs-store/images/8006/vm-8006-disk-4.raw,if=none,id=drive-scsi4,discard=on,format=raw,cache=none,aio=native,detect-zeroes=unmap' \
   -device 'scsi-hd,bus=scsihw0.0,scsi-id=4,drive=drive-scsi4,id=scsi4' \
-  -drive 'file=/mnt/pve/cifs-store/images/8006/vm-8006-disk-5.raw,if=none,id=drive-scsi5,aio=io_uring,discard=on,format=raw,cache=none,detect-zeroes=unmap' \
+  -drive 'file=/mnt/pve/cifs-store/images/8006/vm-8006-disk-5.raw,if=none,id=drive-scsi5,discard=on,format=raw,cache=none,aio=io_uring,detect-zeroes=unmap' \
   -device 'scsi-hd,bus=scsihw0.0,scsi-id=5,drive=drive-scsi5,id=scsi5' \
   -drive 'file=/dev/rbd-pve/fc4181a6-56eb-4f68-b452-8ba1f381ca2a/cpool/vm-8006-disk-6,if=none,id=drive-scsi6,discard=on,format=raw,cache=none,aio=io_uring,detect-zeroes=unmap' \
   -device 'scsi-hd,bus=scsihw0.0,scsi-id=6,drive=drive-scsi6,id=scsi6' \
   -device 'lsi,id=scsihw1,bus=pci.0,addr=0x6' \
-  -drive 'file=/dev/rbd-pve/fc4181a6-56eb-4f68-b452-8ba1f381ca2a/cpool/vm-8006-disk-7,if=none,id=drive-scsi7,aio=io_uring,discard=on,format=raw,cache=none,detect-zeroes=unmap' \
+  -drive 'file=/dev/rbd-pve/fc4181a6-56eb-4f68-b452-8ba1f381ca2a/cpool/vm-8006-disk-7,if=none,id=drive-scsi7,discard=on,format=raw,cache=none,aio=io_uring,detect-zeroes=unmap' \
   -device 'scsi-hd,bus=scsihw1.0,scsi-id=0,drive=drive-scsi7,id=scsi7' \
   -drive 'file=/dev/rbd-pve/fc4181a6-56eb-4f68-b452-8ba1f381ca2a/cpool/vm-8006-disk-8,if=none,id=drive-scsi8,cache=writeback,discard=on,format=raw,aio=threads,detect-zeroes=unmap' \
   -device 'scsi-hd,bus=scsihw1.0,scsi-id=1,drive=drive-scsi8,id=scsi8' \
-  -drive 'file=/dev/rbd-pve/fc4181a6-56eb-4f68-b452-8ba1f381ca2a/cpool/vm-8006-disk-9,if=none,id=drive-scsi9,cache=writeback,aio=io_uring,discard=on,format=raw,detect-zeroes=unmap' \
+  -drive 'file=/dev/rbd-pve/fc4181a6-56eb-4f68-b452-8ba1f381ca2a/cpool/vm-8006-disk-9,if=none,id=drive-scsi9,cache=writeback,discard=on,format=raw,aio=io_uring,detect-zeroes=unmap' \
   -device 'scsi-hd,bus=scsihw1.0,scsi-id=2,drive=drive-scsi9,id=scsi9' \
   -drive 'file=rbd:cpool/vm-8006-disk-8:mon_host=127.0.0.42;127.0.0.21;[\:\:1]:auth_supported=none,if=none,id=drive-scsi10,discard=on,format=raw,cache=none,aio=io_uring,detect-zeroes=unmap' \
   -device 'scsi-hd,bus=scsihw1.0,scsi-id=3,drive=drive-scsi10,id=scsi10' \
-  -drive 'file=rbd:cpool/vm-8006-disk-8:mon_host=127.0.0.42;127.0.0.21;[\:\:1]:auth_supported=none,if=none,id=drive-scsi11,aio=io_uring,discard=on,format=raw,cache=none,detect-zeroes=unmap' \
+  -drive 'file=rbd:cpool/vm-8006-disk-8:mon_host=127.0.0.42;127.0.0.21;[\:\:1]:auth_supported=none,if=none,id=drive-scsi11,discard=on,format=raw,cache=none,aio=io_uring,detect-zeroes=unmap' \
   -device 'scsi-hd,bus=scsihw1.0,scsi-id=4,drive=drive-scsi11,id=scsi11' \
   -drive 'file=/dev/veegee/vm-8006-disk-9,if=none,id=drive-scsi12,discard=on,format=raw,cache=none,aio=native,detect-zeroes=unmap' \
   -device 'scsi-hd,bus=scsihw1.0,scsi-id=5,drive=drive-scsi12,id=scsi12' \
-  -drive 'file=/dev/veegee/vm-8006-disk-9,if=none,id=drive-scsi13,aio=io_uring,discard=on,format=raw,cache=none,detect-zeroes=unmap' \
+  -drive 'file=/dev/veegee/vm-8006-disk-9,if=none,id=drive-scsi13,discard=on,format=raw,cache=none,aio=io_uring,detect-zeroes=unmap' \
   -device 'scsi-hd,bus=scsihw1.0,scsi-id=6,drive=drive-scsi13,id=scsi13' \
   -machine 'type=pc+pve1'
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 10/22] drive: introduce detect_zeroes_cmdline_option() helper

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm       | 10 +---------
 PVE/QemuServer/Drive.pm | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index a2e51849..b9705367 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1492,15 +1492,7 @@ sub print_drive_commandline_full {
 	if !defined($drive->{media}) && defined($vtype) && $vtype eq 'iso';
 
     if (!drive_is_cdrom($drive)) {
-	my $detectzeroes;
-	if (defined($drive->{detect_zeroes}) && !$drive->{detect_zeroes}) {
-	    $detectzeroes = 'off';
-	} elsif ($drive->{discard}) {
-	    $detectzeroes = $drive->{discard} eq 'on' ? 'unmap' : 'on';
-	} else {
-	    # This used to be our default with discard not being specified:
-	    $detectzeroes = 'on';
-	}
+	my $detectzeroes = PVE::QemuServer::Drive::detect_zeroes_cmdline_option($drive);
 
 	# note: 'detect-zeroes' works per blockdev and we want it to persist
 	# after the alloc-track is removed, so put it on 'file' directly
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index f7611662..aaa0ef0b 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -1034,4 +1034,20 @@ sub aio_cmdline_option {
     }
 }
 
+# must not be called for CD-ROMs
+sub detect_zeroes_cmdline_option {
+    my ($drive) = @_;
+
+    die "cannot use detect-zeroes for CD-ROM\n" if drive_is_cdrom($drive);
+
+    if (defined($drive->{detect_zeroes}) && !$drive->{detect_zeroes}) {
+	return 'off';
+    } elsif ($drive->{discard}) {
+	return $drive->{discard} eq 'on' ? 'unmap' : 'on';
+    }
+
+    # This used to be our default with discard not being specified:
+    return 'on';
+}
+
 1;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 11/22] introduce StateFile module for state file related helpers

[Fiona Ebner]

The following commit will also move the state file handling which uses
the migration IP to the module.

Since QemuServer.pm does not use QemuMigrate.pm and since state file
handling can be both network migration or regular state file, it seems
best to add a new module. The goal is to make vm_start_nolock() more
readable.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm           | 44 ++++++++++---------------------------
 PVE/QemuServer/Makefile     |  1 +
 PVE/QemuServer/StateFile.pm | 32 +++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 32 deletions(-)
 create mode 100644 PVE/QemuServer/StateFile.pm

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index b9705367..91b55cf9 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -72,6 +72,7 @@ use PVE::QemuServer::Monitor qw(mon_cmd);
 use PVE::QemuServer::PCI qw(print_pci_addr print_pcie_addr print_pcie_root_port parse_hostpci);
 use PVE::QemuServer::QMPHelpers qw(qemu_deviceadd qemu_devicedel qemu_objectadd qemu_objectdel);
 use PVE::QemuServer::RNG qw(parse_rng print_rng_device_commandline print_rng_object_commandline);
+use PVE::QemuServer::StateFile;
 use PVE::QemuServer::USB;
 use PVE::QemuServer::Virtiofs qw(max_virtiofs start_all_virtiofsd);
 
@@ -5574,6 +5575,7 @@ sub vm_start_nolock {
 
     my $migratedfrom = $migrate_opts->{migratedfrom};
     my $migration_type = $migrate_opts->{type};
+    my $nbd_protocol_version = $migrate_opts->{nbd_proto_version} // 0;
 
     my $res = {};
 
@@ -5638,35 +5640,14 @@ sub vm_start_nolock {
     );
 
     my $migration_ip;
-    my $get_migration_ip = sub {
-	my ($nodename) = @_;
-
-	return $migration_ip if defined($migration_ip);
-
-	my $cidr = $migrate_opts->{network};
-
-	if (!defined($cidr)) {
-	    my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');
-	    $cidr = $dc_conf->{migration}->{network};
-	}
-
-	if (defined($cidr)) {
-	    my $ips = PVE::Network::get_local_ip_from_cidr($cidr);
-
-	    die "could not get IP: no address configured on local " .
-		"node for network '$cidr'\n" if scalar(@$ips) == 0;
-
-	    die "could not get IP: multiple addresses configured on local " .
-		"node for network '$cidr'\n" if scalar(@$ips) > 1;
-
-	    $migration_ip = @$ips[0];
-	}
-
-	$migration_ip = PVE::Cluster::remote_node_ip($nodename, 1)
-	    if !defined($migration_ip);
-
-	return $migration_ip;
-    };
+    if (
+	($statefile && $statefile eq 'tcp' && $migration_type eq 'insecure')
+	|| ($migrate_opts->{nbd} && ($nbd_protocol_version == 0 || $migration_type eq 'insecure'))
+    ) {
+	my $nodename = nodename();
+	$migration_ip = PVE::QemuServer::StateFile::get_migration_ip(
+	    $nodename, $migrate_opts->{network});
+    }
 
     if ($statefile) {
 	if ($statefile eq 'tcp') {
@@ -5684,7 +5665,7 @@ sub vm_start_nolock {
 	    }
 
 	    if ($migration_type eq 'insecure') {
-		$migrate->{addr} = $get_migration_ip->($nodename);
+		$migrate->{addr} = $migration_ip // die "internal error - no migration IP";
 		$migrate->{addr} = "[$migrate->{addr}]" if Net::IP::ip_is_ipv6($migrate->{addr});
 	    }
 
@@ -5896,7 +5877,6 @@ sub vm_start_nolock {
 
     #start nbd server for storage migration
     if (my $nbd = $migrate_opts->{nbd}) {
-	my $nbd_protocol_version = $migrate_opts->{nbd_proto_version} // 0;
 
 	my $migrate_storage_uri;
 	# nbd_protocol_version > 0 for unix socket support
@@ -5907,7 +5887,7 @@ sub vm_start_nolock {
 	    $res->{migrate}->{unix_sockets} = [$socket_path];
 	} else {
 	    my $nodename = nodename();
-	    my $localip = $get_migration_ip->($nodename);
+	    my $localip = $migration_ip // die "internal error - no migration IP";
 	    my $pfamily = PVE::Tools::get_host_address_family($nodename);
 	    my $storage_migrate_port = PVE::Tools::next_migrate_port($pfamily);
 
diff --git a/PVE/QemuServer/Makefile b/PVE/QemuServer/Makefile
index 8bcd484e..bbd5b5c0 100644
--- a/PVE/QemuServer/Makefile
+++ b/PVE/QemuServer/Makefile
@@ -13,6 +13,7 @@ SOURCES=PCI.pm		\
 	CGroup.pm	\
 	Drive.pm	\
 	QMPHelpers.pm	\
+	StateFile.pm	\
 	Virtiofs.pm
 
 .PHONY: install
diff --git a/PVE/QemuServer/StateFile.pm b/PVE/QemuServer/StateFile.pm
new file mode 100644
index 00000000..e297839b
--- /dev/null
+++ b/PVE/QemuServer/StateFile.pm
@@ -0,0 +1,32 @@
+package PVE::QemuServer::StateFile;
+
+use strict;
+use warnings;
+
+use PVE::Cluster;
+use PVE::Network;
+
+sub get_migration_ip {
+    my ($nodename, $cidr) = @_;
+
+    if (!defined($cidr)) {
+	my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');
+	$cidr = $dc_conf->{migration}->{network};
+    }
+
+    if (defined($cidr)) {
+	my $ips = PVE::Network::get_local_ip_from_cidr($cidr);
+
+	die "could not get IP: no address configured on local " .
+	    "node for network '$cidr'\n" if scalar(@$ips) == 0;
+
+	die "could not get IP: multiple addresses configured on local " .
+	    "node for network '$cidr'\n" if scalar(@$ips) > 1;
+
+	return $ips->[0];
+    }
+
+    return PVE::Cluster::remote_node_ip($nodename, 1);
+}
+
+1;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 12/22] vm start: move state file handling to dedicated module

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm           | 56 +++++++------------------------------
 PVE/QemuServer/StateFile.pm | 56 +++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 46 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 91b55cf9..25ba709d 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5577,8 +5577,6 @@ sub vm_start_nolock {
     my $migration_type = $migrate_opts->{type};
     my $nbd_protocol_version = $migrate_opts->{nbd_proto_version} // 0;
 
-    my $res = {};
-
     # clean up leftover reboot request files
     eval { clear_reboot_request($vmid); };
     warn $@ if $@;
@@ -5649,54 +5647,20 @@ sub vm_start_nolock {
 	    $nodename, $migrate_opts->{network});
     }
 
+    my $res = {};
+    my $statefile_is_a_volume;
+    my $state_cmdline = [];
     if ($statefile) {
-	if ($statefile eq 'tcp') {
-	    my $migrate = $res->{migrate} = { proto => 'tcp' };
-	    $migrate->{addr} = "localhost";
-	    my $datacenterconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
-	    my $nodename = nodename();
-
-	    if (!defined($migration_type)) {
-		if (defined($datacenterconf->{migration}->{type})) {
-		    $migration_type = $datacenterconf->{migration}->{type};
-		} else {
-		    $migration_type = 'secure';
-		}
-	    }
-
-	    if ($migration_type eq 'insecure') {
-		$migrate->{addr} = $migration_ip // die "internal error - no migration IP";
-		$migrate->{addr} = "[$migrate->{addr}]" if Net::IP::ip_is_ipv6($migrate->{addr});
-	    }
-
-	    # see #4501: port reservation should be done close to usage - tell QEMU where to listen
-	    # via QMP later
-	    push @$cmd, '-incoming', 'defer';
-	    push @$cmd, '-S';
-
-	} elsif ($statefile eq 'unix') {
-	    # should be default for secure migrations as a ssh TCP forward
-	    # tunnel is not deterministic reliable ready and fails regurarly
-	    # to set up in time, so use UNIX socket forwards
-	    my $migrate = $res->{migrate} = { proto => 'unix' };
-	    $migrate->{addr} = "/run/qemu-server/$vmid.migrate";
-	    unlink $migrate->{addr};
-
-	    $migrate->{uri} = "unix:$migrate->{addr}";
-	    push @$cmd, '-incoming', $migrate->{uri};
-	    push @$cmd, '-S';
-
-	} elsif (-e $statefile) {
-	    push @$cmd, '-loadstate', $statefile;
-	} else {
-	    my $statepath = PVE::Storage::path($storecfg, $statefile);
-	    push @$vollist, $statefile;
-	    push @$cmd, '-loadstate', $statepath;
-	}
+	($state_cmdline, $res->{migrate}, $statefile_is_a_volume) =
+	    PVE::QemuServer::StateFile::statefile_cmdline_option(
+		$storecfg, $vmid, $statefile, $migrate_opts, $migration_ip);
+	push @$vollist, $statefile if $statefile_is_a_volume;
     } elsif ($params->{paused}) {
-	push @$cmd, '-S';
+	$state_cmdline = ['-S'];
     }
 
+    push $cmd->@*, $state_cmdline->@*;
+
     my $memory = get_current_memory($conf->{memory});
     my $start_timeout = $params->{timeout} // config_aware_timeout($conf, $memory, $resume);
 
diff --git a/PVE/QemuServer/StateFile.pm b/PVE/QemuServer/StateFile.pm
index e297839b..630ccca3 100644
--- a/PVE/QemuServer/StateFile.pm
+++ b/PVE/QemuServer/StateFile.pm
@@ -29,4 +29,60 @@ sub get_migration_ip {
     return PVE::Cluster::remote_node_ip($nodename, 1);
 }
 
+# $migration_ip must be defined if using insecure TCP migration
+sub statefile_cmdline_option {
+    my ($storecfg, $vmid, $statefile, $migrate_opts, $migration_ip) = @_;
+
+    my $migration_type = $migrate_opts->{type};
+
+    my $statefile_is_a_volume = 0;
+    my $res = {};
+    my $cmd = [];
+
+    if ($statefile eq 'tcp') {
+	my $migrate = $res->{migrate} = { proto => 'tcp' };
+	$migrate->{addr} = "localhost";
+	my $datacenterconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
+
+	if (!defined($migration_type)) {
+	    if (defined($datacenterconf->{migration}->{type})) {
+		$migration_type = $datacenterconf->{migration}->{type};
+	    } else {
+		$migration_type = 'secure';
+	    }
+	}
+
+	if ($migration_type eq 'insecure') {
+	    $migrate->{addr} = $migration_ip // die "internal error - no migration IP";
+	    $migrate->{addr} = "[$migrate->{addr}]" if Net::IP::ip_is_ipv6($migrate->{addr});
+	}
+
+	# see #4501: port reservation should be done close to usage - tell QEMU where to listen
+	# via QMP later
+	push @$cmd, '-incoming', 'defer';
+	push @$cmd, '-S';
+
+    } elsif ($statefile eq 'unix') {
+	# should be default for secure migrations as a ssh TCP forward
+	# tunnel is not deterministic reliable ready and fails regurarly
+	# to set up in time, so use UNIX socket forwards
+	my $migrate = $res->{migrate} = { proto => 'unix' };
+	$migrate->{addr} = "/run/qemu-server/$vmid.migrate";
+	unlink $migrate->{addr};
+
+	$migrate->{uri} = "unix:$migrate->{addr}";
+	push @$cmd, '-incoming', $migrate->{uri};
+	push @$cmd, '-S';
+
+    } elsif (-e $statefile) {
+	push @$cmd, '-loadstate', $statefile;
+    } else {
+	my $statepath = PVE::Storage::path($storecfg, $statefile);
+	$statefile_is_a_volume = 1;
+	push @$cmd, '-loadstate', $statepath;
+    }
+
+    return ($cmd, $res->{migrate}, $statefile_is_a_volume);
+}
+
 1;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 13/22] vm start: move config_to_command() call further down

[Fiona Ebner]

For command line generation of '-blockdev', it will be necessary to
activate the VM's volumes before calling config_to_command() and thus
also deactivate in an error scenario. Avoid the need to put more code
than necessary into the resulting eval.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 25ba709d..dfd4351b 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5625,18 +5625,6 @@ sub vm_start_nolock {
 	print "Resuming suspended VM\n";
     }
 
-    # Note that for certain cases like templates, the configuration is minimized, so need to ensure
-    # the rest of the function here uses the same configuration that was used to build the command
-    (my $cmd, my $vollist, my $spice_port, my $pci_devices, $conf) = config_to_command(
-	$storecfg,
-	$vmid,
-	$conf,
-	$defaults,
-	$forcemachine,
-	$forcecpu,
-	$params->{'live-restore-backing'},
-    );
-
     my $migration_ip;
     if (
 	($statefile && $statefile eq 'tcp' && $migration_type eq 'insecure')
@@ -5654,16 +5642,28 @@ sub vm_start_nolock {
 	($state_cmdline, $res->{migrate}, $statefile_is_a_volume) =
 	    PVE::QemuServer::StateFile::statefile_cmdline_option(
 		$storecfg, $vmid, $statefile, $migrate_opts, $migration_ip);
-	push @$vollist, $statefile if $statefile_is_a_volume;
     } elsif ($params->{paused}) {
 	$state_cmdline = ['-S'];
     }
 
-    push $cmd->@*, $state_cmdline->@*;
-
     my $memory = get_current_memory($conf->{memory});
     my $start_timeout = $params->{timeout} // config_aware_timeout($conf, $memory, $resume);
 
+    # Note that for certain cases like templates, the configuration is minimized, so need to ensure
+    # the rest of the function here uses the same configuration that was used to build the command
+    (my $cmd, my $vollist, my $spice_port, my $pci_devices, $conf) = config_to_command(
+	$storecfg,
+	$vmid,
+	$conf,
+	$defaults,
+	$forcemachine,
+	$forcecpu,
+	$params->{'live-restore-backing'},
+    );
+
+    push $cmd->@*, $state_cmdline->@*;
+    push @$vollist, $statefile if $statefile_is_a_volume;
+
     my $pci_reserve_list = [];
     for my $device (values $pci_devices->%*) {
 	next if $device->{mdev}; # we don't reserve for mdev devices
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 14/22] vm start/commandline: also clean up pci reservation when config_to_command() fails

[Fiona Ebner]

The config_to_command() function already calls into
print_hostpci_devices() which makes reservations.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm | 56 +++++++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 24 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index dfd4351b..934adf60 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5649,31 +5649,32 @@ sub vm_start_nolock {
     my $memory = get_current_memory($conf->{memory});
     my $start_timeout = $params->{timeout} // config_aware_timeout($conf, $memory, $resume);
 
-    # Note that for certain cases like templates, the configuration is minimized, so need to ensure
-    # the rest of the function here uses the same configuration that was used to build the command
-    (my $cmd, my $vollist, my $spice_port, my $pci_devices, $conf) = config_to_command(
-	$storecfg,
-	$vmid,
-	$conf,
-	$defaults,
-	$forcemachine,
-	$forcecpu,
-	$params->{'live-restore-backing'},
-    );
-
-    push $cmd->@*, $state_cmdline->@*;
-    push @$vollist, $statefile if $statefile_is_a_volume;
-
+    my ($cmd, $vollist, $spice_port);
     my $pci_reserve_list = [];
-    for my $device (values $pci_devices->%*) {
-	next if $device->{mdev}; # we don't reserve for mdev devices
-	push $pci_reserve_list->@*, map { $_->{id} } $device->{ids}->@*;
-    }
-
-    # reserve all PCI IDs before actually doing anything with them
-    PVE::QemuServer::PCI::reserve_pci_usage($pci_reserve_list, $vmid, $start_timeout);
-
     eval {
+	# Note that for certain cases like templates, the configuration is minimized, so need to ensure
+	# the rest of the function here uses the same configuration that was used to build the command
+	($cmd, $vollist, $spice_port, my $pci_devices, $conf) = config_to_command(
+	    $storecfg,
+	    $vmid,
+	    $conf,
+	    $defaults,
+	    $forcemachine,
+	    $forcecpu,
+	    $params->{'live-restore-backing'},
+	);
+
+	push $cmd->@*, $state_cmdline->@*;
+	push @$vollist, $statefile if $statefile_is_a_volume;
+
+	for my $device (values $pci_devices->%*) {
+	    next if $device->{mdev}; # we don't reserve for mdev devices
+	    push $pci_reserve_list->@*, map { $_->{id} } $device->{ids}->@*;
+	}
+
+	# reserve all PCI IDs before actually doing anything with them
+	PVE::QemuServer::PCI::reserve_pci_usage($pci_reserve_list, $vmid, $start_timeout);
+
 	my $uuid;
 	for my $id (sort keys %$pci_devices) {
 	    my $d = $pci_devices->{$id};
@@ -5981,13 +5982,20 @@ sub vm_commandline {
 
     my $defaults = load_defaults();
 
-    my $cmd = config_to_command($storecfg, $vmid, $conf, $defaults, $forcemachine, $forcecpu);
+    my $cmd;
+    eval {
+	$cmd = config_to_command($storecfg, $vmid, $conf, $defaults, $forcemachine, $forcecpu);
+    };
+    my $err = $@;
+
     # if the vm is not running, we need to clean up the reserved/created devices
     if (!PVE::QemuServer::Helpers::vm_running_locally($vmid)) {
 	eval { cleanup_pci_devices($vmid, $conf) };
 	warn $@ if $@;
     }
 
+    die $err if $err;
+
     return PVE::Tools::cmd2string($cmd);
 }
 
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 15/22] vm start/commandline: activate volumes before config_to_command()

[Fiona Ebner]

With '-blockdev', it is necessary to activate the volumes to generate
the command line, because it can be necessary to check whether the
volume is a block device or a regular file.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm                | 61 +++++++++++++++++++++++---------
 test/run_config2command_tests.pl | 10 ++++++
 2 files changed, 55 insertions(+), 16 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 934adf60..82304096 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3844,7 +3844,6 @@ sub config_to_command {
 	push @$devices, '-watchdog-action', $wdopts->{action} if $wdopts->{action};
     }
 
-    my $vollist = [];
     my $scsicontroller = {};
     my $ahcicontroller = {};
     my $scsihw = defined($conf->{scsihw}) ? $conf->{scsihw} : $defaults->{scsihw};
@@ -3857,11 +3856,6 @@ sub config_to_command {
     PVE::QemuConfig->foreach_volume($conf, sub {
 	my ($ds, $drive) = @_;
 
-	if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
-	    check_volume_storage_type($storecfg, $drive->{file});
-	    push @$vollist, $drive->{file};
-	}
-
 	# ignore efidisk here, already added in bios/fw handling code above
 	return if $drive->{interface} eq 'efidisk';
 	# similar for TPM
@@ -4035,7 +4029,6 @@ sub config_to_command {
 
     if (my $vmstate = $conf->{vmstate}) {
 	my $statepath = PVE::Storage::path($storecfg, $vmstate);
-	push @$vollist, $vmstate;
 	push @$cmd, '-loadstate', $statepath;
 	print "activating and using '$vmstate' as vmstate\n";
     }
@@ -4051,7 +4044,7 @@ sub config_to_command {
 	push @$cmd, @$aa;
     }
 
-    return wantarray ? ($cmd, $vollist, $spice_port, $pci_devices, $conf) : $cmd;
+    return wantarray ? ($cmd, $spice_port, $pci_devices, $conf) : $cmd;
 }
 
 sub spice_port {
@@ -5649,12 +5642,18 @@ sub vm_start_nolock {
     my $memory = get_current_memory($conf->{memory});
     my $start_timeout = $params->{timeout} // config_aware_timeout($conf, $memory, $resume);
 
-    my ($cmd, $vollist, $spice_port);
+    my $vollist = get_current_vm_volumes($storecfg, $conf);
+    push $vollist->@*, $statefile if $statefile_is_a_volume;
+
+    my ($cmd, $spice_port);
     my $pci_reserve_list = [];
     eval {
+	# With -blockdev, it is necessary to activate the volumes before generating the command line
+	PVE::Storage::activate_volumes($storecfg, $vollist);
+
 	# Note that for certain cases like templates, the configuration is minimized, so need to ensure
 	# the rest of the function here uses the same configuration that was used to build the command
-	($cmd, $vollist, $spice_port, my $pci_devices, $conf) = config_to_command(
+	($cmd, $spice_port, my $pci_devices, $conf) = config_to_command(
 	    $storecfg,
 	    $vmid,
 	    $conf,
@@ -5665,7 +5664,6 @@ sub vm_start_nolock {
 	);
 
 	push $cmd->@*, $state_cmdline->@*;
-	push @$vollist, $statefile if $statefile_is_a_volume;
 
 	for my $device (values $pci_devices->%*) {
 	    next if $device->{mdev}; # we don't reserve for mdev devices
@@ -5707,14 +5705,13 @@ sub vm_start_nolock {
 	push @$cmd, '-uuid', $uuid if defined($uuid);
     };
     if (my $err = $@) {
+	eval { PVE::Storage::deactivate_volumes($storecfg, $vollist); };
+	warn $@ if $@;
 	eval { cleanup_pci_devices($vmid, $conf) };
 	warn $@ if $@;
 	die $err;
     }
 
-    PVE::Storage::activate_volumes($storecfg, $vollist);
-
-
     my %silence_std_outs = (outfunc => sub {}, errfunc => sub {});
     eval { run_command(['/bin/systemctl', 'reset-failed', "$vmid.scope"], %silence_std_outs) };
     eval { run_command(['/bin/systemctl', 'stop', "$vmid.scope"], %silence_std_outs) };
@@ -5982,14 +5979,25 @@ sub vm_commandline {
 
     my $defaults = load_defaults();
 
+    my $running = PVE::QemuServer::Helpers::vm_running_locally($vmid);
+    my $volumes = [];
+
+    # With -blockdev, it is necessary to activate the volumes before generating the command line
+    if (!$running) {
+	$volumes = get_current_vm_volumes($storecfg, $conf);
+	PVE::Storage::activate_volumes($storecfg, $volumes);
+    }
+
     my $cmd;
     eval {
 	$cmd = config_to_command($storecfg, $vmid, $conf, $defaults, $forcemachine, $forcecpu);
     };
     my $err = $@;
 
-    # if the vm is not running, we need to clean up the reserved/created devices
-    if (!PVE::QemuServer::Helpers::vm_running_locally($vmid)) {
+    # if the vm is not running, need to clean up the reserved/created devices and activated volumes
+    if (!$running) {
+	eval { PVE::Storage::deactivate_volumes($storecfg, $volumes); };
+	warn $@ if $@;
 	eval { cleanup_pci_devices($vmid, $conf) };
 	warn $@ if $@;
     }
@@ -6030,6 +6038,27 @@ sub get_vm_volumes {
     return $vollist;
 }
 
+# Get volumes defined in the current VM configuration, including the VM state file.
+sub get_current_vm_volumes {
+    my ($storecfg, $conf) = @_;
+
+    my $volumes = [];
+
+    PVE::QemuConfig->foreach_volume($conf, sub {
+	my ($ds, $drive) = @_;
+
+	if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
+	    check_volume_storage_type($storecfg, $drive->{file});
+	    push $volumes->@*, $drive->{file};
+	}
+    });
+    if (my $vmstate = $conf->{vmstate}) {
+	push $volumes->@*, $vmstate;
+    }
+
+    return $volumes;
+}
+
 sub cleanup_pci_devices {
     my ($vmid, $conf) = @_;
 
diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
index 7c581ad2..5217a020 100755
--- a/test/run_config2command_tests.pl
+++ b/test/run_config2command_tests.pl
@@ -255,6 +255,16 @@ $qemu_server_module->mock(
     },
 );
 
+my $storage_module = Test::MockModule->new("PVE::Storage");
+$storage_module->mock(
+    activate_volumes => sub {
+	return;
+    },
+    deactivate_volumes => sub {
+	return;
+    },
+);
+
 my $zfsplugin_module = Test::MockModule->new("PVE::Storage::ZFSPlugin");
 $zfsplugin_module->mock(
     zfs_get_lu_name => sub {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 16/22] print drive device: explicitly set write-cache starting with machine version 10.0

[Fiona Ebner]

With the 'blockdev' command line option, the cache options are split
up. While cache.direct and cache.no-flush can be set in the -blockdev
options, cache.writeback is a front-end property and was intentionally
removed from the 'blockdev' options by QEMU commit aaa436f998 ("block:
Remove cache.writeback from blockdev-add"). It needs to be configured
as the 'write-cache' property for the ide-hd/scsi-hd/virtio-blk
device.

Table from 'man kvm':

┌─────────────┬─────────────────┬──────────────┬────────────────┐
│             │ cache.writeback │ cache.direct │ cache.no-flush │
├─────────────┼─────────────────┼──────────────┼────────────────┤
│writeback    │ on              │ off          │ off            │
├─────────────┼─────────────────┼──────────────┼────────────────┤
│none         │ on              │ on           │ off            │
├─────────────┼─────────────────┼──────────────┼────────────────┤
│writethrough │ off             │ off          │ off            │
├─────────────┼─────────────────┼──────────────┼────────────────┤
│directsync   │ off             │ on           │ off            │
├─────────────┼─────────────────┼──────────────┼────────────────┤
│unsafe       │ on              │ off          │ on             │
└─────────────┴─────────────────┴──────────────┴────────────────┘

Suggested-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 82304096..f9e1b3f9 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1317,6 +1317,8 @@ sub print_drivedevice_full {
     my $device = '';
     my $maxdev = 0;
 
+    my $machine_version = extract_version($machine_type, kvm_user_version());
+
     my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
     if ($drive->{interface} eq 'virtio') {
 	my $pciaddr = print_pci_addr("$drive_id", $bridges, $arch, $machine_type);
@@ -1327,7 +1329,6 @@ sub print_drivedevice_full {
 	my ($maxdev, $controller, $controller_prefix) = scsihw_infos($conf, $drive);
 	my $unit = $drive->{index} % $maxdev;
 
-	my $machine_version = extract_version($machine_type, kvm_user_version());
 	my $device_type = PVE::QemuServer::Drive::get_scsi_device_type(
 	    $drive, $storecfg, $machine_version);
 
@@ -1403,6 +1404,15 @@ sub print_drivedevice_full {
 	$device .= ",serial=$serial";
     }
 
+    if (min_version($machine_version, 10, 0)) {
+	if (!drive_is_cdrom($drive)) {
+	    my $write_cache = 'on';
+	    if (my $cache = $drive->{cache}) {
+		$write_cache = 'off' if $cache eq 'writethrough' || $cache eq 'directsync';
+	    }
+	    $device .= ",write-cache=$write_cache";
+	}
+    }
 
     return $device;
 }
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 17/22] print drive device: set {r, w}error front-end properties starting with machine version 10.0

[Fiona Ebner]

These properties cannot be specified via '-blockdev' like they could
with '-drive', because they are properties of the front-end drive
device.

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

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index f9e1b3f9..90d07cee 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1412,6 +1412,9 @@ sub print_drivedevice_full {
 	    }
 	    $device .= ",write-cache=$write_cache";
 	}
+	for my $o (qw(rerror werror)) {
+	    $device .= ",$o=$drive->{$o}" if defined($drive->{$o});
+	}
     }
 
     return $device;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 18/22] print drive device: don't reference any drive for 'none' starting with machine version 10.0

[Fiona Ebner]

There will be no block node for 'none' after switching to '-blockdev'.

Co-developed-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
[FE: split out from larger patch
     do it also for non-SCSI cases]
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 90d07cee..e5f8a607 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1322,7 +1322,13 @@ sub print_drivedevice_full {
     my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
     if ($drive->{interface} eq 'virtio') {
 	my $pciaddr = print_pci_addr("$drive_id", $bridges, $arch, $machine_type);
-	$device = "virtio-blk-pci,drive=drive-$drive_id,id=${drive_id}${pciaddr}";
+	$device = 'virtio-blk-pci';
+	if (min_version($machine_version, 10, 0)) { # there is no blockdev for 'none'
+	    $device .= ",drive=drive-$drive_id" if $drive->{file} ne 'none';
+	    $device .= ",id=${drive_id}${pciaddr}";
+	} else {
+	    $device .= ",drive=drive-$drive_id,id=${drive_id}${pciaddr}";
+	}
 	$device .= ",iothread=iothread-$drive_id" if $drive->{iothread};
     } elsif ($drive->{interface} eq 'scsi') {
 
@@ -1338,7 +1344,12 @@ sub print_drivedevice_full {
 	    $device = "scsi-$device_type,bus=$controller_prefix$controller.0,channel=0,scsi-id=0"
 	        .",lun=$drive->{index}";
 	}
-	$device .= ",drive=drive-$drive_id,id=$drive_id";
+	if (min_version($machine_version, 10, 0)) { # there is no blockdev for 'none'
+	    $device .= ",drive=drive-$drive_id" if $drive->{file} ne 'none';
+	    $device .= ",id=$drive_id";
+	} else {
+	    $device .= ",drive=drive-$drive_id,id=$drive_id";
+	}
 
 	if ($drive->{ssd} && ($device_type eq 'block' || $device_type eq 'hd')) {
 	    $device .= ",rotation_rate=1";
@@ -1378,7 +1389,12 @@ sub print_drivedevice_full {
 	} else {
 	    $device .= ",bus=ahci$controller.$unit";
 	}
-	$device .= ",drive=drive-$drive_id,id=$drive_id";
+	if (min_version($machine_version, 10, 0)) { # there is no blockdev for 'none'
+	    $device .= ",drive=drive-$drive_id" if $drive->{file} ne 'none';
+	    $device .= ",id=$drive_id";
+	} else {
+	    $device .= ",drive=drive-$drive_id,id=$drive_id";
+	}
 
 	if ($device_type eq 'hd') {
 	    if (my $model = $drive->{model}) {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 19/22] drive: create a throttle group for each drive starting with machine version 10.0

[Fiona Ebner]

The throttle group will be referenced via the 'blockdev' schema.

Co-developed-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm          | 51 ++++++++++++++++++++++++++++++++++++++
 PVE/QemuServer/Blockdev.pm | 44 ++++++++++++++++++++++++++++++++
 PVE/QemuServer/Makefile    |  1 +
 3 files changed, 96 insertions(+)
 create mode 100644 PVE/QemuServer/Blockdev.pm

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index e5f8a607..bfadba98 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -51,6 +51,7 @@ use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_for
 use PVE::QMPClient;
 use PVE::QemuConfig;
 use PVE::QemuConfig::NoWrite;
+use PVE::QemuServer::Blockdev;
 use PVE::QemuServer::Helpers qw(config_aware_timeout min_version kvm_user_version windows_version);
 use PVE::QemuServer::Cloudinit;
 use PVE::QemuServer::CGroup;
@@ -3943,6 +3944,12 @@ sub config_to_command {
 	    push @$devices, '-blockdev', $live_restore->{blockdev};
 	}
 
+	if (min_version($machine_version, 10, 0)) {
+	    my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
+	    my $throttle_group = PVE::QemuServer::Blockdev::generate_throttle_group($drive);
+	    push @$cmd, '-object', to_json($throttle_group, { canonical => 1 });
+	}
+
 	my $drive_cmd = print_drive_commandline_full($storecfg, $vmid, $drive, $live_blockdev_name);
 
 	# extra protection for templates, but SATA and IDE don't support it..
@@ -4315,6 +4322,14 @@ sub qemu_iothread_del {
 sub qemu_driveadd {
     my ($storecfg, $vmid, $device) = @_;
 
+    my $machine_type = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
+
+    if (PVE::QemuServer::Machine::is_machine_version_at_least($machine_type, 10, 0)) {
+	my $drive_id = PVE::QemuServer::Drive::get_drive_id($device);
+	my $throttle_group = PVE::QemuServer::Blockdev::generate_throttle_group($device);
+	mon_cmd($vmid, 'object-add', %$throttle_group);
+    }
+
     my $drive = print_drive_commandline_full($storecfg, $vmid, $device, undef);
     $drive =~ s/\\/\\\\/g;
     my $ret = PVE::QemuServer::Monitor::hmp_cmd($vmid, "drive_add auto \"$drive\"", 60);
@@ -4328,6 +4343,12 @@ sub qemu_driveadd {
 sub qemu_drivedel {
     my ($vmid, $deviceid) = @_;
 
+    my $machine_type = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
+
+    if (PVE::QemuServer::Machine::is_machine_version_at_least($machine_type, 10, 0)) {
+	mon_cmd($vmid, 'object-del', id => 'throttle-drive-$deviceid');
+    }
+
     my $ret = PVE::QemuServer::Monitor::hmp_cmd($vmid, "drive_del drive-$deviceid", 10 * 60);
     $ret =~ s/^\s+//;
 
@@ -4571,6 +4592,36 @@ sub qemu_block_set_io_throttle {
 
     return if !check_running($vmid) ;
 
+    my $machine_type = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
+    if (PVE::QemuServer::Machine::is_machine_version_at_least($machine_type, 10, 0)) {
+	mon_cmd(
+	    $vmid,
+	    'qom-set',
+	    path => "throttle-$deviceid",
+	    property => "limits",
+	    value => {
+		'bps-total' => int($bps),
+		'bps-read' => int($bps_rd),
+		'bps-write' => int($bps_wr),
+		'iops-total' => int($iops),
+		'iops-read' => int($iops_rd),
+		'iops-write' => int($iops_wr),
+		'bps-total-max' => int($bps_max),
+		'bps-read-max' => int($bps_rd_max),
+		'bps-write-max' => int($bps_wr_max),
+		'iops-total-max' => int($iops_max),
+		'iops-read-max' => int($iops_rd_max),
+		'iops-write-max' => int($iops_wr_max),
+		'bps-total-max-length' => int($bps_max_length),
+		'bps-read-max-length' => int($bps_rd_max_length),
+		'bps-write-max-length' => int($bps_wr_max_length),
+		'iops-total-max-length' => int($iops_max_length),
+		'iops-read-max-length' => int($iops_rd_max_length),
+		'iops-write-max-length' => int($iops_wr_max_length),
+	    }
+	);
+    }
+
     mon_cmd($vmid, "block_set_io_throttle", device => $deviceid,
 	bps => int($bps),
 	bps_rd => int($bps_rd),
diff --git a/PVE/QemuServer/Blockdev.pm b/PVE/QemuServer/Blockdev.pm
new file mode 100644
index 00000000..b1150141
--- /dev/null
+++ b/PVE/QemuServer/Blockdev.pm
@@ -0,0 +1,44 @@
+package PVE::QemuServer::Blockdev;
+
+use strict;
+use warnings;
+
+use PVE::QemuServer::Drive;
+
+sub generate_throttle_group {
+    my ($drive) = @_;
+
+    my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
+
+    my $limits = {};
+
+    for 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);
+	}
+    }
+
+    return {
+	id => "throttle-drive-$drive_id",
+	limits => $limits,
+	'qom-type' => 'throttle-group',
+    };
+}
+
+1;
diff --git a/PVE/QemuServer/Makefile b/PVE/QemuServer/Makefile
index bbd5b5c0..8054a93b 100644
--- a/PVE/QemuServer/Makefile
+++ b/PVE/QemuServer/Makefile
@@ -1,4 +1,5 @@
 SOURCES=PCI.pm		\
+	Blockdev.pm	\
 	RNG.pm		\
 	USB.pm		\
 	Memory.pm	\
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 20/22] blockdev: add helpers to generate blockdev commandline

[Fiona Ebner]

The drive device and node structure is:
front-end device {ide-hd,scsi-hd,virtio-blk-pci} (id=$drive_id)
 - throttle node (node-name=$drive_id)
  - format node  (node-name=f$encoded-info)
   - file node   (node-name=e$encoded-info)

The node-name can only be 31 characters long and needs to start with a
letter. The throttle node will stay inserted below the front-end
device. The other nodes might change however, because of drive
mirroring and similar. There currently are no good helpers to
query/walk the block graph via QMP, x-debug-query-block-graph is
experimental and for debugging only. Therefore, necessary information
is encoded in the node name to be able to find it again. In
particular, this is the volume ID, the drive ID and optionally a
snapshot name. As long as the configuration file matches with the
running instance, this is enough to find the correct node for
block operations like mirror and resize.

The 'snapshot' option, for QEMU's snapshot mode, i.e. writes are only
temporary, is not yet supported.

Originally-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
[FE: split up patch
     expand commit message
     explicitly test for drivers with aio setting
     improve readonly handling
     improve CD-ROM handling
     fix failure for storage named 'nbd' by always using full regex
     improve node name generation
     fail when drive->{snapshot} is set]
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

If we do not get around to implement the 'snapshot' option in time for
PVE 9, we can still fall back to the legacy drive for that (and warn
users that not all operations might work with such drives).

 PVE/QemuServer/Blockdev.pm | 172 ++++++++++++++++++++++++++++++++++++-
 PVE/QemuServer/Makefile    |   1 +
 2 files changed, 172 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer/Blockdev.pm b/PVE/QemuServer/Blockdev.pm
index b1150141..2d3760f0 100644
--- a/PVE/QemuServer/Blockdev.pm
+++ b/PVE/QemuServer/Blockdev.pm
@@ -3,7 +3,42 @@ package PVE::QemuServer::Blockdev;
 use strict;
 use warnings;
 
-use PVE::QemuServer::Drive;
+use Digest::SHA;
+use Fcntl qw(S_ISBLK S_ISCHR);
+use File::stat;
+use JSON;
+
+use PVE::Storage;
+
+use PVE::QemuServer::Drive qw(drive_is_cdrom);
+
+my sub get_node_name {
+    my ($type, $drive_id, $volid, $snap) = @_;
+
+    my $info = "drive=$drive_id,";
+    $info .= "snap=$snap," if defined($snap);
+    $info .= "volid=$volid";
+
+    my $hash = substr(Digest::SHA::sha256_hex($info), 0, 30);
+
+    my $prefix = "";
+    if ($type eq 'fmt') {
+	$prefix = 'f';
+    } elsif ($type eq 'file') {
+	$prefix = 'e';
+    } else {
+	die "unknown node type '$type'";
+    }
+    # node-name must start with an alphabetical character
+    return "${prefix}${hash}";
+}
+
+my sub read_only_json_option {
+    my ($drive, $options) = @_;
+
+    return JSON::true if $drive->{ro} || drive_is_cdrom($drive) || $options->{'read-only'};
+    return JSON::false;
+}
 
 sub generate_throttle_group {
     my ($drive) = @_;
@@ -41,4 +76,139 @@ sub generate_throttle_group {
     };
 }
 
+sub generate_blockdev_drive_cache {
+    my ($drive, $scfg) = @_;
+
+    my $cache_direct = PVE::QemuServer::Drive::drive_uses_cache_direct($drive, $scfg);
+    my $cache = {};
+    $cache->{direct} = $cache_direct ? JSON::true : JSON::false;
+    $cache->{'no-flush'} = $drive->{cache} && $drive->{cache} eq 'unsafe' ? JSON::true : JSON::false;
+    return $cache;
+}
+
+sub generate_file_blockdev {
+    my ($storecfg, $drive, $options) = @_;
+
+    my $blockdev = {};
+    my $scfg = undef;
+
+    die "generate_file_blockdev called without volid/path\n" if !$drive->{file};
+    die "generate_file_blockdev called with 'none'\n" if $drive->{file} eq 'none';
+    # FIXME use overlay and new config option to define storage for temp write device
+    die "'snapshot' option is not yet supported for '-blockdev'\n" if $drive->{snapshot};
+
+    my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
+
+    if ($drive->{file} eq 'cdrom') {
+	my $path = PVE::QemuServer::Drive::get_iso_path($storecfg, $drive->{file});
+	$blockdev = { driver => 'host_cdrom', filename => $path };
+    } elsif ($drive->{file} =~ m|^/|) {
+	my $path = $drive->{file};
+	# The 'file' driver only works for regular files. The check below is taken from
+	# block/file-posix.c:hdev_probe_device() in QEMU. To detect CD-ROM host devices, QEMU issues
+	# an ioctl, while the code here relies on the media=cdrom flag instead.
+	my $st = File::stat::stat($path) or die "stat for '$path' failed - $!\n";
+	my $driver = 'file';
+	if (S_ISCHR($st->mode) || S_ISBLK($st->mode)) {
+	    $driver = drive_is_cdrom($drive) ? 'host_cdrom' : 'host_device';
+	}
+	$blockdev = { driver => $driver, filename => $path };
+    } else {
+	my $volid = $drive->{file};
+	my ($storeid) = PVE::Storage::parse_volume_id($volid);
+
+	my $vtype = (PVE::Storage::parse_volname($storecfg, $drive->{file}))[0];
+	die "$drive_id: explicit media parameter is required for iso images\n"
+	    if !defined($drive->{media}) && defined($vtype) && $vtype eq 'iso';
+
+	my $storage_opts = { hints => {} };
+	$storage_opts->{hints}->{'efi-disk'} = 1 if $drive->{interface} eq 'efidisk';
+	$storage_opts->{'snapshot-name'} = $options->{'snapshot-name'}
+	    if defined($options->{'snapshot-name'});
+	$blockdev = PVE::Storage::qemu_blockdev_options($storecfg, $volid, $storage_opts);
+	$scfg = PVE::Storage::storage_config($storecfg, $storeid);
+    }
+
+    $blockdev->{cache} = generate_blockdev_drive_cache($drive, $scfg);
+
+    my $driver = $blockdev->{driver};
+    # only certain drivers have the aio setting
+    if ($driver eq 'file' || $driver eq 'host_cdrom' || $driver eq 'host_device') {
+	$blockdev->{aio} = PVE::QemuServer::Drive::aio_cmdline_option(
+	    $scfg, $drive, $blockdev->{cache}->{direct});
+    }
+
+    if (!drive_is_cdrom($drive)) {
+	$blockdev->{discard} = $drive->{discard} && $drive->{discard} eq 'on' ? 'unmap' : 'ignore';
+	$blockdev->{'detect-zeroes'} = PVE::QemuServer::Drive::detect_zeroes_cmdline_option($drive);
+    }
+
+    $blockdev->{'node-name'} = get_node_name(
+	'file', $drive_id, $drive->{file}, $options->{'snapshot-name'});
+
+    $blockdev->{'read-only'} = read_only_json_option($drive, $options);
+
+    return $blockdev;
+}
+
+sub generate_format_blockdev {
+    my ($storecfg, $drive, $child, $options) = @_;
+
+    die "generate_format_blockdev called without volid/path\n" if !$drive->{file};
+    die "generate_format_blockdev called with 'none'\n" if $drive->{file} eq 'none';
+
+    my $scfg;
+    my $format;
+    my $volid = $drive->{file};
+    my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
+    my ($storeid) = PVE::Storage::parse_volume_id($volid, 1);
+
+    # For PVE-managed volumes, use the format from the storage layer and prevent overrides via the
+    # drive's 'format' option. For unmanaged volumes, fallback to 'raw' to avoid auto-detection by
+    # QEMU.
+    if ($storeid) {
+	$scfg = PVE::Storage::storage_config($storecfg, $storeid);
+	$format = PVE::QemuServer::Drive::checked_volume_format($storecfg, $volid);
+	if ($drive->{format} && $drive->{format} ne $format) {
+	    die "drive '$drive->{interface}$drive->{index}' - volume '$volid'"
+		." - 'format=$drive->{format}' option different from storage format '$format'\n";
+	}
+    } else {
+	$format = $drive->{format} // 'raw';
+    }
+
+    # define cache option on both format && file node like libvirt does
+    my $cache = generate_blockdev_drive_cache($drive, $scfg);
+
+    my $node_name = get_node_name('fmt', $drive_id, $drive->{file}, $options->{'snapshot-name'});
+
+    return {
+	'node-name' => "$node_name",
+	driver => "$format",
+	file => $child,
+	cache => $cache,
+	'read-only' => read_only_json_option($drive, $options),
+    };
+}
+
+sub generate_drive_blockdev {
+    my ($storecfg, $drive, $options) = @_;
+
+    my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
+
+    die "generate_drive_blockdev called without volid/path\n" if !$drive->{file};
+    die "generate_drive_blockdev called with 'none'\n" if $drive->{file} eq 'none';
+
+    my $child = generate_file_blockdev($storecfg, $drive, $options);
+    $child = generate_format_blockdev($storecfg, $drive, $child, $options);
+
+    # this is the top filter entry point, use $drive-drive_id as nodename
+    return {
+	driver => "throttle",
+	'node-name' => "drive-$drive_id",
+	'throttle-group' => "throttle-drive-$drive_id",
+	file => $child,
+    };
+}
+
 1;
diff --git a/PVE/QemuServer/Makefile b/PVE/QemuServer/Makefile
index 8054a93b..e3671e5b 100644
--- a/PVE/QemuServer/Makefile
+++ b/PVE/QemuServer/Makefile
@@ -12,6 +12,7 @@ SOURCES=PCI.pm		\
 	MetaInfo.pm	\
 	CPUConfig.pm	\
 	CGroup.pm	\
+	Blockdev.pm	\
 	Drive.pm	\
 	QMPHelpers.pm	\
 	StateFile.pm	\
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC qemu-server 21/22] blockdev: add support for NBD paths

[Fiona Ebner]

Co-developed-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

RFC, because I didn't test it yet.

 PVE/QemuServer/Blockdev.pm | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuServer/Blockdev.pm b/PVE/QemuServer/Blockdev.pm
index 2d3760f0..839e195a 100644
--- a/PVE/QemuServer/Blockdev.pm
+++ b/PVE/QemuServer/Blockdev.pm
@@ -12,6 +12,15 @@ use PVE::Storage;
 
 use PVE::QemuServer::Drive qw(drive_is_cdrom);
 
+# gives ($host, $port, $export)
+my $NBD_PATH_RE_3 = qr/nbd:(\S+):(\d+):exportname=(\S+)/;
+
+my sub is_nbd {
+    my ($drive) = @_;
+
+    return $drive->{file} =~ $NBD_PATH_RE_3 ? 1 : 0
+}
+
 my sub get_node_name {
     my ($type, $drive_id, $volid, $snap) = @_;
 
@@ -99,7 +108,10 @@ sub generate_file_blockdev {
 
     my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
 
-    if ($drive->{file} eq 'cdrom') {
+    if ($drive->{file} =~ m/^$NBD_PATH_RE_3$/) {
+	my $server = { type => 'inet', host => $1, port => $2 };
+	$blockdev = { driver => 'nbd', server => $server, export => $3 };
+    } elsif ($drive->{file} eq 'cdrom') {
 	my $path = PVE::QemuServer::Drive::get_iso_path($storecfg, $drive->{file});
 	$blockdev = { driver => 'host_cdrom', filename => $path };
     } elsif ($drive->{file} =~ m|^/|) {
@@ -156,6 +168,7 @@ sub generate_format_blockdev {
 
     die "generate_format_blockdev called without volid/path\n" if !$drive->{file};
     die "generate_format_blockdev called with 'none'\n" if $drive->{file} eq 'none';
+    die "generate_format_blockdev called with NBD path\n" if is_nbd($drive);
 
     my $scfg;
     my $format;
@@ -200,7 +213,9 @@ sub generate_drive_blockdev {
     die "generate_drive_blockdev called with 'none'\n" if $drive->{file} eq 'none';
 
     my $child = generate_file_blockdev($storecfg, $drive, $options);
-    $child = generate_format_blockdev($storecfg, $drive, $child, $options);
+    if (!is_nbd($drive)) {
+	$child = generate_format_blockdev($storecfg, $drive, $child, $options);
+    }
 
     # this is the top filter entry point, use $drive-drive_id as nodename
     return {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC qemu-server 22/22] command line: switch to blockdev starting with machine version 10.0

[Fiona Ebner]

Co-developed-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

RFC, because it should be applied together with properly supporting
all operations.

 PVE/QemuServer.pm | 119 ++++++++++++++++++++++++++++------------------
 1 file changed, 72 insertions(+), 47 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index bfadba98..40ccafe8 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3948,14 +3948,27 @@ sub config_to_command {
 	    my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
 	    my $throttle_group = PVE::QemuServer::Blockdev::generate_throttle_group($drive);
 	    push @$cmd, '-object', to_json($throttle_group, { canonical => 1 });
+
+	    die "FIXME: blockdev for live restore not yet implemented" if $live_blockdev_name;
+
+	    my $extra_blockdev_options = {};
+	    # extra protection for templates, but SATA and IDE don't support it..
+	    $extra_blockdev_options->{'read-only'} = 1 if drive_is_read_only($conf, $drive);
+
+	    if ($drive->{file} ne 'none') {
+		my $blockdev = PVE::QemuServer::Blockdev::generate_drive_blockdev(
+		    $storecfg, $drive, $extra_blockdev_options);
+		push @$devices, '-blockdev', to_json($blockdev, { canonical => 1 });
+	    }
+	} else {
+	    my $drive_cmd = print_drive_commandline_full($storecfg, $vmid, $drive, $live_blockdev_name);
+
+	    # extra protection for templates, but SATA and IDE don't support it..
+	    $drive_cmd .= ',readonly=on' if drive_is_read_only($conf, $drive);
+
+	    push @$devices, '-drive',$drive_cmd;
 	}
 
-	my $drive_cmd = print_drive_commandline_full($storecfg, $vmid, $drive, $live_blockdev_name);
-
-	# extra protection for templates, but SATA and IDE don't support it..
-	$drive_cmd .= ',readonly=on' if drive_is_read_only($conf, $drive);
-
-	push @$devices, '-drive',$drive_cmd;
 	push @$devices, '-device', print_drivedevice_full(
 	    $storecfg, $conf, $vmid, $drive, $bridges, $arch, $machine_type);
     });
@@ -4328,16 +4341,21 @@ sub qemu_driveadd {
 	my $drive_id = PVE::QemuServer::Drive::get_drive_id($device);
 	my $throttle_group = PVE::QemuServer::Blockdev::generate_throttle_group($device);
 	mon_cmd($vmid, 'object-add', %$throttle_group);
+
+	my $blockdev = PVE::QemuServer::Blockdev::generate_drive_blockdev($storecfg, $device, {});
+	mon_cmd($vmid, 'blockdev-add', %$blockdev, timeout => 60);
+
+	return 1;
+    } else {
+	my $drive = print_drive_commandline_full($storecfg, $vmid, $device, undef);
+	$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;
+
+	die "adding drive failed: $ret\n";
     }
-
-    my $drive = print_drive_commandline_full($storecfg, $vmid, $device, undef);
-    $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;
-
-    die "adding drive failed: $ret\n";
 }
 
 sub qemu_drivedel {
@@ -4346,18 +4364,26 @@ sub qemu_drivedel {
     my $machine_type = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
 
     if (PVE::QemuServer::Machine::is_machine_version_at_least($machine_type, 10, 0)) {
+	# QEMU recursively auto-removes the file children, i.e. file and format node below the top
+	# node
+	eval {
+	    mon_cmd($vmid, 'blockdev-del', 'node-name' => "drive-$deviceid", timeout => 10 * 60);
+	};
+	die "deleting blockdev $deviceid failed : $@\n" if $@;
+	# FIXME ignore already removed scenario like below?
+
 	mon_cmd($vmid, 'object-del', id => 'throttle-drive-$deviceid');
+    } else {
+	my $ret = PVE::QemuServer::Monitor::hmp_cmd($vmid, "drive_del drive-$deviceid", 10 * 60);
+	$ret =~ s/^\s+//;
+
+	return 1 if $ret eq "";
+
+	# NB: device not found errors mean the drive was auto-deleted and we ignore the error
+	return 1 if $ret =~ m/Device \'.*?\' not found/s;
+
+	die "deleting drive $deviceid failed : $ret\n";
     }
-
-    my $ret = PVE::QemuServer::Monitor::hmp_cmd($vmid, "drive_del drive-$deviceid", 10 * 60);
-    $ret =~ s/^\s+//;
-
-    return 1 if $ret eq "";
-
-    # NB: device not found errors mean the drive was auto-deleted and we ignore the error
-    return 1 if $ret =~ m/Device \'.*?\' not found/s;
-
-    die "deleting drive $deviceid failed : $ret\n";
 }
 
 sub qemu_deviceaddverify {
@@ -4620,29 +4646,28 @@ sub qemu_block_set_io_throttle {
 		'iops-write-max-length' => int($iops_wr_max_length),
 	    }
 	);
+    } else {
+	mon_cmd($vmid, "block_set_io_throttle", device => $deviceid,
+	    bps => int($bps),
+	    bps_rd => int($bps_rd),
+	    bps_wr => int($bps_wr),
+	    iops => int($iops),
+	    iops_rd => int($iops_rd),
+	    iops_wr => int($iops_wr),
+	    bps_max => int($bps_max),
+	    bps_rd_max => int($bps_rd_max),
+	    bps_wr_max => int($bps_wr_max),
+	    iops_max => int($iops_max),
+	    iops_rd_max => int($iops_rd_max),
+	    iops_wr_max => int($iops_wr_max),
+	    bps_max_length => int($bps_max_length),
+	    bps_rd_max_length => int($bps_rd_max_length),
+	    bps_wr_max_length => int($bps_wr_max_length),
+	    iops_max_length => int($iops_max_length),
+	    iops_rd_max_length => int($iops_rd_max_length),
+	    iops_wr_max_length => int($iops_wr_max_length),
+	);
     }
-
-    mon_cmd($vmid, "block_set_io_throttle", device => $deviceid,
-	bps => int($bps),
-	bps_rd => int($bps_rd),
-	bps_wr => int($bps_wr),
-	iops => int($iops),
-	iops_rd => int($iops_rd),
-	iops_wr => int($iops_wr),
-	bps_max => int($bps_max),
-	bps_rd_max => int($bps_rd_max),
-	bps_wr_max => int($bps_wr_max),
-	iops_max => int($iops_max),
-	iops_rd_max => int($iops_rd_max),
-	iops_wr_max => int($iops_wr_max),
-	bps_max_length => int($bps_max_length),
-	bps_rd_max_length => int($bps_rd_max_length),
-	bps_wr_max_length => int($bps_wr_max_length),
-	iops_max_length => int($iops_max_length),
-	iops_rd_max_length => int($iops_rd_max_length),
-	iops_wr_max_length => int($iops_wr_max_length),
-    );
-
 }
 
 sub qemu_block_resize {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 02/22] cfg2cmd: require at least QEMU binary version 6.0

[Fabian Grünbichler]

On June 12, 2025 4:02 pm, Fiona Ebner wrote:
> The minimum supported binary version for Proxmox VE 9 is QEMU 10.0, so
> there still is more than enough room for eventual regression testing
> with older binaries.

given that PVE 8.0 released with QEMU 8.0, and PVE 7.0 already with 6.0,
dropping support for < 6.0 in PVE 8 should be okay as well, right?

> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuServer.pm          | 15 ++++++---------
>  test/cfg2cmd/old-qemu.conf |  4 ++--
>  2 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index bf0c8bfe..58a5cfa8 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -1443,7 +1443,7 @@ my sub drive_uses_cache_direct {
>  }
>  
>  sub print_drive_commandline_full {
> -    my ($storecfg, $vmid, $drive, $live_restore_name, $io_uring) = @_;
> +    my ($storecfg, $vmid, $drive, $live_restore_name) = @_;
>  
>      my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
>  
> @@ -1508,7 +1508,7 @@ sub print_drive_commandline_full {
>      $opts .= ",cache=none" if !$drive->{cache} && $cache_direct;
>  
>      if (!$drive->{aio}) {
> -	if ($io_uring && storage_allows_io_uring_default($scfg, $cache_direct)) {
> +	if (storage_allows_io_uring_default($scfg, $cache_direct)) {
>  	    # io_uring supports all cache modes
>  	    $opts .= ",aio=io_uring";
>  	} else {
> @@ -3478,9 +3478,9 @@ sub config_to_command {
>      my $kvm_binary = PVE::QemuServer::Helpers::get_command_for_arch($arch);
>      my $kvmver = kvm_user_version($kvm_binary);
>  
> -    if (!$kvmver || $kvmver !~ m/^(\d+)\.(\d+)/ || $1 < 5) {
> +    if (!$kvmver || $kvmver !~ m/^(\d+)\.(\d+)/ || $1 < 6) {
>  	$kvmver //= "undefined";
> -	die "Detected old QEMU binary ('$kvmver', at least 5.0 is required)\n";
> +	die "Detected old QEMU binary ('$kvmver', at least 6.0 is required)\n";
>      }
>  
>      my $machine_type = PVE::QemuServer::Machine::get_vm_machine($conf, $forcemachine, $arch);
> @@ -3960,8 +3960,7 @@ sub config_to_command {
>  	    push @$devices, '-blockdev', $live_restore->{blockdev};
>  	}
>  
> -	my $drive_cmd = print_drive_commandline_full(
> -	    $storecfg, $vmid, $drive, $live_blockdev_name, min_version($kvmver, 6, 0));
> +	my $drive_cmd = print_drive_commandline_full($storecfg, $vmid, $drive, $live_blockdev_name);
>  
>  	# extra protection for templates, but SATA and IDE don't support it..
>  	$drive_cmd .= ',readonly=on' if drive_is_read_only($conf, $drive);
> @@ -4334,9 +4333,7 @@ sub qemu_iothread_del {
>  sub qemu_driveadd {
>      my ($storecfg, $vmid, $device) = @_;
>  
> -    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);
> +    my $drive = print_drive_commandline_full($storecfg, $vmid, $device, undef);
>      $drive =~ s/\\/\\\\/g;
>      my $ret = PVE::QemuServer::Monitor::hmp_cmd($vmid, "drive_add auto \"$drive\"", 60);
>  
> diff --git a/test/cfg2cmd/old-qemu.conf b/test/cfg2cmd/old-qemu.conf
> index e8129cf6..e560cac7 100644
> --- a/test/cfg2cmd/old-qemu.conf
> +++ b/test/cfg2cmd/old-qemu.conf
> @@ -1,4 +1,4 @@
>  # TEST: Test QEMU version detection and expect fail on old version
> -# QEMU_VERSION: 4.0.1
> -# EXPECT_ERROR: Detected old QEMU binary ('4.0.1', at least 5.0 is required)
> +# QEMU_VERSION: 5.0.1
> +# EXPECT_ERROR: Detected old QEMU binary ('5.0.1', at least 6.0 is required)
>  smbios1: uuid=7b10d7af-b932-4c66-b2c3-3996152ec465
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 05/22] restore: parse drive with additional properties

[Fabian Grünbichler]

On June 12, 2025 4:02 pm, Fiona Ebner wrote:
> For backwards-compat, to allow dropping some drive properties that are
> long gone from QEMU.
> 
> 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 58a5cfa8..5408ba8a 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -6818,7 +6818,7 @@ sub restore_update_config_line {
>      } elsif ($line =~ m/^((ide|scsi|virtio|sata|efidisk|tpmstate)\d+):\s*(\S+)\s*$/) {
>  	my $virtdev = $1;
>  	my $value = $3;
> -	my $di = parse_drive($virtdev, $value);
> +	my $di = parse_drive($virtdev, $value, { 'additional-properties' => 1 });

there's another call to parse_drive on the raw config line in
`parse_backup_hints` - granted, that is only for cloudinit drives, but
might still make sense to set it there as well in case that call site
does more in the future?

setting this here might also have side-effects when restoring backups
made on newer PVE systems on older ones (which is never guaranteed to
work, of course), right? should we make it more selective, e.g. by
passing a list of keys that should be skipped? or just mangling the line
to remove those specific keys and not allowing *all* additional
properties? property strings are thankfully rather straight-forward to
split into properties and recombine ;)

>  	if (defined($di->{backup}) && !$di->{backup}) {
>  	    $res .= "#$line";
>  	} elsif ($map->{$virtdev}) {
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 12/22] vm start: move state file handling to dedicated module

[Fabian Grünbichler]

On June 12, 2025 4:02 pm, Fiona Ebner wrote:
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuServer.pm           | 56 +++++++------------------------------
>  PVE/QemuServer/StateFile.pm | 56 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+), 46 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 91b55cf9..25ba709d 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5577,8 +5577,6 @@ sub vm_start_nolock {
>      my $migration_type = $migrate_opts->{type};
>      my $nbd_protocol_version = $migrate_opts->{nbd_proto_version} // 0;
>  
> -    my $res = {};
> -
>      # clean up leftover reboot request files
>      eval { clear_reboot_request($vmid); };
>      warn $@ if $@;
> @@ -5649,54 +5647,20 @@ sub vm_start_nolock {
>  	    $nodename, $migrate_opts->{network});
>      }
>  
> +    my $res = {};
> +    my $statefile_is_a_volume;
> +    my $state_cmdline = [];
>      if ($statefile) {
> -	if ($statefile eq 'tcp') {
> -	    my $migrate = $res->{migrate} = { proto => 'tcp' };
> -	    $migrate->{addr} = "localhost";
> -	    my $datacenterconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
> -	    my $nodename = nodename();
> -
> -	    if (!defined($migration_type)) {
> -		if (defined($datacenterconf->{migration}->{type})) {
> -		    $migration_type = $datacenterconf->{migration}->{type};
> -		} else {
> -		    $migration_type = 'secure';
> -		}
> -	    }

I think this fallback here was already not needed..

the migration type is always set in migration context, either via `qm
start` (intra-cluster) or via the start call over mtunnel (remote
migration).

a few lines up we already have the assumptions the $statefile being set
implies $migration_type being set as well.

> -
> -	    if ($migration_type eq 'insecure') {
> -		$migrate->{addr} = $migration_ip // die "internal error - no migration IP";
> -		$migrate->{addr} = "[$migrate->{addr}]" if Net::IP::ip_is_ipv6($migrate->{addr});
> -	    }
> -
> -	    # see #4501: port reservation should be done close to usage - tell QEMU where to listen
> -	    # via QMP later
> -	    push @$cmd, '-incoming', 'defer';
> -	    push @$cmd, '-S';
> -
> -	} elsif ($statefile eq 'unix') {
> -	    # should be default for secure migrations as a ssh TCP forward
> -	    # tunnel is not deterministic reliable ready and fails regurarly
> -	    # to set up in time, so use UNIX socket forwards
> -	    my $migrate = $res->{migrate} = { proto => 'unix' };
> -	    $migrate->{addr} = "/run/qemu-server/$vmid.migrate";
> -	    unlink $migrate->{addr};
> -
> -	    $migrate->{uri} = "unix:$migrate->{addr}";
> -	    push @$cmd, '-incoming', $migrate->{uri};
> -	    push @$cmd, '-S';
> -
> -	} elsif (-e $statefile) {
> -	    push @$cmd, '-loadstate', $statefile;
> -	} else {
> -	    my $statepath = PVE::Storage::path($storecfg, $statefile);
> -	    push @$vollist, $statefile;
> -	    push @$cmd, '-loadstate', $statepath;
> -	}
> +	($state_cmdline, $res->{migrate}, $statefile_is_a_volume) =
> +	    PVE::QemuServer::StateFile::statefile_cmdline_option(
> +		$storecfg, $vmid, $statefile, $migrate_opts, $migration_ip);

which means we could just pass $migrate_type instead of $migrate_opts
here (or leave opts for future extensibility)

> +	push @$vollist, $statefile if $statefile_is_a_volume;
>      } elsif ($params->{paused}) {
> -	push @$cmd, '-S';
> +	$state_cmdline = ['-S'];
>      }
>  
> +    push $cmd->@*, $state_cmdline->@*;
> +
>      my $memory = get_current_memory($conf->{memory});
>      my $start_timeout = $params->{timeout} // config_aware_timeout($conf, $memory, $resume);
>  
> diff --git a/PVE/QemuServer/StateFile.pm b/PVE/QemuServer/StateFile.pm
> index e297839b..630ccca3 100644
> --- a/PVE/QemuServer/StateFile.pm
> +++ b/PVE/QemuServer/StateFile.pm
> @@ -29,4 +29,60 @@ sub get_migration_ip {
>      return PVE::Cluster::remote_node_ip($nodename, 1);
>  }
>  
> +# $migration_ip must be defined if using insecure TCP migration
> +sub statefile_cmdline_option {
> +    my ($storecfg, $vmid, $statefile, $migrate_opts, $migration_ip) = @_;
> +
> +    my $migration_type = $migrate_opts->{type};
> +
> +    my $statefile_is_a_volume = 0;
> +    my $res = {};
> +    my $cmd = [];
> +
> +    if ($statefile eq 'tcp') {
> +	my $migrate = $res->{migrate} = { proto => 'tcp' };
> +	$migrate->{addr} = "localhost";
> +	my $datacenterconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
> +
> +	if (!defined($migration_type)) {
> +	    if (defined($datacenterconf->{migration}->{type})) {
> +		$migration_type = $datacenterconf->{migration}->{type};
> +	    } else {
> +		$migration_type = 'secure';
> +	    }
> +	}

and here we can just assert that a type is set, instead of parsing
datacenter.cfg again and falling back..

> +
> +	if ($migration_type eq 'insecure') {
> +	    $migrate->{addr} = $migration_ip // die "internal error - no migration IP";
> +	    $migrate->{addr} = "[$migrate->{addr}]" if Net::IP::ip_is_ipv6($migrate->{addr});
> +	}
> +
> +	# see #4501: port reservation should be done close to usage - tell QEMU where to listen
> +	# via QMP later
> +	push @$cmd, '-incoming', 'defer';
> +	push @$cmd, '-S';
> +
> +    } elsif ($statefile eq 'unix') {
> +	# should be default for secure migrations as a ssh TCP forward
> +	# tunnel is not deterministic reliable ready and fails regurarly
> +	# to set up in time, so use UNIX socket forwards
> +	my $migrate = $res->{migrate} = { proto => 'unix' };
> +	$migrate->{addr} = "/run/qemu-server/$vmid.migrate";
> +	unlink $migrate->{addr};
> +
> +	$migrate->{uri} = "unix:$migrate->{addr}";
> +	push @$cmd, '-incoming', $migrate->{uri};
> +	push @$cmd, '-S';
> +
> +    } elsif (-e $statefile) {
> +	push @$cmd, '-loadstate', $statefile;
> +    } else {
> +	my $statepath = PVE::Storage::path($storecfg, $statefile);
> +	$statefile_is_a_volume = 1;
> +	push @$cmd, '-loadstate', $statepath;
> +    }
> +
> +    return ($cmd, $res->{migrate}, $statefile_is_a_volume);
> +}
> +
>  1;
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 13/22] vm start: move config_to_command() call further down

[Fabian Grünbichler]

On June 12, 2025 4:02 pm, Fiona Ebner wrote:
> For command line generation of '-blockdev', it will be necessary to
> activate the VM's volumes before calling config_to_command() and thus
> also deactivate in an error scenario. Avoid the need to put more code
> than necessary into the resulting eval.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuServer.pm | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 25ba709d..dfd4351b 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5625,18 +5625,6 @@ sub vm_start_nolock {
>  	print "Resuming suspended VM\n";
>      }
>  
> -    # Note that for certain cases like templates, the configuration is minimized, so need to ensure
> -    # the rest of the function here uses the same configuration that was used to build the command
> -    (my $cmd, my $vollist, my $spice_port, my $pci_devices, $conf) = config_to_command(
> -	$storecfg,
> -	$vmid,
> -	$conf,
> -	$defaults,
> -	$forcemachine,
> -	$forcecpu,
> -	$params->{'live-restore-backing'},
> -    );
> -
>      my $migration_ip;
>      if (
>  	($statefile && $statefile eq 'tcp' && $migration_type eq 'insecure')
> @@ -5654,16 +5642,28 @@ sub vm_start_nolock {
>  	($state_cmdline, $res->{migrate}, $statefile_is_a_volume) =
>  	    PVE::QemuServer::StateFile::statefile_cmdline_option(
>  		$storecfg, $vmid, $statefile, $migrate_opts, $migration_ip);
> -	push @$vollist, $statefile if $statefile_is_a_volume;
>      } elsif ($params->{paused}) {
>  	$state_cmdline = ['-S'];
>      }
>  
> -    push $cmd->@*, $state_cmdline->@*;
> -
>      my $memory = get_current_memory($conf->{memory});
>      my $start_timeout = $params->{timeout} // config_aware_timeout($conf, $memory, $resume);

this part here now operates on the original instead of the minimized
config in case of a template/.. - shouldn't be a problem, but wanted to
call it out in case you disagree since it wasn't mentioned..

>  
> +    # Note that for certain cases like templates, the configuration is minimized, so need to ensure
> +    # the rest of the function here uses the same configuration that was used to build the command
> +    (my $cmd, my $vollist, my $spice_port, my $pci_devices, $conf) = config_to_command(
> +	$storecfg,
> +	$vmid,
> +	$conf,
> +	$defaults,
> +	$forcemachine,
> +	$forcecpu,
> +	$params->{'live-restore-backing'},
> +    );
> +
> +    push $cmd->@*, $state_cmdline->@*;
> +    push @$vollist, $statefile if $statefile_is_a_volume;
> +
>      my $pci_reserve_list = [];
>      for my $device (values $pci_devices->%*) {
>  	next if $device->{mdev}; # we don't reserve for mdev devices
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 15/22] vm start/commandline: activate volumes before config_to_command()

[Fabian Grünbichler]

On June 12, 2025 4:02 pm, Fiona Ebner wrote:
> With '-blockdev', it is necessary to activate the volumes to generate
> the command line, because it can be necessary to check whether the
> volume is a block device or a regular file.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuServer.pm                | 61 +++++++++++++++++++++++---------
>  test/run_config2command_tests.pl | 10 ++++++
>  2 files changed, 55 insertions(+), 16 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 934adf60..82304096 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -3844,7 +3844,6 @@ sub config_to_command {
>  	push @$devices, '-watchdog-action', $wdopts->{action} if $wdopts->{action};
>      }
>  
> -    my $vollist = [];
>      my $scsicontroller = {};
>      my $ahcicontroller = {};
>      my $scsihw = defined($conf->{scsihw}) ? $conf->{scsihw} : $defaults->{scsihw};
> @@ -3857,11 +3856,6 @@ sub config_to_command {
>      PVE::QemuConfig->foreach_volume($conf, sub {
>  	my ($ds, $drive) = @_;
>  
> -	if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
> -	    check_volume_storage_type($storecfg, $drive->{file});
> -	    push @$vollist, $drive->{file};
> -	}
> -
>  	# ignore efidisk here, already added in bios/fw handling code above
>  	return if $drive->{interface} eq 'efidisk';
>  	# similar for TPM
> @@ -4035,7 +4029,6 @@ sub config_to_command {
>  
>      if (my $vmstate = $conf->{vmstate}) {
>  	my $statepath = PVE::Storage::path($storecfg, $vmstate);
> -	push @$vollist, $vmstate;
>  	push @$cmd, '-loadstate', $statepath;
>  	print "activating and using '$vmstate' as vmstate\n";
>      }
> @@ -4051,7 +4044,7 @@ sub config_to_command {
>  	push @$cmd, @$aa;
>      }
>  
> -    return wantarray ? ($cmd, $vollist, $spice_port, $pci_devices, $conf) : $cmd;
> +    return wantarray ? ($cmd, $spice_port, $pci_devices, $conf) : $cmd;
>  }
>  
>  sub spice_port {
> @@ -5649,12 +5642,18 @@ sub vm_start_nolock {
>      my $memory = get_current_memory($conf->{memory});
>      my $start_timeout = $params->{timeout} // config_aware_timeout($conf, $memory, $resume);
>  
> -    my ($cmd, $vollist, $spice_port);
> +    my $vollist = get_current_vm_volumes($storecfg, $conf);
> +    push $vollist->@*, $statefile if $statefile_is_a_volume;
> +
> +    my ($cmd, $spice_port);
>      my $pci_reserve_list = [];
>      eval {
> +	# With -blockdev, it is necessary to activate the volumes before generating the command line
> +	PVE::Storage::activate_volumes($storecfg, $vollist);
> +
>  	# Note that for certain cases like templates, the configuration is minimized, so need to ensure
>  	# the rest of the function here uses the same configuration that was used to build the command
> -	($cmd, $vollist, $spice_port, my $pci_devices, $conf) = config_to_command(
> +	($cmd, $spice_port, my $pci_devices, $conf) = config_to_command(
>  	    $storecfg,
>  	    $vmid,
>  	    $conf,
> @@ -5665,7 +5664,6 @@ sub vm_start_nolock {
>  	);
>  
>  	push $cmd->@*, $state_cmdline->@*;
> -	push @$vollist, $statefile if $statefile_is_a_volume;
>  
>  	for my $device (values $pci_devices->%*) {
>  	    next if $device->{mdev}; # we don't reserve for mdev devices
> @@ -5707,14 +5705,13 @@ sub vm_start_nolock {
>  	push @$cmd, '-uuid', $uuid if defined($uuid);
>      };
>      if (my $err = $@) {
> +	eval { PVE::Storage::deactivate_volumes($storecfg, $vollist); };
> +	warn $@ if $@;
>  	eval { cleanup_pci_devices($vmid, $conf) };
>  	warn $@ if $@;
>  	die $err;
>      }
>  
> -    PVE::Storage::activate_volumes($storecfg, $vollist);
> -
> -
>      my %silence_std_outs = (outfunc => sub {}, errfunc => sub {});
>      eval { run_command(['/bin/systemctl', 'reset-failed', "$vmid.scope"], %silence_std_outs) };
>      eval { run_command(['/bin/systemctl', 'stop', "$vmid.scope"], %silence_std_outs) };
> @@ -5982,14 +5979,25 @@ sub vm_commandline {
>  
>      my $defaults = load_defaults();
>  
> +    my $running = PVE::QemuServer::Helpers::vm_running_locally($vmid);
> +    my $volumes = [];
> +
> +    # With -blockdev, it is necessary to activate the volumes before generating the command line
> +    if (!$running) {
> +	$volumes = get_current_vm_volumes($storecfg, $conf);
> +	PVE::Storage::activate_volumes($storecfg, $volumes);
> +    }
> +
>      my $cmd;
>      eval {
>  	$cmd = config_to_command($storecfg, $vmid, $conf, $defaults, $forcemachine, $forcecpu);
>      };
>      my $err = $@;
>  
> -    # if the vm is not running, we need to clean up the reserved/created devices
> -    if (!PVE::QemuServer::Helpers::vm_running_locally($vmid)) {
> +    # if the vm is not running, need to clean up the reserved/created devices and activated volumes
> +    if (!$running) {
> +	eval { PVE::Storage::deactivate_volumes($storecfg, $volumes); };

I don't think we are allowed to do that here - `qm showcmd` can run
concurrently with other tasks (move disk, offline migration, ..) that
might be at a point in time where they've just activated but not yet
started using the volume..

in general, volume deactivation is tricky, and should often be avoided
unless it's 100% clear cut that it's safe to do (e.g., freshly allocated
volume while holding a lock, freeing a volume, ..) or required
(migration).

> +	warn $@ if $@;
>  	eval { cleanup_pci_devices($vmid, $conf) };

and this here might be dangerous as well, given that we don't take a
lock and the running state might have changed already since we checked?

should `qm showcmd` take a lock? or should we avoid doing the actual PCI
reservation if we are in the "just show command" code path?

>  	warn $@ if $@;
>      }
> @@ -6030,6 +6038,27 @@ sub get_vm_volumes {
>      return $vollist;
>  }
>  
> +# Get volumes defined in the current VM configuration, including the VM state file.
> +sub get_current_vm_volumes {
> +    my ($storecfg, $conf) = @_;
> +
> +    my $volumes = [];
> +
> +    PVE::QemuConfig->foreach_volume($conf, sub {
> +	my ($ds, $drive) = @_;
> +
> +	if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
> +	    check_volume_storage_type($storecfg, $drive->{file});
> +	    push $volumes->@*, $drive->{file};
> +	}
> +    });
> +    if (my $vmstate = $conf->{vmstate}) {
> +	push $volumes->@*, $vmstate;
> +    }
> +
> +    return $volumes;
> +}
> +
>  sub cleanup_pci_devices {
>      my ($vmid, $conf) = @_;
>  
> diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
> index 7c581ad2..5217a020 100755
> --- a/test/run_config2command_tests.pl
> +++ b/test/run_config2command_tests.pl
> @@ -255,6 +255,16 @@ $qemu_server_module->mock(
>      },
>  );
>  
> +my $storage_module = Test::MockModule->new("PVE::Storage");
> +$storage_module->mock(
> +    activate_volumes => sub {
> +	return;
> +    },
> +    deactivate_volumes => sub {
> +	return;
> +    },
> +);
> +
>  my $zfsplugin_module = Test::MockModule->new("PVE::Storage::ZFSPlugin");
>  $zfsplugin_module->mock(
>      zfs_get_lu_name => sub {
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 15/22] vm start/commandline: activate volumes before config_to_command()

[Fabian Grünbichler]

On June 12, 2025 4:02 pm, Fiona Ebner wrote:
> With '-blockdev', it is necessary to activate the volumes to generate
> the command line, because it can be necessary to check whether the
> volume is a block device or a regular file.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuServer.pm                | 61 +++++++++++++++++++++++---------
>  test/run_config2command_tests.pl | 10 ++++++
>  2 files changed, 55 insertions(+), 16 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 934adf60..82304096 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm

[..]

> @@ -6030,6 +6038,27 @@ sub get_vm_volumes {
>      return $vollist;
>  }
>  
> +# Get volumes defined in the current VM configuration, including the VM state file.
> +sub get_current_vm_volumes {
> +    my ($storecfg, $conf) = @_;
> +
> +    my $volumes = [];
> +
> +    PVE::QemuConfig->foreach_volume($conf, sub {
> +	my ($ds, $drive) = @_;
> +
> +	if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
> +	    check_volume_storage_type($storecfg, $drive->{file});
> +	    push $volumes->@*, $drive->{file};
> +	}
> +    });
> +    if (my $vmstate = $conf->{vmstate}) {
> +	push $volumes->@*, $vmstate;

could be handled with `extra_keys => ['vmstate']` and switching to
`foreach_volume_full`, like we do in `foreach_volid`?

> +    }
> +
> +    return $volumes;
> +}
> +


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 16/22] print drive device: explicitly set write-cache starting with machine version 10.0

[Fabian Grünbichler]

On June 12, 2025 4:02 pm, Fiona Ebner wrote:
> With the 'blockdev' command line option, the cache options are split
> up. While cache.direct and cache.no-flush can be set in the -blockdev
> options, cache.writeback is a front-end property and was intentionally
> removed from the 'blockdev' options by QEMU commit aaa436f998 ("block:
> Remove cache.writeback from blockdev-add"). It needs to be configured
> as the 'write-cache' property for the ide-hd/scsi-hd/virtio-blk
> device.
> 
> Table from 'man kvm':
> 
> ┌─────────────┬─────────────────┬──────────────┬────────────────┐
> │             │ cache.writeback │ cache.direct │ cache.no-flush │
> ├─────────────┼─────────────────┼──────────────┼────────────────┤
> │writeback    │ on              │ off          │ off            │
> ├─────────────┼─────────────────┼──────────────┼────────────────┤
> │none         │ on              │ on           │ off            │
> ├─────────────┼─────────────────┼──────────────┼────────────────┤
> │writethrough │ off             │ off          │ off            │
> ├─────────────┼─────────────────┼──────────────┼────────────────┤
> │directsync   │ off             │ on           │ off            │
> ├─────────────┼─────────────────┼──────────────┼────────────────┤
> │unsafe       │ on              │ off          │ on             │
> └─────────────┴─────────────────┴──────────────┴────────────────┘
> 
> Suggested-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuServer.pm | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 82304096..f9e1b3f9 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -1317,6 +1317,8 @@ sub print_drivedevice_full {
>      my $device = '';
>      my $maxdev = 0;
>  
> +    my $machine_version = extract_version($machine_type, kvm_user_version());
> +
>      my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
>      if ($drive->{interface} eq 'virtio') {
>  	my $pciaddr = print_pci_addr("$drive_id", $bridges, $arch, $machine_type);
> @@ -1327,7 +1329,6 @@ sub print_drivedevice_full {
>  	my ($maxdev, $controller, $controller_prefix) = scsihw_infos($conf, $drive);
>  	my $unit = $drive->{index} % $maxdev;
>  
> -	my $machine_version = extract_version($machine_type, kvm_user_version());
>  	my $device_type = PVE::QemuServer::Drive::get_scsi_device_type(
>  	    $drive, $storecfg, $machine_version);
>  
> @@ -1403,6 +1404,15 @@ sub print_drivedevice_full {
>  	$device .= ",serial=$serial";
>      }
>  
> +    if (min_version($machine_version, 10, 0)) {

should we add a comment here (and for similar conditionals in following
patches) that this is for the switch to blockdev? obvious now, might be
lass obvious down the line ;)

> +	if (!drive_is_cdrom($drive)) {
> +	    my $write_cache = 'on';
> +	    if (my $cache = $drive->{cache}) {
> +		$write_cache = 'off' if $cache eq 'writethrough' || $cache eq 'directsync';
> +	    }
> +	    $device .= ",write-cache=$write_cache";
> +	}
> +    }
>  
>      return $device;
>  }
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 18/22] print drive device: don't reference any drive for 'none' starting with machine version 10.0

[Fabian Grünbichler]

On June 12, 2025 4:02 pm, Fiona Ebner wrote:
> There will be no block node for 'none' after switching to '-blockdev'.
> 
> Co-developed-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
> [FE: split out from larger patch
>      do it also for non-SCSI cases]
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuServer.pm | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 90d07cee..e5f8a607 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -1322,7 +1322,13 @@ sub print_drivedevice_full {
>      my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
>      if ($drive->{interface} eq 'virtio') {
>  	my $pciaddr = print_pci_addr("$drive_id", $bridges, $arch, $machine_type);
> -	$device = "virtio-blk-pci,drive=drive-$drive_id,id=${drive_id}${pciaddr}";
> +	$device = 'virtio-blk-pci';
> +	if (min_version($machine_version, 10, 0)) { # there is no blockdev for 'none'
> +	    $device .= ",drive=drive-$drive_id" if $drive->{file} ne 'none';
> +	    $device .= ",id=${drive_id}${pciaddr}";
> +	} else {
> +	    $device .= ",drive=drive-$drive_id,id=${drive_id}${pciaddr}";

the ID is the same in both branches, so this could be (some variant of)

# blockdev has no drive for 'none'
if (!(min_version($machine_version, 10, 0) && $drive->{file} eq 'none'))
{
    $device .= "$drive=drive-$drive_id";
}

$device .= ",id=${drive_id}${pciaddr}";

> +	}
>  	$device .= ",iothread=iothread-$drive_id" if $drive->{iothread};
>      } elsif ($drive->{interface} eq 'scsi') {
>  
> @@ -1338,7 +1344,12 @@ sub print_drivedevice_full {
>  	    $device = "scsi-$device_type,bus=$controller_prefix$controller.0,channel=0,scsi-id=0"
>  	        .",lun=$drive->{index}";
>  	}
> -	$device .= ",drive=drive-$drive_id,id=$drive_id";
> +	if (min_version($machine_version, 10, 0)) { # there is no blockdev for 'none'
> +	    $device .= ",drive=drive-$drive_id" if $drive->{file} ne 'none';
> +	    $device .= ",id=$drive_id";
> +	} else {
> +	    $device .= ",drive=drive-$drive_id,id=$drive_id";
> +	}

same here

>  
>  	if ($drive->{ssd} && ($device_type eq 'block' || $device_type eq 'hd')) {
>  	    $device .= ",rotation_rate=1";
> @@ -1378,7 +1389,12 @@ sub print_drivedevice_full {
>  	} else {
>  	    $device .= ",bus=ahci$controller.$unit";
>  	}
> -	$device .= ",drive=drive-$drive_id,id=$drive_id";
> +	if (min_version($machine_version, 10, 0)) { # there is no blockdev for 'none'
> +	    $device .= ",drive=drive-$drive_id" if $drive->{file} ne 'none';
> +	    $device .= ",id=$drive_id";
> +	} else {
> +	    $device .= ",drive=drive-$drive_id,id=$drive_id";
> +	}

same here

or it could even be a small helper that adds drive+id as applicable?

>  
>  	if ($device_type eq 'hd') {
>  	    if (my $model = $drive->{model}) {
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 19/22] drive: create a throttle group for each drive starting with machine version 10.0

[Fabian Grünbichler]

On June 12, 2025 4:02 pm, Fiona Ebner wrote:
> The throttle group will be referenced via the 'blockdev' schema.
> 
> Co-developed-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuServer.pm          | 51 ++++++++++++++++++++++++++++++++++++++
>  PVE/QemuServer/Blockdev.pm | 44 ++++++++++++++++++++++++++++++++
>  PVE/QemuServer/Makefile    |  1 +
>  3 files changed, 96 insertions(+)
>  create mode 100644 PVE/QemuServer/Blockdev.pm
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index e5f8a607..bfadba98 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -51,6 +51,7 @@ use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_for
>  use PVE::QMPClient;
>  use PVE::QemuConfig;
>  use PVE::QemuConfig::NoWrite;
> +use PVE::QemuServer::Blockdev;
>  use PVE::QemuServer::Helpers qw(config_aware_timeout min_version kvm_user_version windows_version);
>  use PVE::QemuServer::Cloudinit;
>  use PVE::QemuServer::CGroup;
> @@ -3943,6 +3944,12 @@ sub config_to_command {
>  	    push @$devices, '-blockdev', $live_restore->{blockdev};
>  	}
>  
> +	if (min_version($machine_version, 10, 0)) {
> +	    my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);

this is done in generate_throttle_group as well, and the var is not used
here?

> +	    my $throttle_group = PVE::QemuServer::Blockdev::generate_throttle_group($drive);
> +	    push @$cmd, '-object', to_json($throttle_group, { canonical => 1 });
> +	}
> +
>  	my $drive_cmd = print_drive_commandline_full($storecfg, $vmid, $drive, $live_blockdev_name);
>  
>  	# extra protection for templates, but SATA and IDE don't support it..
> @@ -4315,6 +4322,14 @@ sub qemu_iothread_del {
>  sub qemu_driveadd {
>      my ($storecfg, $vmid, $device) = @_;
>  
> +    my $machine_type = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
> +
> +    if (PVE::QemuServer::Machine::is_machine_version_at_least($machine_type, 10, 0)) {
> +	my $drive_id = PVE::QemuServer::Drive::get_drive_id($device);

same

> +	my $throttle_group = PVE::QemuServer::Blockdev::generate_throttle_group($device);
> +	mon_cmd($vmid, 'object-add', %$throttle_group);
> +    }
> +
>      my $drive = print_drive_commandline_full($storecfg, $vmid, $device, undef);
>      $drive =~ s/\\/\\\\/g;
>      my $ret = PVE::QemuServer::Monitor::hmp_cmd($vmid, "drive_add auto \"$drive\"", 60);
> @@ -4328,6 +4343,12 @@ sub qemu_driveadd {
>  sub qemu_drivedel {
>      my ($vmid, $deviceid) = @_;
>  
> +    my $machine_type = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
> +
> +    if (PVE::QemuServer::Machine::is_machine_version_at_least($machine_type, 10, 0)) {
> +	mon_cmd($vmid, 'object-del', id => 'throttle-drive-$deviceid');
> +    }
> +
>      my $ret = PVE::QemuServer::Monitor::hmp_cmd($vmid, "drive_del drive-$deviceid", 10 * 60);
>      $ret =~ s/^\s+//;
>  
> @@ -4571,6 +4592,36 @@ sub qemu_block_set_io_throttle {
>  
>      return if !check_running($vmid) ;
>  
> +    my $machine_type = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
> +    if (PVE::QemuServer::Machine::is_machine_version_at_least($machine_type, 10, 0)) {
> +	mon_cmd(
> +	    $vmid,
> +	    'qom-set',
> +	    path => "throttle-$deviceid",
> +	    property => "limits",
> +	    value => {
> +		'bps-total' => int($bps),
> +		'bps-read' => int($bps_rd),
> +		'bps-write' => int($bps_wr),
> +		'iops-total' => int($iops),
> +		'iops-read' => int($iops_rd),
> +		'iops-write' => int($iops_wr),
> +		'bps-total-max' => int($bps_max),
> +		'bps-read-max' => int($bps_rd_max),
> +		'bps-write-max' => int($bps_wr_max),
> +		'iops-total-max' => int($iops_max),
> +		'iops-read-max' => int($iops_rd_max),
> +		'iops-write-max' => int($iops_wr_max),
> +		'bps-total-max-length' => int($bps_max_length),
> +		'bps-read-max-length' => int($bps_rd_max_length),
> +		'bps-write-max-length' => int($bps_wr_max_length),
> +		'iops-total-max-length' => int($iops_max_length),
> +		'iops-read-max-length' => int($iops_rd_max_length),
> +		'iops-write-max-length' => int($iops_wr_max_length),
> +	    }
> +	);
> +    }
> +
>      mon_cmd($vmid, "block_set_io_throttle", device => $deviceid,
>  	bps => int($bps),
>  	bps_rd => int($bps_rd),
> diff --git a/PVE/QemuServer/Blockdev.pm b/PVE/QemuServer/Blockdev.pm
> new file mode 100644
> index 00000000..b1150141
> --- /dev/null
> +++ b/PVE/QemuServer/Blockdev.pm
> @@ -0,0 +1,44 @@
> +package PVE::QemuServer::Blockdev;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::QemuServer::Drive;
> +
> +sub generate_throttle_group {
> +    my ($drive) = @_;
> +
> +    my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
> +
> +    my $limits = {};
> +
> +    for 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);
> +	}
> +    }
> +
> +    return {
> +	id => "throttle-drive-$drive_id",
> +	limits => $limits,
> +	'qom-type' => 'throttle-group',
> +    };
> +}
> +
> +1;
> diff --git a/PVE/QemuServer/Makefile b/PVE/QemuServer/Makefile
> index bbd5b5c0..8054a93b 100644
> --- a/PVE/QemuServer/Makefile
> +++ b/PVE/QemuServer/Makefile
> @@ -1,4 +1,5 @@
>  SOURCES=PCI.pm		\
> +	Blockdev.pm	\
>  	RNG.pm		\
>  	USB.pm		\
>  	Memory.pm	\
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 20/22] blockdev: add helpers to generate blockdev commandline

[Fabian Grünbichler]

On June 12, 2025 4:02 pm, Fiona Ebner wrote:
> The drive device and node structure is:
> front-end device {ide-hd,scsi-hd,virtio-blk-pci} (id=$drive_id)
>  - throttle node (node-name=$drive_id)

should we prefix this is well to not "use" the basic name (e.g., in case
in the future a new "topmost" node comes along)?

>   - format node  (node-name=f$encoded-info)
>    - file node   (node-name=e$encoded-info)
> 
> The node-name can only be 31 characters long and needs to start with a
> letter. The throttle node will stay inserted below the front-end
> device. The other nodes might change however, because of drive
> mirroring and similar. There currently are no good helpers to
> query/walk the block graph via QMP, x-debug-query-block-graph is
> experimental and for debugging only. Therefore, necessary information
> is encoded in the node name to be able to find it again. In
> particular, this is the volume ID, the drive ID and optionally a
> snapshot name. As long as the configuration file matches with the
> running instance, this is enough to find the correct node for
> block operations like mirror and resize.
> 
> The 'snapshot' option, for QEMU's snapshot mode, i.e. writes are only
> temporary, is not yet supported.
> 
> Originally-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
> [FE: split up patch
>      expand commit message
>      explicitly test for drivers with aio setting
>      improve readonly handling
>      improve CD-ROM handling
>      fix failure for storage named 'nbd' by always using full regex
>      improve node name generation
>      fail when drive->{snapshot} is set]
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> If we do not get around to implement the 'snapshot' option in time for
> PVE 9, we can still fall back to the legacy drive for that (and warn
> users that not all operations might work with such drives).
> 
>  PVE/QemuServer/Blockdev.pm | 172 ++++++++++++++++++++++++++++++++++++-
>  PVE/QemuServer/Makefile    |   1 +
>  2 files changed, 172 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer/Blockdev.pm b/PVE/QemuServer/Blockdev.pm
> index b1150141..2d3760f0 100644
> --- a/PVE/QemuServer/Blockdev.pm
> +++ b/PVE/QemuServer/Blockdev.pm
> @@ -3,7 +3,42 @@ package PVE::QemuServer::Blockdev;
>  use strict;
>  use warnings;
>  
> -use PVE::QemuServer::Drive;
> +use Digest::SHA;
> +use Fcntl qw(S_ISBLK S_ISCHR);
> +use File::stat;
> +use JSON;
> +
> +use PVE::Storage;
> +
> +use PVE::QemuServer::Drive qw(drive_is_cdrom);
> +
> +my sub get_node_name {
> +    my ($type, $drive_id, $volid, $snap) = @_;
> +
> +    my $info = "drive=$drive_id,";
> +    $info .= "snap=$snap," if defined($snap);
> +    $info .= "volid=$volid";
> +
> +    my $hash = substr(Digest::SHA::sha256_hex($info), 0, 30);
> +
> +    my $prefix = "";
> +    if ($type eq 'fmt') {
> +	$prefix = 'f';
> +    } elsif ($type eq 'file') {
> +	$prefix = 'e';
> +    } else {
> +	die "unknown node type '$type'";
> +    }
> +    # node-name must start with an alphabetical character
> +    return "${prefix}${hash}";
> +}
> +
> +my sub read_only_json_option {
> +    my ($drive, $options) = @_;
> +
> +    return JSON::true if $drive->{ro} || drive_is_cdrom($drive) || $options->{'read-only'};
> +    return JSON::false;

should we maybe have a generic jsonify-helper instead? this is only
called twice, but we have (counting this as well) three call sites here
that basically do

foo ? JSON::true : JSON::false

which could become `json_bool(foo)`

with a few more in PVE::VZDump::QemuServer and other qemu-server
modules..

we could still have a drive_is_ro helper in any case ;)

> +}
>  
>  sub generate_throttle_group {
>      my ($drive) = @_;
> @@ -41,4 +76,139 @@ sub generate_throttle_group {
>      };
>  }
>  
> +sub generate_blockdev_drive_cache {
> +    my ($drive, $scfg) = @_;
> +
> +    my $cache_direct = PVE::QemuServer::Drive::drive_uses_cache_direct($drive, $scfg);
> +    my $cache = {};
> +    $cache->{direct} = $cache_direct ? JSON::true : JSON::false;
> +    $cache->{'no-flush'} = $drive->{cache} && $drive->{cache} eq 'unsafe' ? JSON::true : JSON::false;
> +    return $cache;
> +}
> +
> +sub generate_file_blockdev {
> +    my ($storecfg, $drive, $options) = @_;
> +
> +    my $blockdev = {};
> +    my $scfg = undef;
> +
> +    die "generate_file_blockdev called without volid/path\n" if !$drive->{file};
> +    die "generate_file_blockdev called with 'none'\n" if $drive->{file} eq 'none';
> +    # FIXME use overlay and new config option to define storage for temp write device
> +    die "'snapshot' option is not yet supported for '-blockdev'\n" if $drive->{snapshot};
> +
> +    my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
> +
> +    if ($drive->{file} eq 'cdrom') {
> +	my $path = PVE::QemuServer::Drive::get_iso_path($storecfg, $drive->{file});
> +	$blockdev = { driver => 'host_cdrom', filename => $path };
> +    } elsif ($drive->{file} =~ m|^/|) {
> +	my $path = $drive->{file};
> +	# The 'file' driver only works for regular files. The check below is taken from
> +	# block/file-posix.c:hdev_probe_device() in QEMU. To detect CD-ROM host devices, QEMU issues
> +	# an ioctl, while the code here relies on the media=cdrom flag instead.
> +	my $st = File::stat::stat($path) or die "stat for '$path' failed - $!\n";
> +	my $driver = 'file';
> +	if (S_ISCHR($st->mode) || S_ISBLK($st->mode)) {
> +	    $driver = drive_is_cdrom($drive) ? 'host_cdrom' : 'host_device';
> +	}
> +	$blockdev = { driver => $driver, filename => $path };
> +    } else {
> +	my $volid = $drive->{file};
> +	my ($storeid) = PVE::Storage::parse_volume_id($volid);
> +
> +	my $vtype = (PVE::Storage::parse_volname($storecfg, $drive->{file}))[0];
> +	die "$drive_id: explicit media parameter is required for iso images\n"
> +	    if !defined($drive->{media}) && defined($vtype) && $vtype eq 'iso';
> +
> +	my $storage_opts = { hints => {} };
> +	$storage_opts->{hints}->{'efi-disk'} = 1 if $drive->{interface} eq 'efidisk';
> +	$storage_opts->{'snapshot-name'} = $options->{'snapshot-name'}
> +	    if defined($options->{'snapshot-name'});
> +	$blockdev = PVE::Storage::qemu_blockdev_options($storecfg, $volid, $storage_opts);
> +	$scfg = PVE::Storage::storage_config($storecfg, $storeid);
> +    }
> +
> +    $blockdev->{cache} = generate_blockdev_drive_cache($drive, $scfg);
> +
> +    my $driver = $blockdev->{driver};
> +    # only certain drivers have the aio setting
> +    if ($driver eq 'file' || $driver eq 'host_cdrom' || $driver eq 'host_device') {
> +	$blockdev->{aio} = PVE::QemuServer::Drive::aio_cmdline_option(
> +	    $scfg, $drive, $blockdev->{cache}->{direct});
> +    }
> +
> +    if (!drive_is_cdrom($drive)) {
> +	$blockdev->{discard} = $drive->{discard} && $drive->{discard} eq 'on' ? 'unmap' : 'ignore';
> +	$blockdev->{'detect-zeroes'} = PVE::QemuServer::Drive::detect_zeroes_cmdline_option($drive);
> +    }
> +
> +    $blockdev->{'node-name'} = get_node_name(
> +	'file', $drive_id, $drive->{file}, $options->{'snapshot-name'});
> +
> +    $blockdev->{'read-only'} = read_only_json_option($drive, $options);
> +
> +    return $blockdev;
> +}
> +
> +sub generate_format_blockdev {
> +    my ($storecfg, $drive, $child, $options) = @_;
> +
> +    die "generate_format_blockdev called without volid/path\n" if !$drive->{file};
> +    die "generate_format_blockdev called with 'none'\n" if $drive->{file} eq 'none';
> +
> +    my $scfg;
> +    my $format;
> +    my $volid = $drive->{file};
> +    my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
> +    my ($storeid) = PVE::Storage::parse_volume_id($volid, 1);
> +
> +    # For PVE-managed volumes, use the format from the storage layer and prevent overrides via the
> +    # drive's 'format' option. For unmanaged volumes, fallback to 'raw' to avoid auto-detection by
> +    # QEMU.
> +    if ($storeid) {
> +	$scfg = PVE::Storage::storage_config($storecfg, $storeid);
> +	$format = PVE::QemuServer::Drive::checked_volume_format($storecfg, $volid);
> +	if ($drive->{format} && $drive->{format} ne $format) {
> +	    die "drive '$drive->{interface}$drive->{index}' - volume '$volid'"
> +		." - 'format=$drive->{format}' option different from storage format '$format'\n";
> +	}
> +    } else {
> +	$format = $drive->{format} // 'raw';
> +    }
> +
> +    # define cache option on both format && file node like libvirt does
> +    my $cache = generate_blockdev_drive_cache($drive, $scfg);
> +
> +    my $node_name = get_node_name('fmt', $drive_id, $drive->{file}, $options->{'snapshot-name'});
> +
> +    return {
> +	'node-name' => "$node_name",
> +	driver => "$format",
> +	file => $child,
> +	cache => $cache,
> +	'read-only' => read_only_json_option($drive, $options),
> +    };
> +}
> +
> +sub generate_drive_blockdev {
> +    my ($storecfg, $drive, $options) = @_;
> +
> +    my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
> +
> +    die "generate_drive_blockdev called without volid/path\n" if !$drive->{file};
> +    die "generate_drive_blockdev called with 'none'\n" if $drive->{file} eq 'none';
> +
> +    my $child = generate_file_blockdev($storecfg, $drive, $options);
> +    $child = generate_format_blockdev($storecfg, $drive, $child, $options);
> +
> +    # this is the top filter entry point, use $drive-drive_id as nodename
> +    return {
> +	driver => "throttle",
> +	'node-name' => "drive-$drive_id",
> +	'throttle-group' => "throttle-drive-$drive_id",
> +	file => $child,
> +    };
> +}
> +
>  1;
> diff --git a/PVE/QemuServer/Makefile b/PVE/QemuServer/Makefile
> index 8054a93b..e3671e5b 100644
> --- a/PVE/QemuServer/Makefile
> +++ b/PVE/QemuServer/Makefile
> @@ -12,6 +12,7 @@ SOURCES=PCI.pm		\
>  	MetaInfo.pm	\
>  	CPUConfig.pm	\
>  	CGroup.pm	\
> +	Blockdev.pm	\

already added by the previous patch ;)

should we sort this list?

>  	Drive.pm	\
>  	QMPHelpers.pm	\
>  	StateFile.pm	\
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH-SERIES qemu-server 00/22] preparation for switch to blockdev

[DERUMIER, Alexandre via pve-devel]

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

From: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH-SERIES qemu-server 00/22] preparation for switch to blockdev
Date: Mon, 16 Jun 2025 06:22:46 +0000
Message-ID: <7a91faa0cd64ff186bd4015ce88f9495dcc525e3.camel@groupe-cyllene.com>

Hi Fiona,
I'm going to test your patch serie this week

>>While the last patch actually does the switch, many operations are
>>not
>>yet supported. It is included to show what changes I made there. It
>should not yet be applied and supporting everything is the goal for a
f>ollowing series based on the rest of Alexandre's patches. It was time
>>to send this however, as it's already gotten large enough (but most
>>patches are quite small, so don't worry reviewers ;)).

Do you want a rebase of my other patches on top of this patch serie ?
or are you already working on it ?


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

Re: [pve-devel] [PATCH-SERIES qemu-server 00/22] preparation for switch to blockdev

[Fiona Ebner]

Hi Alexandre,

Am 16.06.25 um 08:22 schrieb DERUMIER, Alexandre via pve-devel:
> Hi Fiona,
> I'm going to test your patch serie this week
> 
>>> While the last patch actually does the switch, many operations are
>>> not
>>> yet supported. It is included to show what changes I made there. It
>> should not yet be applied and supporting everything is the goal for a
> f>ollowing series based on the rest of Alexandre's patches. It was time
>>> to send this however, as it's already gotten large enough (but most
>>> patches are quite small, so don't worry reviewers ;)).
> 
> Do you want a rebase of my other patches on top of this patch serie ?
> or are you already working on it ?

I'll post a v2 incorporating Fabian's feedback (hopefully later today or
tomorrow). And then another series doing the switch to blockdev based on
the rest of your patches. So I think it's best if you rebase the
external snapshot support only after that. But you're very much invited
to take a look at the patches, in particular 15-22 and even more
particular 20-22. Let me know if the changes there seem good to you or
if you have any comments!

Best Regards,
Fiona


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 02/22] cfg2cmd: require at least QEMU binary version 6.0

[Fiona Ebner]

Am 13.06.25 um 11:18 schrieb Fabian Grünbichler:
> On June 12, 2025 4:02 pm, Fiona Ebner wrote:
>> The minimum supported binary version for Proxmox VE 9 is QEMU 10.0, so
>> there still is more than enough room for eventual regression testing
>> with older binaries.
> 
> given that PVE 8.0 released with QEMU 8.0, and PVE 7.0 already with 6.0,
> dropping support for < 6.0 in PVE 8 should be okay as well, right?

Yes, but there is no real need to roll out the bump for PVE 8. Except if
it would help with the backport of a fix.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 05/22] restore: parse drive with additional properties

[Fiona Ebner]

Am 13.06.25 um 11:30 schrieb Fabian Grünbichler:
> On June 12, 2025 4:02 pm, Fiona Ebner wrote:
>> For backwards-compat, to allow dropping some drive properties that are
>> long gone from QEMU.
>>
>> 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 58a5cfa8..5408ba8a 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -6818,7 +6818,7 @@ sub restore_update_config_line {
>>      } elsif ($line =~ m/^((ide|scsi|virtio|sata|efidisk|tpmstate)\d+):\s*(\S+)\s*$/) {
>>  	my $virtdev = $1;
>>  	my $value = $3;
>> -	my $di = parse_drive($virtdev, $value);
>> +	my $di = parse_drive($virtdev, $value, { 'additional-properties' => 1 });
> 
> there's another call to parse_drive on the raw config line in
> `parse_backup_hints` - granted, that is only for cloudinit drives, but
> might still make sense to set it there as well in case that call site
> does more in the future?

Good catch!

> setting this here might also have side-effects when restoring backups
> made on newer PVE systems on older ones (which is never guaranteed to
> work, of course), right?

Yes.

> should we make it more selective, e.g. by
> passing a list of keys that should be skipped? or just mangling the line
> to remove those specific keys and not allowing *all* additional
> properties? property strings are thankfully rather straight-forward to
> split into properties and recombine ;)

Yeah, that would be nicer. We already have a $skip argument for
print_property_string(). I'll try adding it for parse_property_string()
and use that.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 12/22] vm start: move state file handling to dedicated module

[Fiona Ebner]

Am 13.06.25 um 12:00 schrieb Fabian Grünbichler:
> On June 12, 2025 4:02 pm, Fiona Ebner wrote:
>> @@ -5649,54 +5647,20 @@ sub vm_start_nolock {
>>  	    $nodename, $migrate_opts->{network});
>>      }
>>  
>> +    my $res = {};
>> +    my $statefile_is_a_volume;
>> +    my $state_cmdline = [];
>>      if ($statefile) {
>> -	if ($statefile eq 'tcp') {
>> -	    my $migrate = $res->{migrate} = { proto => 'tcp' };
>> -	    $migrate->{addr} = "localhost";
>> -	    my $datacenterconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
>> -	    my $nodename = nodename();
>> -
>> -	    if (!defined($migration_type)) {
>> -		if (defined($datacenterconf->{migration}->{type})) {
>> -		    $migration_type = $datacenterconf->{migration}->{type};
>> -		} else {
>> -		    $migration_type = 'secure';
>> -		}
>> -	    }
> 
> I think this fallback here was already not needed..
>
> the migration type is always set in migration context, either via `qm
> start` (intra-cluster) or via the start call over mtunnel (remote
> migration).

Okay, I can add a preparatory patch.

> a few lines up we already have the assumptions the $statefile being set
> implies $migration_type being set as well.

What assumption do you mean? $statefile might also be set after
hibernation for example and then there is no $migration_type.

>> diff --git a/PVE/QemuServer/StateFile.pm b/PVE/QemuServer/StateFile.pm
>> index e297839b..630ccca3 100644
>> --- a/PVE/QemuServer/StateFile.pm
>> +++ b/PVE/QemuServer/StateFile.pm
>> @@ -29,4 +29,60 @@ sub get_migration_ip {
>>      return PVE::Cluster::remote_node_ip($nodename, 1);
>>  }
>>  
>> +# $migration_ip must be defined if using insecure TCP migration
>> +sub statefile_cmdline_option {
>> +    my ($storecfg, $vmid, $statefile, $migrate_opts, $migration_ip) = @_;
>> +
>> +    my $migration_type = $migrate_opts->{type};
>> +
>> +    my $statefile_is_a_volume = 0;
>> +    my $res = {};
>> +    my $cmd = [];
>> +
>> +    if ($statefile eq 'tcp') {
>> +	my $migrate = $res->{migrate} = { proto => 'tcp' };
>> +	$migrate->{addr} = "localhost";
>> +	my $datacenterconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
>> +
>> +	if (!defined($migration_type)) {
>> +	    if (defined($datacenterconf->{migration}->{type})) {
>> +		$migration_type = $datacenterconf->{migration}->{type};
>> +	    } else {
>> +		$migration_type = 'secure';
>> +	    }
>> +	}
> 
> and here we can just assert that a type is set, instead of parsing
> datacenter.cfg again and falling back..

Why "again"? It is the very same fallback, just moved here.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC qemu-server 22/22] command line: switch to blockdev starting with machine version 10.0

[DERUMIER, Alexandre via pve-devel]

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

From: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [RFC qemu-server 22/22] command line: switch to blockdev starting with machine version 10.0
Date: Mon, 16 Jun 2025 10:40:04 +0000
Message-ID: <3e1c031bdb2f018c5301a1eb328b04bde92b5c8d.camel@groupe-cyllene.com>

>>+	# QEMU recursively auto-removes the file children, i.e. file
>>and format node below the top

From my tests, it's not removing backing nodes when snapshots are used,
at least when then are defined with nodename. Don't have tested with
autogenerated backing nodes, I'll verify with linked qcow2 clones.


>>+	# node
>>+	eval {
>>+	    mon_cmd($vmid, 'blockdev-del', 'node-name' => "drive-
>>$deviceid", timeout => 10 * 60);
>>+	};
>>+	die "deleting blockdev $deviceid failed : $@\n" if $@;
>>+	# FIXME ignore already removed scenario like below?
>>+
>> 	mon_cmd($vmid, 'object-del', id => 'throttle-drive-
$deviceid');
>>+    } else {
>>+	my $ret = PVE::QemuServer::Monitor::hmp_cmd($vmid, "drive_del
>>drive-$deviceid", 10 * 60);
>>+	$ret =~ s/^\s+//;

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

Re: [pve-devel] [RFC qemu-server 21/22] blockdev: add support for NBD paths

[DERUMIER, Alexandre via pve-devel]

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

From: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [RFC qemu-server 21/22] blockdev: add support for NBD paths
Date: Mon, 16 Jun 2025 10:46:47 +0000
Message-ID: <f6e6c5e228ed3ffd8709f4f6e6ec06043c0c68d6.camel@groupe-cyllene.com>

I think I have wrongly dropped  nbd with unix socket  in my last patch
series, but previously it was:


+    } elsif ($path =~ m/^nbd:(\S+):(\d+):exportname=(\S+)$/) {
+	my $server = { type => 'inet', host => $1, port => $2 };
+	$blockdev = { driver => 'nbd', server => $server, export => $3
};
+    } elsif ($path =~ m/^nbd:unix:(\S+):exportname=(\S+)$/) {
+	my $server = { type => 'unix', path => $1 };
+	$blockdev = { driver => 'nbd', server => $server, export => $2
};


-------- Message initial --------
De: Fiona Ebner <f.ebner@proxmox.com>
Répondre à: Proxmox VE development discussion <pve-
devel@lists.proxmox.com>
À: pve-devel@lists.proxmox.com
Objet: [pve-devel] [RFC qemu-server 21/22] blockdev: add support for
NBD paths
Date: 12/06/2025 16:02:52

Co-developed-by: Alexandre Derumier <alexandre.derumier@groupe-
cyllene.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

RFC, because I didn't test it yet.

 PVE/QemuServer/Blockdev.pm | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuServer/Blockdev.pm b/PVE/QemuServer/Blockdev.pm
index 2d3760f0..839e195a 100644
--- a/PVE/QemuServer/Blockdev.pm
+++ b/PVE/QemuServer/Blockdev.pm
@@ -12,6 +12,15 @@ use PVE::Storage;
 
 use PVE::QemuServer::Drive qw(drive_is_cdrom);
 
+# gives ($host, $port, $export)
+my $NBD_PATH_RE_3 = qr/nbd:(\S+):(\d+):exportname=(\S+)/;
+
+my sub is_nbd {
+    my ($drive) = @_;
+
+    return $drive->{file} =~ $NBD_PATH_RE_3 ? 1 : 0
+}
+
 my sub get_node_name {
     my ($type, $drive_id, $volid, $snap) = @_;
 
@@ -99,7 +108,10 @@ sub generate_file_blockdev {
 
     my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
 
-    if ($drive->{file} eq 'cdrom') {
+    if ($drive->{file} =~ m/^$NBD_PATH_RE_3$/) {
+	my $server = { type => 'inet', host => $1, port => $2 };
+	$blockdev = { driver => 'nbd', server => $server, export => $3
};
+    } elsif ($drive->{file} eq 'cdrom') {
 	my $path = PVE::QemuServer::Drive::get_iso_path($storecfg,
$drive->{file});
 	$blockdev = { driver => 'host_cdrom', filename => $path };
     } elsif ($drive->{file} =~ m|^/|) {
@@ -156,6 +168,7 @@ sub generate_format_blockdev {
 
     die "generate_format_blockdev called without volid/path\n" if
!$drive->{file};
     die "generate_format_blockdev called with 'none'\n" if $drive-
>{file} eq 'none';
+    die "generate_format_blockdev called with NBD path\n" if
is_nbd($drive);
 
     my $scfg;
     my $format;
@@ -200,7 +213,9 @@ sub generate_drive_blockdev {
     die "generate_drive_blockdev called with 'none'\n" if $drive-
>{file} eq 'none';
 
     my $child = generate_file_blockdev($storecfg, $drive, $options);
-    $child = generate_format_blockdev($storecfg, $drive, $child,
$options);
+    if (!is_nbd($drive)) {
+	$child = generate_format_blockdev($storecfg, $drive, $child,
$options);
+    }
 
     # this is the top filter entry point, use $drive-drive_id as
nodename
     return {

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

Re: [pve-devel] [PATCH qemu-server 12/22] vm start: move state file handling to dedicated module

[Fabian Grünbichler]

> Fiona Ebner <f.ebner@proxmox.com> hat am 16.06.2025 12:34 CEST geschrieben:
> 
>  
> Am 13.06.25 um 12:00 schrieb Fabian Grünbichler:
> > On June 12, 2025 4:02 pm, Fiona Ebner wrote:
> >> @@ -5649,54 +5647,20 @@ sub vm_start_nolock {
> >>  	    $nodename, $migrate_opts->{network});
> >>      }
> >>  
> >> +    my $res = {};
> >> +    my $statefile_is_a_volume;
> >> +    my $state_cmdline = [];
> >>      if ($statefile) {
> >> -	if ($statefile eq 'tcp') {
> >> -	    my $migrate = $res->{migrate} = { proto => 'tcp' };
> >> -	    $migrate->{addr} = "localhost";
> >> -	    my $datacenterconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
> >> -	    my $nodename = nodename();
> >> -
> >> -	    if (!defined($migration_type)) {
> >> -		if (defined($datacenterconf->{migration}->{type})) {
> >> -		    $migration_type = $datacenterconf->{migration}->{type};
> >> -		} else {
> >> -		    $migration_type = 'secure';
> >> -		}
> >> -	    }
> > 
> > I think this fallback here was already not needed..
> >
> > the migration type is always set in migration context, either via `qm
> > start` (intra-cluster) or via the start call over mtunnel (remote
> > migration).
> 
> Okay, I can add a preparatory patch.
> 
> > a few lines up we already have the assumptions the $statefile being set
> > implies $migration_type being set as well.
> 
> What assumption do you mean? $statefile might also be set after
> hibernation for example and then there is no $migration_type.

5728	($statefile && $statefile eq 'tcp' && $migration_type eq 'insecure')

$statefile set to *tcp*, sorry.

> 
> >> diff --git a/PVE/QemuServer/StateFile.pm b/PVE/QemuServer/StateFile.pm
> >> index e297839b..630ccca3 100644
> >> --- a/PVE/QemuServer/StateFile.pm
> >> +++ b/PVE/QemuServer/StateFile.pm
> >> @@ -29,4 +29,60 @@ sub get_migration_ip {
> >>      return PVE::Cluster::remote_node_ip($nodename, 1);
> >>  }
> >>  
> >> +# $migration_ip must be defined if using insecure TCP migration
> >> +sub statefile_cmdline_option {
> >> +    my ($storecfg, $vmid, $statefile, $migrate_opts, $migration_ip) = @_;
> >> +
> >> +    my $migration_type = $migrate_opts->{type};
> >> +
> >> +    my $statefile_is_a_volume = 0;
> >> +    my $res = {};
> >> +    my $cmd = [];
> >> +
> >> +    if ($statefile eq 'tcp') {
> >> +	my $migrate = $res->{migrate} = { proto => 'tcp' };
> >> +	$migrate->{addr} = "localhost";
> >> +	my $datacenterconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
> >> +
> >> +	if (!defined($migration_type)) {
> >> +	    if (defined($datacenterconf->{migration}->{type})) {
> >> +		$migration_type = $datacenterconf->{migration}->{type};
> >> +	    } else {
> >> +		$migration_type = 'secure';
> >> +	    }
> >> +	}
> > 
> > and here we can just assert that a type is set, instead of parsing
> > datacenter.cfg again and falling back..
> 
> Why "again"? It is the very same fallback, just moved here.

again, because we already parsed it in get_migration_ip (the only other
sub in PVE::QemuServer::StateFile, which gets called in vm_start_nolock
right before this one here is).


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 13/22] vm start: move config_to_command() call further down

[Fiona Ebner]

Am 13.06.25 um 12:05 schrieb Fabian Grünbichler:
> On June 12, 2025 4:02 pm, Fiona Ebner wrote:
>> @@ -5654,16 +5642,28 @@ sub vm_start_nolock {
>>  	($state_cmdline, $res->{migrate}, $statefile_is_a_volume) =
>>  	    PVE::QemuServer::StateFile::statefile_cmdline_option(
>>  		$storecfg, $vmid, $statefile, $migrate_opts, $migration_ip);
>> -	push @$vollist, $statefile if $statefile_is_a_volume;
>>      } elsif ($params->{paused}) {
>>  	$state_cmdline = ['-S'];
>>      }
>>  
>> -    push $cmd->@*, $state_cmdline->@*;
>> -
>>      my $memory = get_current_memory($conf->{memory});
>>      my $start_timeout = $params->{timeout} // config_aware_timeout($conf, $memory, $resume);
> 
> this part here now operates on the original instead of the minimized
> config in case of a template/.. - shouldn't be a problem, but wanted to
> call it out in case you disagree since it wasn't mentioned..

Good catch! I'll keep these two lines below. We should keep using the
timeout for the effectively used config.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 12/22] vm start: move state file handling to dedicated module

[Fiona Ebner]

Am 16.06.25 um 12:54 schrieb Fabian Grünbichler:
>> Fiona Ebner <f.ebner@proxmox.com> hat am 16.06.2025 12:34 CEST geschrieben:
>> Am 13.06.25 um 12:00 schrieb Fabian Grünbichler:
>>> On June 12, 2025 4:02 pm, Fiona Ebner wrote:
>>>> @@ -5649,54 +5647,20 @@ sub vm_start_nolock {
>>>>  	    $nodename, $migrate_opts->{network});
>>>>      }
>>>>  
>>>> +    my $res = {};
>>>> +    my $statefile_is_a_volume;
>>>> +    my $state_cmdline = [];
>>>>      if ($statefile) {
>>>> -	if ($statefile eq 'tcp') {
>>>> -	    my $migrate = $res->{migrate} = { proto => 'tcp' };
>>>> -	    $migrate->{addr} = "localhost";
>>>> -	    my $datacenterconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
>>>> -	    my $nodename = nodename();
>>>> -
>>>> -	    if (!defined($migration_type)) {
>>>> -		if (defined($datacenterconf->{migration}->{type})) {
>>>> -		    $migration_type = $datacenterconf->{migration}->{type};
>>>> -		} else {
>>>> -		    $migration_type = 'secure';
>>>> -		}
>>>> -	    }
>>>
>>> I think this fallback here was already not needed..
>>>
>>> the migration type is always set in migration context, either via `qm
>>> start` (intra-cluster) or via the start call over mtunnel (remote
>>> migration).
>>
>> Okay, I can add a preparatory patch.
>>
>>> a few lines up we already have the assumptions the $statefile being set
>>> implies $migration_type being set as well.
>>
>> What assumption do you mean? $statefile might also be set after
>> hibernation for example and then there is no $migration_type.
> 
> 5728	($statefile && $statefile eq 'tcp' && $migration_type eq 'insecure')
> 
> $statefile set to *tcp*, sorry.
> 
>>
>>>> diff --git a/PVE/QemuServer/StateFile.pm b/PVE/QemuServer/StateFile.pm
>>>> index e297839b..630ccca3 100644
>>>> --- a/PVE/QemuServer/StateFile.pm
>>>> +++ b/PVE/QemuServer/StateFile.pm
>>>> @@ -29,4 +29,60 @@ sub get_migration_ip {
>>>>      return PVE::Cluster::remote_node_ip($nodename, 1);
>>>>  }
>>>>  
>>>> +# $migration_ip must be defined if using insecure TCP migration
>>>> +sub statefile_cmdline_option {
>>>> +    my ($storecfg, $vmid, $statefile, $migrate_opts, $migration_ip) = @_;
>>>> +
>>>> +    my $migration_type = $migrate_opts->{type};
>>>> +
>>>> +    my $statefile_is_a_volume = 0;
>>>> +    my $res = {};
>>>> +    my $cmd = [];
>>>> +
>>>> +    if ($statefile eq 'tcp') {
>>>> +	my $migrate = $res->{migrate} = { proto => 'tcp' };
>>>> +	$migrate->{addr} = "localhost";
>>>> +	my $datacenterconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
>>>> +
>>>> +	if (!defined($migration_type)) {
>>>> +	    if (defined($datacenterconf->{migration}->{type})) {
>>>> +		$migration_type = $datacenterconf->{migration}->{type};
>>>> +	    } else {
>>>> +		$migration_type = 'secure';
>>>> +	    }
>>>> +	}
>>>
>>> and here we can just assert that a type is set, instead of parsing
>>> datacenter.cfg again and falling back..
>>
>> Why "again"? It is the very same fallback, just moved here.
> 
> again, because we already parsed it in get_migration_ip (the only other
> sub in PVE::QemuServer::StateFile, which gets called in vm_start_nolock
> right before this one here is).

Oh, I see. I was looking at dropping the fallback without this patch
already applied and thus the context was different 0:) Now everything
makes sense. Thank you for clarifying!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 20/22] blockdev: add helpers to generate blockdev commandline

[DERUMIER, Alexandre via pve-devel]

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

From: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH qemu-server 20/22] blockdev: add helpers to generate blockdev commandline
Date: Mon, 16 Jun 2025 11:07:06 +0000
Message-ID: <13a513bf621ef2c97171011ee6b440833ce0c694.camel@groupe-cyllene.com>

>>The 'snapshot' option, for QEMU's snapshot mode, i.e. writes are only
>>temporary, is not yet supported.

from qemu manpage:

"
       -snapshot
              Write to temporary files instead of disk image files. In
this case, the raw disk image you use is not written back. You can
however force the write back by pressing C-a s (see the Disk Images
chapter in the System Emulation Users Guide).

              WARNING:
                 snapshot is incompatible with -blockdev (instead use
qemu-img to manually create snapshot images to attach to your
blockdev).  If you have mixed -blockdev and -drive declarations you can
use the 'snapshot' property on your drive declarations instead of this
global
                 option.
"

So, if we want to keep this option. (I never used it to be honest),
I think we should create a temporary qcow2 file at vm start and delete
it at vm stop.
I'm not sure of current behaviour when you use a block device ? 
how qemu known where to write a temp snapshot file ?

maybe we could reuse state storage ? only with a temp qcow2 file ? or
do we want to reuse snapshot feature of storage ?


Also, I think that some features should be disable when it's used.
(vm snapshots for example, maybe move disk,...)


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

Re: [pve-devel] [PATCH qemu-server 15/22] vm start/commandline: activate volumes before config_to_command()

[DERUMIER, Alexandre via pve-devel]

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

From: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH qemu-server 15/22] vm start/commandline: activate volumes before config_to_command()
Date: Mon, 16 Jun 2025 11:31:22 +0000
Message-ID: <409e12ad0b53d1b51c30717e6b9df3d370112df4.camel@groupe-cyllene.com>

>>With '-blockdev', it is necessary to activate the volumes to generate
>>the command line, because it can be necessary to check whether the
>>volume is a block device or a regular file.

I was thinking about that, but do we have storage with activate_volume
need to be done for a regular file ?

for lvm plugin for example, we could return always driver=>host_device.

activate_volume is always done in specific plugin, so the plugin should
be able to tell if it's a block or file storage

only custom path passthrough in vm configuration need to be checked if
it's a file or device, but we don't have activate_volume anyway







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

Re: [pve-devel] [PATCH qemu-server 15/22] vm start/commandline: activate volumes before config_to_command()

[Fiona Ebner]

Am 16.06.25 um 13:31 schrieb DERUMIER, Alexandre via pve-devel:
>>> With '-blockdev', it is necessary to activate the volumes to generate
>>> the command line, because it can be necessary to check whether the
>>> volume is a block device or a regular file.
> 
> I was thinking about that, but do we have storage with activate_volume
> need to be done for a regular file ?
> 
> for lvm plugin for example, we could return always driver=>host_device.
> 
> activate_volume is always done in specific plugin, so the plugin should
> be able to tell if it's a block or file storage
> 
> only custom path passthrough in vm configuration need to be checked if
> it's a file or device, but we don't have activate_volume anyway

Good point! It is needed for the default implementation of
qemu_blockdev_options() in Plugin.pm. We could go ahead and have all
plugins use their own implementation of qemu_blockdev_options(), that
would avoid doing any actual IO there. That sounds good to me.

The downside is that it would break third-party plugins that would
otherwise work fine with the default implementation. But actually, it's
only those that need to do something in activate_volume() to make the
volume accessible. Maybe requiring those to provide their own
qemu_blockdev_options() implementation is an acceptable level of
breakage, not entirely sure. We could even try and just guess in the
default implementation if we cannot access the volume, maybe based on
the presence of 'path'.

@Fabian does that sound acceptable to you?


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 15/22] vm start/commandline: activate volumes before config_to_command()

[Fabian Grünbichler]

> Fiona Ebner <f.ebner@proxmox.com> hat am 16.06.2025 13:51 CEST geschrieben:
> 
>  
> Am 16.06.25 um 13:31 schrieb DERUMIER, Alexandre via pve-devel:
> >>> With '-blockdev', it is necessary to activate the volumes to generate
> >>> the command line, because it can be necessary to check whether the
> >>> volume is a block device or a regular file.
> > 
> > I was thinking about that, but do we have storage with activate_volume
> > need to be done for a regular file ?
> > 
> > for lvm plugin for example, we could return always driver=>host_device.
> > 
> > activate_volume is always done in specific plugin, so the plugin should
> > be able to tell if it's a block or file storage
> > 
> > only custom path passthrough in vm configuration need to be checked if
> > it's a file or device, but we don't have activate_volume anyway
> 
> Good point! It is needed for the default implementation of
> qemu_blockdev_options() in Plugin.pm. We could go ahead and have all
> plugins use their own implementation of qemu_blockdev_options(), that
> would avoid doing any actual IO there. That sounds good to me.
> 
> The downside is that it would break third-party plugins that would
> otherwise work fine with the default implementation. But actually, it's
> only those that need to do something in activate_volume() to make the
> volume accessible. Maybe requiring those to provide their own
> qemu_blockdev_options() implementation is an acceptable level of
> breakage, not entirely sure. We could even try and just guess in the
> default implementation if we cannot access the volume, maybe based on
> the presence of 'path'.
> 
> @Fabian does that sound acceptable to you?

not sure I follow completely, to avoid misunderstandings:

variant A:

move activate_volume up front, no breakage, but more churn and higher 
chance of leaving activated volumes around in case starting fails

variant B:

move decision which driver to use to plugin via qemu_blockdev_options
how would the default/fallback look like here? without activation, we
can't rely on path since that doesn't currently require being activated,
so we can't use it to distinguish block devices from files prior to
activation..

so far we said we'd try to keep the qemu_blockdev_options switch as
breakage free as possible..


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 15/22] vm start/commandline: activate volumes before config_to_command()

[Fiona Ebner]

Am 16.06.25 um 14:46 schrieb Fabian Grünbichler:
>> Fiona Ebner <f.ebner@proxmox.com> hat am 16.06.2025 13:51 CEST geschrieben:
>> Am 16.06.25 um 13:31 schrieb DERUMIER, Alexandre via pve-devel:
>>>>> With '-blockdev', it is necessary to activate the volumes to generate
>>>>> the command line, because it can be necessary to check whether the
>>>>> volume is a block device or a regular file.
>>>
>>> I was thinking about that, but do we have storage with activate_volume
>>> need to be done for a regular file ?
>>>
>>> for lvm plugin for example, we could return always driver=>host_device.
>>>
>>> activate_volume is always done in specific plugin, so the plugin should
>>> be able to tell if it's a block or file storage
>>>
>>> only custom path passthrough in vm configuration need to be checked if
>>> it's a file or device, but we don't have activate_volume anyway
>>
>> Good point! It is needed for the default implementation of
>> qemu_blockdev_options() in Plugin.pm. We could go ahead and have all
>> plugins use their own implementation of qemu_blockdev_options(), that
>> would avoid doing any actual IO there. That sounds good to me.
>>
>> The downside is that it would break third-party plugins that would
>> otherwise work fine with the default implementation. But actually, it's
>> only those that need to do something in activate_volume() to make the
>> volume accessible. Maybe requiring those to provide their own
>> qemu_blockdev_options() implementation is an acceptable level of
>> breakage, not entirely sure. We could even try and just guess in the
>> default implementation if we cannot access the volume, maybe based on
>> the presence of 'path'.
>>
>> @Fabian does that sound acceptable to you?
> 
> not sure I follow completely, to avoid misunderstandings:
> 
> variant A:
> 
> move activate_volume up front, no breakage, but more churn and higher 
> chance of leaving activated volumes around in case starting fails

It's not "no breakage". Plugins that are not happy with the default
implementation of qemu_blockdev_options() would still fail.

> variant B:
> 
> move decision which driver to use to plugin via qemu_blockdev_options

Yes.

> how would the default/fallback look like here? without activation, we
> can't rely on path since that doesn't currently require being activated,
> so we can't use it to distinguish block devices from files prior to
> activation..

As written above, we could still try to probe the file like we currently
do. If that works, great, we don't lose anything compared to variant A.
If that fails, we look at whether path is set. If yes, then assume it's
a file-based storage and use the 'file' driver. Otherwise, use the
'host_device' driver.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 15/22] vm start/commandline: activate volumes before config_to_command()

[Fabian Grünbichler]

> Fiona Ebner <f.ebner@proxmox.com> hat am 16.06.2025 14:57 CEST geschrieben:
> 
>  
> Am 16.06.25 um 14:46 schrieb Fabian Grünbichler:
> >> Fiona Ebner <f.ebner@proxmox.com> hat am 16.06.2025 13:51 CEST geschrieben:
> >> Am 16.06.25 um 13:31 schrieb DERUMIER, Alexandre via pve-devel:
> >>>>> With '-blockdev', it is necessary to activate the volumes to generate
> >>>>> the command line, because it can be necessary to check whether the
> >>>>> volume is a block device or a regular file.
> >>>
> >>> I was thinking about that, but do we have storage with activate_volume
> >>> need to be done for a regular file ?
> >>>
> >>> for lvm plugin for example, we could return always driver=>host_device.
> >>>
> >>> activate_volume is always done in specific plugin, so the plugin should
> >>> be able to tell if it's a block or file storage
> >>>
> >>> only custom path passthrough in vm configuration need to be checked if
> >>> it's a file or device, but we don't have activate_volume anyway
> >>
> >> Good point! It is needed for the default implementation of
> >> qemu_blockdev_options() in Plugin.pm. We could go ahead and have all
> >> plugins use their own implementation of qemu_blockdev_options(), that
> >> would avoid doing any actual IO there. That sounds good to me.
> >>
> >> The downside is that it would break third-party plugins that would
> >> otherwise work fine with the default implementation. But actually, it's
> >> only those that need to do something in activate_volume() to make the
> >> volume accessible. Maybe requiring those to provide their own
> >> qemu_blockdev_options() implementation is an acceptable level of
> >> breakage, not entirely sure. We could even try and just guess in the
> >> default implementation if we cannot access the volume, maybe based on
> >> the presence of 'path'.
> >>
> >> @Fabian does that sound acceptable to you?
> > 
> > not sure I follow completely, to avoid misunderstandings:
> > 
> > variant A:
> > 
> > move activate_volume up front, no breakage, but more churn and higher 
> > chance of leaving activated volumes around in case starting fails
> 
> It's not "no breakage". Plugins that are not happy with the default
> implementation of qemu_blockdev_options() would still fail.

yes. what I meant is no breakage caused by file vs host_device, since if
we activate up front, we can query the returned path which should exist
at that point, and make an informed decision. of course, if a storage
requires a different driver altogether and/or doesn't return an actual
path that we can query, then its plugin needs adaptation in any case.

> 
> > variant B:
> > 
> > move decision which driver to use to plugin via qemu_blockdev_options
> 
> Yes.
> 
> > how would the default/fallback look like here? without activation, we
> > can't rely on path since that doesn't currently require being activated,
> > so we can't use it to distinguish block devices from files prior to
> > activation..
> 
> As written above, we could still try to probe the file like we currently
> do. If that works, great, we don't lose anything compared to variant A.
> If that fails, we look at whether path is set. If yes, then assume it's
> a file-based storage and use the 'file' driver. Otherwise, use the
> 'host_device' driver.

but this assumption will be wrong half the time? if we don't activate
before deciding the driver, then we can only guess that a given path
is a file, and not a block device? if we have to activate before guessing,
then we are back at variant A (or at a mixed variant)?


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 15/22] vm start/commandline: activate volumes before config_to_command()

[Fiona Ebner]

Am 16.06.25 um 15:15 schrieb Fabian Grünbichler:
>> Fiona Ebner <f.ebner@proxmox.com> hat am 16.06.2025 14:57 CEST geschrieben:
>> Am 16.06.25 um 14:46 schrieb Fabian Grünbichler:
>>> variant B:
>>>
>>> move decision which driver to use to plugin via qemu_blockdev_options
>>
>> Yes.
>>
>>> how would the default/fallback look like here? without activation, we
>>> can't rely on path since that doesn't currently require being activated,
>>> so we can't use it to distinguish block devices from files prior to
>>> activation..
>>
>> As written above, we could still try to probe the file like we currently
>> do. If that works, great, we don't lose anything compared to variant A.
>> If that fails, we look at whether path is set. If yes, then assume it's
>> a file-based storage and use the 'file' driver. Otherwise, use the
>> 'host_device' driver.
> 
> but this assumption will be wrong half the time? if we don't activate
> before deciding the driver, then we can only guess that a given path
> is a file, and not a block device? if we have to activate before guessing,
> then we are back at variant A (or at a mixed variant)?

Many plugins do not require activation. And I mean decide based on
$scfg->{path}. Might be a good enough default implementation still.

But I'm also fine with doing the activation up-front. We'll just need to
fix the locking for showcmd.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 15/22] vm start/commandline: activate volumes before config_to_command()

[DERUMIER, Alexandre via pve-devel]

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

From: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH qemu-server 15/22] vm start/commandline: activate volumes before config_to_command()
Date: Tue, 17 Jun 2025 06:04:46 +0000
Message-ID: <35a21a093b124a24b4afdff40f26bf98aee18002.camel@groupe-cyllene.com>


> > With '-blockdev', it is necessary to activate the volumes to
> > generate
> > the command line, because it can be necessary to check whether the
> > volume is a block device or a regular file.

>>I was thinking about that, but do we have storage with
>>activate_volume
>>need to be done for a regular file ?

>>for lvm plugin for example, we could return always
>>driver=>host_device.

>>activate_volume is always done in specific plugin, so the plugin
>>should
>>be able to tell if it's a block or file storage

>>only custom path passthrough in vm configuration need to be checked
>>if
>>it's a file or device, but we don't have activate_volume anyway


But for external snapshot, we need to check the backing file chain
inside lvm qcow2 volumes, so maybe it's still needed....

we could use the vm config, but I'm not sure that we can trust it
safely, in case of snapshot error,  or if an external backup tool like
veeam do snapshot,
and if we add snapshot replication like zfs where snapshots are not in
th vm config










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

Re: [pve-devel] [PATCH qemu-server 15/22] vm start/commandline: activate volumes before config_to_command()

[Fiona Ebner]

Am 17.06.25 um 08:04 schrieb DERUMIER, Alexandre via pve-devel:
>>> With '-blockdev', it is necessary to activate the volumes to
>>> generate
>>> the command line, because it can be necessary to check whether the
>>> volume is a block device or a regular file.
> 
>>> I was thinking about that, but do we have storage with
>>> activate_volume
>>> need to be done for a regular file ?
> 
>>> for lvm plugin for example, we could return always
>>> driver=>host_device.
> 
>>> activate_volume is always done in specific plugin, so the plugin
>>> should
>>> be able to tell if it's a block or file storage
> 
>>> only custom path passthrough in vm configuration need to be checked
>>> if
>>> it's a file or device, but we don't have activate_volume anyway
> 
> 
> But for external snapshot, we need to check the backing file chain
> inside lvm qcow2 volumes, so maybe it's still needed....
> 
> we could use the vm config, but I'm not sure that we can trust it
> safely, in case of snapshot error,  or if an external backup tool like
> veeam do snapshot,
> and if we add snapshot replication like zfs where snapshots are not in
> th vm config

Discussed this yesterday with Fabian off-list a bit more, and we decided
to go with the activate_volumes approach. It's nearly there already,
just need to not deactivate after showcmd.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 15/22] vm start/commandline: activate volumes before config_to_command()

[Fiona Ebner]

Am 13.06.25 um 12:16 schrieb Fabian Grünbichler:
> On June 12, 2025 4:02 pm, Fiona Ebner wrote:
>> @@ -5982,14 +5979,25 @@ sub vm_commandline {
>>  
>>      my $defaults = load_defaults();
>>  
>> +    my $running = PVE::QemuServer::Helpers::vm_running_locally($vmid);
>> +    my $volumes = [];
>> +
>> +    # With -blockdev, it is necessary to activate the volumes before generating the command line
>> +    if (!$running) {
>> +	$volumes = get_current_vm_volumes($storecfg, $conf);
>> +	PVE::Storage::activate_volumes($storecfg, $volumes);
>> +    }
>> +
>>      my $cmd;
>>      eval {
>>  	$cmd = config_to_command($storecfg, $vmid, $conf, $defaults, $forcemachine, $forcecpu);
>>      };
>>      my $err = $@;
>>  
>> -    # if the vm is not running, we need to clean up the reserved/created devices
>> -    if (!PVE::QemuServer::Helpers::vm_running_locally($vmid)) {
>> +    # if the vm is not running, need to clean up the reserved/created devices and activated volumes
>> +    if (!$running) {
>> +	eval { PVE::Storage::deactivate_volumes($storecfg, $volumes); };
> 
> I don't think we are allowed to do that here - `qm showcmd` can run
> concurrently with other tasks (move disk, offline migration, ..) that
> might be at a point in time where they've just activated but not yet
> started using the volume..
> 
> in general, volume deactivation is tricky, and should often be avoided
> unless it's 100% clear cut that it's safe to do (e.g., freshly allocated
> volume while holding a lock, freeing a volume, ..) or required
> (migration).

Will drop the deactivation in v2.

>> +	warn $@ if $@;
>>  	eval { cleanup_pci_devices($vmid, $conf) };
> 
> and this here might be dangerous as well, given that we don't take a
> lock and the running state might have changed already since we checked?
> 
> should `qm showcmd` take a lock? or should we avoid doing the actual PCI
> reservation if we are in the "just show command" code path?

I went with the latter approach now, as it's cleaner to not reserve at all.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 16/22] print drive device: explicitly set write-cache starting with machine version 10.0

[Fiona Ebner]

Am 13.06.25 um 12:28 schrieb Fabian Grünbichler:
> On June 12, 2025 4:02 pm, Fiona Ebner wrote:
>> @@ -1403,6 +1404,15 @@ sub print_drivedevice_full {
>>  	$device .= ",serial=$serial";
>>      }
>>  
>> +    if (min_version($machine_version, 10, 0)) {
> 
> should we add a comment here (and for similar conditionals in following
> patches) that this is for the switch to blockdev? obvious now, might be
> lass obvious down the line ;)

Will do!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 18/22] print drive device: don't reference any drive for 'none' starting with machine version 10.0

[Fiona Ebner]

Am 13.06.25 um 13:14 schrieb Fabian Grünbichler:
> On June 12, 2025 4:02 pm, Fiona Ebner wrote:
>> There will be no block node for 'none' after switching to '-blockdev'.
>>
>> Co-developed-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
>> [FE: split out from larger patch
>>      do it also for non-SCSI cases]
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>> ---
>>  PVE/QemuServer.pm | 22 +++++++++++++++++++---
>>  1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 90d07cee..e5f8a607 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -1322,7 +1322,13 @@ sub print_drivedevice_full {
>>      my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
>>      if ($drive->{interface} eq 'virtio') {
>>  	my $pciaddr = print_pci_addr("$drive_id", $bridges, $arch, $machine_type);
>> -	$device = "virtio-blk-pci,drive=drive-$drive_id,id=${drive_id}${pciaddr}";
>> +	$device = 'virtio-blk-pci';
>> +	if (min_version($machine_version, 10, 0)) { # there is no blockdev for 'none'
>> +	    $device .= ",drive=drive-$drive_id" if $drive->{file} ne 'none';
>> +	    $device .= ",id=${drive_id}${pciaddr}";
>> +	} else {
>> +	    $device .= ",drive=drive-$drive_id,id=${drive_id}${pciaddr}";
> 
> the ID is the same in both branches, so this could be (some variant of)
> 
> # blockdev has no drive for 'none'
> if (!(min_version($machine_version, 10, 0) && $drive->{file} eq 'none'))
> {
>     $device .= "$drive=drive-$drive_id";
> }
> 
> $device .= ",id=${drive_id}${pciaddr}";

Ack, I'll go with the following in v2:

if (!min_version($machine_version, 10, 0) || $drive->{file} ne 'none') {
    $device .= ",drive=drive-$drive_id";
}
$device .= ",id=${drive_id}${pciaddr}";



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 19/22] drive: create a throttle group for each drive starting with machine version 10.0

[Fiona Ebner]

Am 13.06.25 um 13:23 schrieb Fabian Grünbichler:
> On June 12, 2025 4:02 pm, Fiona Ebner wrote:
>> @@ -3943,6 +3944,12 @@ sub config_to_command {
>>  	    push @$devices, '-blockdev', $live_restore->{blockdev};
>>  	}
>>  
>> +	if (min_version($machine_version, 10, 0)) {
>> +	    my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
> 
> this is done in generate_throttle_group as well, and the var is not used
> here?

Right, this is a left-over. Will drop it in v2!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 20/22] blockdev: add helpers to generate blockdev commandline

[Fiona Ebner]

Am 13.06.25 um 13:35 schrieb Fabian Grünbichler:
> On June 12, 2025 4:02 pm, Fiona Ebner wrote:
>> The drive device and node structure is:
>> front-end device {ide-hd,scsi-hd,virtio-blk-pci} (id=$drive_id)
>>  - throttle node (node-name=$drive_id)
> 
> should we prefix this is well to not "use" the basic name (e.g., in case
> in the future a new "topmost" node comes along)?

We could, but what would be the advantage?

We can just rename it when the time comes and call the new top-most node
$drive_id then. We reference the node from the front-end device, so if
we do it now, the name would also need to be adapted there and again in
the future. But I'd rather like keeping the front-end device referring
to $drive_id and making sure that is the one we want also in the future.

We want to operate below the throttle node, because the throttling would
also apply to block jobs, not just guest reads, which we do not want. So
it's very likely that we want to insert any future node in the chain
still below the throttling node.

>> +my sub read_only_json_option {
>> +    my ($drive, $options) = @_;
>> +
>> +    return JSON::true if $drive->{ro} || drive_is_cdrom($drive) || $options->{'read-only'};
>> +    return JSON::false;
> 
> should we maybe have a generic jsonify-helper instead?

For bool, I'll add one to pve-common since I need to touch that anyways.
But in general, we cannot know if a string that looks like a number
should be a number or not without additional info.

> this is only
> called twice, but we have (counting this as well) three call sites here
> that basically do
> 
> foo ? JSON::true : JSON::false
> 
> which could become `json_bool(foo)`
> 
> with a few more in PVE::VZDump::QemuServer and other qemu-server
> modules..
> 
> we could still have a drive_is_ro helper in any case ;)

>> diff --git a/PVE/QemuServer/Makefile b/PVE/QemuServer/Makefile
>> index 8054a93b..e3671e5b 100644
>> --- a/PVE/QemuServer/Makefile
>> +++ b/PVE/QemuServer/Makefile
>> @@ -12,6 +12,7 @@ SOURCES=PCI.pm		\
>>  	MetaInfo.pm	\
>>  	CPUConfig.pm	\
>>  	CGroup.pm	\
>> +	Blockdev.pm	\
> 
> already added by the previous patch ;)

Right, too much rebasing :P

> should we sort this list?

Will add a patch in v2.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 20/22] blockdev: add helpers to generate blockdev commandline

[Fabian Grünbichler]

> Fiona Ebner <f.ebner@proxmox.com> hat am 17.06.2025 11:37 CEST geschrieben:
> 
>  
> Am 13.06.25 um 13:35 schrieb Fabian Grünbichler:
> > On June 12, 2025 4:02 pm, Fiona Ebner wrote:
> >> The drive device and node structure is:
> >> front-end device {ide-hd,scsi-hd,virtio-blk-pci} (id=$drive_id)
> >>  - throttle node (node-name=$drive_id)
> > 
> > should we prefix this is well to not "use" the basic name (e.g., in case
> > in the future a new "topmost" node comes along)?
> 
> We could, but what would be the advantage?

mainly a consistent naming scheme I guess? :)

> We can just rename it when the time comes and call the new top-most node
> $drive_id then. We reference the node from the front-end device, so if
> we do it now, the name would also need to be adapted there and again in
> the future. But I'd rather like keeping the front-end device referring
> to $drive_id and making sure that is the one we want also in the future.
> 
> We want to operate below the throttle node, because the throttling would
> also apply to block jobs, not just guest reads, which we do not want. So
> it's very likely that we want to insert any future node in the chain
> still below the throttling node.

sounds sensible

> 
> >> +my sub read_only_json_option {
> >> +    my ($drive, $options) = @_;
> >> +
> >> +    return JSON::true if $drive->{ro} || drive_is_cdrom($drive) || $options->{'read-only'};
> >> +    return JSON::false;
> > 
> > should we maybe have a generic jsonify-helper instead?
> 
> For bool, I'll add one to pve-common since I need to touch that anyways.
> But in general, we cannot know if a string that looks like a number
> should be a number or not without additional info.

yes, I meant for bools. and yes, of course for number handling we'd need
to "know" at the call site whether to encode something as a JSON number
or as-is as a string (our API is quite relaxed in that regard, but that
is not the case for other external interfaces).


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server 20/22] blockdev: add helpers to generate blockdev commandline

[Fiona Ebner]

Am 16.06.25 um 13:07 schrieb DERUMIER, Alexandre via pve-devel:
>>> The 'snapshot' option, for QEMU's snapshot mode, i.e. writes are only
>>> temporary, is not yet supported.
> 
> from qemu manpage:
> 
> "
>        -snapshot
>               Write to temporary files instead of disk image files. In
> this case, the raw disk image you use is not written back. You can
> however force the write back by pressing C-a s (see the Disk Images
> chapter in the System Emulation Users Guide).
> 
>               WARNING:
>                  snapshot is incompatible with -blockdev (instead use
> qemu-img to manually create snapshot images to attach to your
> blockdev).  If you have mixed -blockdev and -drive declarations you can
> use the 'snapshot' property on your drive declarations instead of this
> global
>                  option.
> "
> 
> So, if we want to keep this option. (I never used it to be honest),

I've briefly talked with Fabian about this a while ago. Yes, we do want
to keep it and it will be useful for
https://bugzilla.proxmox.com/show_bug.cgi?id=5187#c3

> I think we should create a temporary qcow2 file at vm start and delete
> it at vm stop.

Yes, we need an overlay. Note that there already is a FIXME comment too ;)

>> +    # FIXME use overlay and new config option to define storage for temp write device
>> +    die "'snapshot' option is not yet supported for '-blockdev'\n" if $drive->{snapshot};


> I'm not sure of current behaviour when you use a block device ? 

Note that what you linked above is for the global '-snapshot' option,
not the drive one. It just doesn't apply to blockdevs AFAIU.

> how qemu known where to write a temp snapshot file ?

It just uses /var/tmp which of course is not ideal.

> maybe we could reuse state storage ? only with a temp qcow2 file ? or
> do we want to reuse snapshot feature of storage ?

We want to add a dedicated option to select a snapshot storage. There
could be a sensible default selection (depending on restrictions for the
overlay image).

> Also, I think that some features should be disable when it's used.
> (vm snapshots for example, maybe move disk,...)

Haven't looked into that, but would be pre-existing issues, so nothing
urgent. We just shouldn't break the feature if not necessary. If we
really don't get around to support it in time for PVE 9 we could also
initially note it as a breaking change or fallback to -drive.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC qemu-server 21/22] blockdev: add support for NBD paths

[Fiona Ebner]

Am 16.06.25 um 12:46 schrieb DERUMIER, Alexandre via pve-devel:
> 
> I think I have wrongly dropped  nbd with unix socket  in my last patch
> series, but previously it was:
> 
> 
> +    } elsif ($path =~ m/^nbd:(\S+):(\d+):exportname=(\S+)$/) {
> +	my $server = { type => 'inet', host => $1, port => $2 };
> +	$blockdev = { driver => 'nbd', server => $server, export => $3
> };
> +    } elsif ($path =~ m/^nbd:unix:(\S+):exportname=(\S+)$/) {
> +	my $server = { type => 'unix', path => $1 };
> +	$blockdev = { driver => 'nbd', server => $server, export => $2
> };

Right, we use those paths too, good catch!

Just noting for completeness: In the future, we might want to switch to
the more modern syntax, but that is something for later (and requires
proper backwards-compat handling for migration). From 'man kvm':

>               Syntax  for  specifying  a  NBD  device  using   TCP,   in   preferred   URI   form:
>               "nbd://<server-ip>[:<port>]/[<export>]"
> 
>               Syntax for specifying a NBD device using Unix Domain Sockets; remember that '?' is a
>               shell glob  character  and  may  need  quoting:  "nbd+unix:///[<export>]?socket=<do‐
>               main-socket>"
> 
>               Older syntax that is also recognized: "nbd:<server-ip>:<port>[:exportname=<export>]"
> 
>               Syntax  for  specifying  a  NBD  device  using  Unix  Domain  Sockets "nbd:unix:<do‐
>               main-socket>[:exportname=<export>]"



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC qemu-server 22/22] command line: switch to blockdev starting with machine version 10.0

[Fiona Ebner]

Am 16.06.25 um 12:40 schrieb DERUMIER, Alexandre via pve-devel:
>>> +	# QEMU recursively auto-removes the file children, i.e. file
>>> and format node below the top
> 
> From my tests, it's not removing backing nodes when snapshots are used,
> at least when then are defined with nodename. Don't have tested with
> autogenerated backing nodes, I'll verify with linked qcow2 clones.

Yes, the comment explicitly mentions 'file' children, not 'backing'. But
actually, implicit backing children (i.e. for linked clones) seem to
also be removed just fine:

> [I] root@pve8a1 ~# cat /etc/pve/qemu-server/600.conf | grep disk-0.qcow2
> scsi1: nfs:109/base-109-disk-0.qcow2/600/vm-600-disk-0.qcow2,iothread=1,size=1G
> [I] root@pve8a1 ~# echo '{"execute": "qmp_capabilities"}{"execute": "query-named-block-nodes"}' | socat - /var/run/qemu-server/600.qmp | jq | grep disk-0.qcow2 | wc -l
> 18
> [I] root@pve8a1 ~# qm set 600 --delete scsi1
> update VM 600: -delete scsi1
> [I] root@pve8a1 ~# echo '{"execute": "qmp_capabilities"}{"execute": "query-named-block-nodes"}' | socat - /var/run/qemu-server/600.qmp | jq | grep disk-0.qcow2 | wc -l
> 0

If you need further functionality for the external snapshot support, you
can add it later :)


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC qemu-server 22/22] command line: switch to blockdev starting with machine version 10.0

[DERUMIER, Alexandre via pve-devel]

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

From: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>, "f.ebner@proxmox.com" <f.ebner@proxmox.com>
Subject: Re: [pve-devel] [RFC qemu-server 22/22] command line: switch to blockdev starting with machine version 10.0
Date: Wed, 18 Jun 2025 12:04:40 +0000
Message-ID: <dccfbd8c5e1c17e3a7dee21c2d703bb4d8b8a7be.camel@groupe-cyllene.com>

>>If you need further functionality for the external snapshot support,
>>you
>>can add it later :)

Yes sure !  If I remember, it's only after taking an new snapshot, so
maybe it's blockdev-reopen related.

if I start a vm with an existing backing file (like a linked clone),
it's removing fine indeed. (have retested it too to be sure)



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