[pve-devel] [PATCH qemu-server 1/4] hotplug: disk: mark aio as non-hotpluggable

[pve-devel] [PATCH qemu-server 1/4] hotplug: disk: mark aio as non-hotpluggable

[Fiona Ebner]

Previously, changing aio would be applied to the configuration, but
the drive would still be using the old setting.

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

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index a0e16dcd..6130fe82 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5388,7 +5388,8 @@ sub vmconfig_update_disk {
 		# update existing disk
 
 		# skip non hotpluggable value
-		if (safe_string_ne($drive->{discard}, $old_drive->{discard}) ||
+		if (safe_string_ne($drive->{aio}, $old_drive->{aio}) ||
+		    safe_string_ne($drive->{discard}, $old_drive->{discard}) ||
 		    safe_string_ne($drive->{iothread}, $old_drive->{iothread}) ||
 		    safe_string_ne($drive->{queues}, $old_drive->{queues}) ||
 		    safe_string_ne($drive->{cache}, $old_drive->{cache}) ||
-- 
2.30.2

[pve-devel] [PATCH qemu-server 2/4] drive commandline: factor out checks if io_uring is allowed by default

[Fiona Ebner]

while getting rid of the double negation.

In preparation to re-use the check for live disk cloning.

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

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 6130fe82..10d29a06 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1598,6 +1598,23 @@ 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;
+}
+
 sub print_drive_commandline_full {
     my ($storecfg, $vmid, $drive, $pbs_name, $io_uring) = @_;
 
@@ -1680,19 +1697,8 @@ sub print_drive_commandline_full {
 	$cache_direct = 1;
     }
 
-    # io_uring with cache mode writeback or writethrough on krbd will hang...
-    my $rbd_no_io_uring = $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...
-    my $lvm_no_io_uring = $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
-    my $cifs_no_io_uring = $scfg && $scfg->{type} eq 'cifs';
-
     if (!$drive->{aio}) {
-	if ($io_uring && !$rbd_no_io_uring && !$lvm_no_io_uring && !$cifs_no_io_uring) {
+	if ($io_uring && storage_allows_io_uring_default($scfg, $cache_direct)) {
 	    # io_uring supports all cache modes
 	    $opts .= ",aio=io_uring";
 	} else {
-- 
2.30.2

[pve-devel] [PATCH qemu-server 3/4] drive commandline: factor out determining direct cache usage into helper

[Fiona Ebner]

In preparation to re-use it for a check for live disk cloning.

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

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 10d29a06..d4b1a984 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1615,6 +1615,20 @@ my sub storage_allows_io_uring_default {
     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, $pbs_name, $io_uring) = @_;
 
@@ -1688,14 +1702,9 @@ sub print_drive_commandline_full {
 	$opts .= ",format=$format";
     }
 
-    my $cache_direct = 0;
+    my $cache_direct = drive_uses_cache_direct($drive, $scfg);
 
-    if (my $cache = $drive->{cache}) {
-	$cache_direct = $cache =~ /^(?:off|none|directsync)$/;
-    } elsif (!drive_is_cdrom($drive) && !($scfg && $scfg->{type} eq 'btrfs' && !$scfg->{nocow})) {
-	$opts .= ",cache=none";
-	$cache_direct = 1;
-    }
+    $opts .= ",cache=none" if !$drive->{cache} && $cache_direct;
 
     if (!$drive->{aio}) {
 	if ($io_uring && storage_allows_io_uring_default($scfg, $cache_direct)) {
-- 
2.30.2

[pve-devel] [PATCH qemu-server 4/4] fix #4525: clone disk: disallow mirror if it might cause problems with io_uring

[Fiona Ebner]

The target of the drive-mirror operation is opened with (essentially)
the same flags as the source in QEMU, in particular whether io_uring
should be used is inherited.

But io_uring currently causes problems in combination with certain
storage types, sometimes even leading to crashes (LVM with Linux 6.1).
Just disallow live cloning of drives when the source uses io_uring and
the target storage is not ready for it. There is one exception, namely
when source and target storage are the same. In that case, just assume
it will keep working for the target.

Migration does not seem to be affected, because there, the target VM
opens the images with the checked aio setting and then NBD exports of
those are used as the targets for mirroring.

It can be that the default determined for the source is not what's
actually used, because after a drive-mirror to a storage with a
different default, it will still use the default from the old storage.
Unfortunately, aio doesn't seem to be part of the 'query-block' QMP
command's result, so just tolerate this edge case.

The check can be removed if either
1. drive-mirror learns to open the target with a different aio setting
or more ideally
2. there are no more bad storages for io_uring.

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

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index d4b1a984..6121bcdd 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -7957,6 +7957,33 @@ sub qemu_blockjobs_cancel {
     }
 }
 
+# Check for bug #4525: drive-mirror will open the target drive with the same aio setting as the
+# source, but some storages have problems with io_uring, sometimes even leading to crashes.
+my sub clone_disk_check_io_uring {
+    my ($src_drive, $storecfg, $src_storeid, $dst_storeid, $use_drive_mirror) = @_;
+
+    return if !$use_drive_mirror;
+
+    # Don't complain when not changing storage.
+    # Assume if it works for the source, it'll work for the target too.
+    return if $src_storeid eq $dst_storeid;
+
+    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 $src_uses_io_uring;
+    if ($src_drive->{aio}) {
+	$src_uses_io_uring = $src_drive->{aio} eq 'io_uring';
+    } else {
+	$src_uses_io_uring = storage_allows_io_uring_default($src_scfg, $cache_direct);
+    }
+
+    die "target storage is known to cause issues with aio=io_uring (used by current drive)\n"
+	if $src_uses_io_uring && !storage_allows_io_uring_default($dst_scfg, $cache_direct);
+}
+
 sub clone_disk {
     my ($storecfg, $source, $dest, $full, $newvollist, $jobs, $completion, $qga, $bwlimit) = @_;
 
@@ -7992,9 +8019,8 @@ sub clone_disk {
 	$newvolid = PVE::Storage::vdisk_clone($storecfg,  $drive->{file}, $newvmid, $snapname);
 	push @$newvollist, $newvolid;
     } else {
-
-	my ($storeid, $volname) = PVE::Storage::parse_volume_id($drive->{file});
-	$storeid = $storage if $storage;
+	my ($src_storeid, $volname) = PVE::Storage::parse_volume_id($drive->{file});
+	my $storeid = $storage || $src_storeid;
 
 	my $dst_format = resolve_dst_disk_format($storecfg, $storeid, $volname, $format);
 
@@ -8014,6 +8040,8 @@ sub clone_disk {
 	    $dst_format = 'raw';
 	    $size = PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE;
 	} else {
+	    clone_disk_check_io_uring($drive, $storecfg, $src_storeid, $storeid, $use_drive_mirror);
+
 	    ($size) = PVE::Storage::volume_size_info($storecfg, $drive->{file}, 10);
 	}
 	$newvolid = PVE::Storage::vdisk_alloc(
-- 
2.30.2

[pve-devel] applied-series: Re: [PATCH qemu-server 1/4] hotplug: disk: mark aio as non-hotpluggable

[Thomas Lamprecht]

On 10/02/2023 15:19, Fiona Ebner wrote:
> Previously, changing aio would be applied to the configuration, but
> the drive would still be using the old setting.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuServer.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
>

applied series, thanks!