From: Fiona Ebner <f.ebner@proxmox.com>
Date: Fri, 10 Feb 2023 14:19:12 +0000 (+0100)
Subject: fix #4525: clone disk: disallow mirror if it might cause problems with io_uring
X-Git-Url: https://git.proxmox.com/?a=commitdiff_plain;h=8fbae1dc8f9041aaa1a1a021740a06d80e8415ed;hp=b7071d6c00d8aeb10be087589ae844414329d106;p=qemu-server.git

fix #4525: clone disk: disallow mirror if it might cause problems with io_uring

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

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index d4b1a98..6121bcd 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(