[PATCH-SERIES qemu/qemu-server 0/9] mirror: fix regressions with blockdev and allow migration with 'ro' flag

[PATCH-SERIES qemu/qemu-server 0/9] mirror: fix regressions with blockdev and allow migration with 'ro' flag

[Fiona Ebner]

fix #7299:

Since the switch to -blockdev was made in Proxmox VE 9, the root block
node is now always a throttle node for optionally limiting the guest
IO. Mirror jobs are accidentally affected by a set limit too, because
the throttle root block node was passed as the source of the mirror
job. Fix this by allowing non-root nodes to be used for the mirror job
and passing along the node below throttle instead.

Unfortunately, my upstream submission for QEMU [0] has not received a
reply yet, and thus won't land in QEMU 11.1. Pick it up downstream for
now.

[0]: https://lore.kernel.org/qemu-devel/20260611102247.622275-1-f.ebner@proxmox.com/

fix #7836:

The switch to -blockdev in Proxmox VE 9 introduced a regression,
making clone of a VM with a read-only drive fail. The reason is that
the target node is also attached as read-only and thus cannot serve as
the mirror target. To fix the issue, open the mirror target as
writeable initially and in case the VM switched to the new drive,
re-open as read-only later. Note that the drive device is always
read-only for the guest, because the top throttle node is.

For migration, there is a similar issue, since a drive with read-only
cannot serve as a writeable block export for the NBD mirror target.
This is not a regression and was always broken, but can be resolved
for -blockdev by re-opening the node below throttle as writeable,
exporting it, and then re-opening it as read-only after finishing the
migration.


Not sure if applying the read-only flags to the nodes below throttle
is fully worth it. It does add some work and complexity, but for the
guest only the read-write status of the throttle node would actually
matter.

NOTE: The QEMU binary version in qemu-server patch "partially fix
#7299: fix regression with guest IO limits applying to mirror job" in
the runs_at_least_qemu_version() check needs to be adapted when
applying, to the pve-qemu version which actually does allow non-root
nodes as a mirror source.


pve-qemu:

Fiona Ebner (1):
  add patch to allow mirror job to use non-root block node

 ...d-support-for-sync-bitmap-mode-never.patch |  8 +--
 ...check-for-bitmap-mode-without-bitmap.patch |  2 +-
 .../0006-mirror-move-some-checks-to-qmp.patch |  2 +-
 ...ror-do-not-require-a-root-block-node.patch | 68 +++++++++++++++++++
 ...ckup-Proxmox-backup-patches-for-QEMU.patch |  2 +-
 debian/patches/series                         |  1 +
 6 files changed, 76 insertions(+), 7 deletions(-)
 create mode 100644 debian/patches/extra/0053-blockdev-mirror-do-not-require-a-root-block-node.patch


qemu-server:

Fiona Ebner (8):
  partially fix #7299: fix regression with guest IO limits applying to
    mirror job
  partially fix #7299: migrate: fix regression with guest IO limits
    applying to NBD export
  blockdev: attach: support explicitly attaching as writeable
  blockdev: implement reopen function
  partially fix #7836: allow mirror for drive with read-only flag when
    using blockdev
  qm: schema: declare 'nbdstop' command as being for internal use only
  migrate: log messages from remote nbdstop command
  partially fix #7836: allow migration for drive with read-only flag
    when using blockdev

 src/PVE/API2/Qemu.pm               |   7 +-
 src/PVE/CLI/qm.pm                  |  32 +++++++-
 src/PVE/QemuMigrate.pm             |  15 +++-
 src/PVE/QemuServer.pm              |  22 +++++-
 src/PVE/QemuServer/BlockJob.pm     | 120 ++++++++++++++++++++++++++++-
 src/PVE/QemuServer/Blockdev.pm     |  76 ++++++++++++++----
 src/test/MigrationTest/QmMock.pm   |   7 --
 src/test/MigrationTest/Shared.pm   |  35 +++++++++
 src/test/run_qemu_migrate_tests.pl |   8 +-
 9 files changed, 284 insertions(+), 38 deletions(-)


Summary over all repositories:
  15 files changed, 360 insertions(+), 45 deletions(-)

-- 
Generated by git-murpp 0.5.0

[PATCH qemu 1/9] add patch to allow mirror job to use non-root block node

[Fiona Ebner]

This is in preparation to fix the regression from bug #7299 and have
guest throttle limits not apply to mirror operations too.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 ...d-support-for-sync-bitmap-mode-never.patch |  8 +--
 ...check-for-bitmap-mode-without-bitmap.patch |  2 +-
 .../0006-mirror-move-some-checks-to-qmp.patch |  2 +-
 ...ror-do-not-require-a-root-block-node.patch | 68 +++++++++++++++++++
 ...ckup-Proxmox-backup-patches-for-QEMU.patch |  2 +-
 debian/patches/series                         |  1 +
 6 files changed, 76 insertions(+), 7 deletions(-)
 create mode 100644 debian/patches/extra/0053-blockdev-mirror-do-not-require-a-root-block-node.patch

diff --git a/debian/patches/bitmap-mirror/0001-drive-mirror-add-support-for-sync-bitmap-mode-never.patch b/debian/patches/bitmap-mirror/0001-drive-mirror-add-support-for-sync-bitmap-mode-never.patch
index 0fb45d16e0..b32e871d96 100644
--- a/debian/patches/bitmap-mirror/0001-drive-mirror-add-support-for-sync-bitmap-mode-never.patch
+++ b/debian/patches/bitmap-mirror/0001-drive-mirror-add-support-for-sync-bitmap-mode-never.patch
@@ -229,7 +229,7 @@ index 089856f4a8..e6f645e0f2 100644
                       base_read_only, errp);
      if (!job) {
 diff --git a/blockdev.c b/blockdev.c
-index 6e86c6262f..b68dfef260 100644
+index 0f70973326..aa39048dd5 100644
 --- a/blockdev.c
 +++ b/blockdev.c
 @@ -2859,6 +2859,9 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
@@ -289,7 +289,7 @@ index 6e86c6262f..b68dfef260 100644
                   target_is_zero, on_source_error, on_target_error, unmap,
                   filter_node_name, copy_mode, errp);
  }
-@@ -3111,6 +3138,8 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
+@@ -3117,6 +3144,8 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
  
      blockdev_mirror_common(arg->job_id, bs, target_bs,
                             arg->replaces, arg->sync,
@@ -298,7 +298,7 @@ index 6e86c6262f..b68dfef260 100644
                             backing_mode, target_is_zero,
                             arg->has_speed, arg->speed,
                             arg->has_granularity, arg->granularity,
-@@ -3130,6 +3159,8 @@ void qmp_blockdev_mirror(const char *job_id,
+@@ -3136,6 +3165,8 @@ void qmp_blockdev_mirror(const char *job_id,
                           const char *device, const char *target,
                           const char *replaces,
                           MirrorSyncMode sync,
@@ -307,7 +307,7 @@ index 6e86c6262f..b68dfef260 100644
                           bool has_speed, int64_t speed,
                           bool has_granularity, uint32_t granularity,
                           bool has_buf_size, int64_t buf_size,
-@@ -3168,7 +3199,8 @@ void qmp_blockdev_mirror(const char *job_id,
+@@ -3182,7 +3213,8 @@ void qmp_blockdev_mirror(const char *job_id,
      }
  
      blockdev_mirror_common(job_id, bs, target_bs,
diff --git a/debian/patches/bitmap-mirror/0003-mirror-add-check-for-bitmap-mode-without-bitmap.patch b/debian/patches/bitmap-mirror/0003-mirror-add-check-for-bitmap-mode-without-bitmap.patch
index dadb0e6f85..773139a4ab 100644
--- a/debian/patches/bitmap-mirror/0003-mirror-add-check-for-bitmap-mode-without-bitmap.patch
+++ b/debian/patches/bitmap-mirror/0003-mirror-add-check-for-bitmap-mode-without-bitmap.patch
@@ -16,7 +16,7 @@ Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
  1 file changed, 3 insertions(+)
 
 diff --git a/blockdev.c b/blockdev.c
-index b68dfef260..878e47a321 100644
+index aa39048dd5..1d0494a090 100644
 --- a/blockdev.c
 +++ b/blockdev.c
 @@ -2952,6 +2952,9 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
diff --git a/debian/patches/bitmap-mirror/0006-mirror-move-some-checks-to-qmp.patch b/debian/patches/bitmap-mirror/0006-mirror-move-some-checks-to-qmp.patch
index 98c3f7b93d..abf1f03770 100644
--- a/debian/patches/bitmap-mirror/0006-mirror-move-some-checks-to-qmp.patch
+++ b/debian/patches/bitmap-mirror/0006-mirror-move-some-checks-to-qmp.patch
@@ -62,7 +62,7 @@ index 0f56ad1f54..75563e6e75 100644
  
          if (bitmap_mode != BITMAP_SYNC_MODE_NEVER) {
 diff --git a/blockdev.c b/blockdev.c
-index 878e47a321..d9575c8367 100644
+index 1d0494a090..9da3f5b8bb 100644
 --- a/blockdev.c
 +++ b/blockdev.c
 @@ -2931,7 +2931,36 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
diff --git a/debian/patches/extra/0053-blockdev-mirror-do-not-require-a-root-block-node.patch b/debian/patches/extra/0053-blockdev-mirror-do-not-require-a-root-block-node.patch
new file mode 100644
index 0000000000..b8667d7bfa
--- /dev/null
+++ b/debian/patches/extra/0053-blockdev-mirror-do-not-require-a-root-block-node.patch
@@ -0,0 +1,68 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Fiona Ebner <f.ebner@proxmox.com>
+Date: Fri, 27 Feb 2026 10:39:13 +0100
+Subject: [PATCH] blockdev: mirror: do not require a root block node
+
+The mirror block job currently only operates on a root block node. It
+might be desired to use a throttle node as the root node for limiting
+the guest IO, without also limiting mirror operations.
+
+There is no obvious reason why mirror should only be able to operate
+on a root block node.
+
+When there is no explicit @replaces argument, implicit filters on top
+are skipped. Allowing mirror to operate on non-root nodes does not
+change that fact and keeps the behavior consistent with the
+documentation for @replaces.
+
+Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
+---
+ blockdev.c | 20 +++++++++++++++++---
+ 1 file changed, 17 insertions(+), 3 deletions(-)
+
+diff --git a/blockdev.c b/blockdev.c
+index 6e86c6262f..0f70973326 100644
+--- a/blockdev.c
++++ b/blockdev.c
+@@ -2987,13 +2987,19 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
+     bool target_is_zero;
+     int ret;
+ 
+-    bs = qmp_get_root_bs(arg->device, errp);
++    bdrv_graph_rdlock_main_loop();
++    bs = bdrv_lookup_bs(arg->device, arg->device, errp);
+     if (!bs) {
++        bdrv_graph_rdunlock_main_loop();
++        return;
++    }
++    if (!bdrv_is_inserted(bs)) {
++        error_setg(errp, "Device has no medium");
++        bdrv_graph_rdunlock_main_loop();
+         return;
+     }
+ 
+     /* Early check to avoid creating target */
+-    bdrv_graph_rdlock_main_loop();
+     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
+         bdrv_graph_rdunlock_main_loop();
+         return;
+@@ -3150,10 +3156,18 @@ void qmp_blockdev_mirror(const char *job_id,
+     BlockMirrorBackingMode backing_mode = MIRROR_LEAVE_BACKING_CHAIN;
+     int ret;
+ 
+-    bs = qmp_get_root_bs(device, errp);
++    bdrv_graph_rdlock_main_loop();
++    bs = bdrv_lookup_bs(device, device, errp);
+     if (!bs) {
++        bdrv_graph_rdunlock_main_loop();
+         return;
+     }
++    if (!bdrv_is_inserted(bs)) {
++        error_setg(errp, "Device has no medium");
++        bdrv_graph_rdunlock_main_loop();
++        return;
++    }
++    bdrv_graph_rdunlock_main_loop();
+ 
+     target_bs = bdrv_lookup_bs(target, target, errp);
+     if (!target_bs) {
diff --git a/debian/patches/pve/0029-PVE-Backup-Proxmox-backup-patches-for-QEMU.patch b/debian/patches/pve/0029-PVE-Backup-Proxmox-backup-patches-for-QEMU.patch
index 74ce2a426e..ee5a0ba95c 100644
--- a/debian/patches/pve/0029-PVE-Backup-Proxmox-backup-patches-for-QEMU.patch
+++ b/debian/patches/pve/0029-PVE-Backup-Proxmox-backup-patches-for-QEMU.patch
@@ -165,7 +165,7 @@ index 1fd28d59eb..c5c1fcd583 100644
 +    hmp_handle_error(mon, error);
 +}
 diff --git a/blockdev.c b/blockdev.c
-index d9575c8367..38e24e12bd 100644
+index 9da3f5b8bb..3a9f557f5b 100644
 --- a/blockdev.c
 +++ b/blockdev.c
 @@ -37,6 +37,7 @@
diff --git a/debian/patches/series b/debian/patches/series
index a93c8eb474..804581193f 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -50,6 +50,7 @@ extra/0049-hw-dma-i8257-Return-zeroes-for-read_memory-in-verify.patch
 extra/0050-intel_iommu-Correctly-set-pt-bit-in-extended-capabil.patch
 extra/0051-intel_iommu-fix-guest-triggerable-abort-on-oversized.patch
 extra/0052-backends-rng-cap-request-size-to-avoid-oversized-all.patch
+extra/0053-blockdev-mirror-do-not-require-a-root-block-node.patch
 bitmap-mirror/0001-drive-mirror-add-support-for-sync-bitmap-mode-never.patch
 bitmap-mirror/0002-drive-mirror-add-support-for-conditional-and-always-.patch
 bitmap-mirror/0003-mirror-add-check-for-bitmap-mode-without-bitmap.patch
-- 
2.47.3

[PATCH qemu-server 2/9] partially fix #7299: fix regression with guest IO limits applying to mirror job

[Fiona Ebner]

Since the switch to -blockdev was made in Proxmox VE 9, the root block
node is now always a throttle node for optionally limiting the guest
IO. Mirror jobs are accidentally affected by a set limit too, because
the throttle root block node was passed as the source of the mirror
job. Since pve-qemu-kvm 11.0.3, non-root nodes can be used for the
mirror job too. Use the node node below throttle as the mirror source.

During mirror completion, the throttle node on top is also drained by
QEMU (which temporarily ignores the throttling).

For migration, the dirty bitmap also needs to be created on the node
below throttle.

For the tests, it is necessary to mock the query-block QMP command for
the blockdev module.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

The QEMU binary version in the runs_at_least_qemu_version() check
needs to be adapted when applying, to the pve-qemu version which
actually does allow non-root nodes as a mirror source.

 src/PVE/QemuMigrate.pm             |  8 ++++--
 src/PVE/QemuServer/BlockJob.pm     | 42 +++++++++++++++++++++++++++---
 src/test/MigrationTest/Shared.pm   | 35 +++++++++++++++++++++++++
 src/test/run_qemu_migrate_tests.pl |  8 +++---
 4 files changed, 83 insertions(+), 10 deletions(-)

diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
index 8da6f15d..ab930955 100644
--- a/src/PVE/QemuMigrate.pm
+++ b/src/PVE/QemuMigrate.pm
@@ -626,7 +626,9 @@ sub handle_replication {
 
             # start tracking before replication to get full delta + a few duplicates
             $self->log('info', "$drive: start tracking writes using block-dirty-bitmap '$bitmap'");
-            mon_cmd($vmid, 'block-dirty-bitmap-add', node => "drive-$drive", name => $bitmap);
+            my $block_node_name =
+                PVE::QemuServer::BlockJob::get_mirror_source_node($vmid, "drive-$drive");
+            mon_cmd($vmid, 'block-dirty-bitmap-add', node => "$block_node_name", name => $bitmap);
 
             # other info comes from target node in phase 2
             $self->{target_drive}->{$drive}->{bitmap} = $bitmap;
@@ -817,9 +819,11 @@ sub cleanup_bitmaps {
         my $bitmap = $self->{target_drive}->{$drive}->{bitmap};
         next if !$bitmap;
         $self->log('info', "$drive: removing block-dirty-bitmap '$bitmap'");
+        my $block_node_name =
+            PVE::QemuServer::BlockJob::get_mirror_source_node($self->{vmid}, "drive-$drive");
         mon_cmd(
             $self->{vmid}, 'block-dirty-bitmap-remove',
-            node => "drive-$drive",
+            node => "$block_node_name",
             name => $bitmap,
         );
     }
diff --git a/src/PVE/QemuServer/BlockJob.pm b/src/PVE/QemuServer/BlockJob.pm
index 921f046c..7be5a1e4 100644
--- a/src/PVE/QemuServer/BlockJob.pm
+++ b/src/PVE/QemuServer/BlockJob.pm
@@ -13,7 +13,9 @@ use PVE::Storage;
 use PVE::QemuServer::Agent qw(qga_check_running);
 use PVE::QemuServer::Blockdev;
 use PVE::QemuServer::Drive qw(checked_volume_format);
+use PVE::QemuServer::Machine;
 use PVE::QemuServer::Monitor qw(mon_cmd qmp_cmd vm_qmp_peer);
+use PVE::QemuServer::QMPHelpers;
 use PVE::QemuServer::RunState;
 
 # If the job was started with auto-dismiss=false, it's necessary to dismiss it manually. Using this
@@ -242,12 +244,44 @@ sub monitor {
     }
 }
 
+=pod
+
+=head3 get_mirror_source_node
+
+    my $node_name = get_mirror_source_node($vmid, $device_id);
+
+Return the node for the drive device C<$device_id> that should serve as the mirror source.
+
+=cut
+
+sub get_mirror_source_node {
+    my ($vmid, $device_id) = @_;
+
+    # Before QEMU binary 11.0.3, blockdev-mirror requires a root node.
+    if (!PVE::QemuServer::QMPHelpers::runs_at_least_qemu_version($vmid, 11, 0, 3)) {
+        return $device_id;
+    }
+
+    # The switch to -blockdev happened with machine version 10.0.
+    my $machine_type = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
+    if (!PVE::QemuServer::Machine::is_machine_version_at_least($machine_type, 10, 0)) {
+        return $device_id;
+    }
+
+    # Avoid throttle limits intended for the guest applying to the mirror job (bug #7299).
+    return PVE::QemuServer::Blockdev::get_node_name_below_throttle(
+        vm_qmp_peer($vmid), $device_id, 1,
+    );
+}
+
 my sub common_mirror_qmp_options {
-    my ($device_id, $qemu_target, $src_bitmap, $bwlimit) = @_;
+    my ($vmid, $device_id, $qemu_target, $src_bitmap, $bwlimit) = @_;
+
+    my $source_node_name = get_mirror_source_node($vmid, $device_id);
 
     my $opts = {
         timeout => 10,
-        device => "$device_id",
+        device => "$source_node_name",
         sync => "full",
         target => $qemu_target,
         'auto-dismiss' => JSON::false,
@@ -304,7 +338,7 @@ sub qemu_drive_mirror {
         $qemu_target = $is_zero_initialized ? "zeroinit:$dst_path" : $dst_path;
     }
 
-    my $opts = common_mirror_qmp_options($device_id, $qemu_target, $src_bitmap, $bwlimit);
+    my $opts = common_mirror_qmp_options($vmid, $device_id, $qemu_target, $src_bitmap, $bwlimit);
     $opts->{mode} = "existing";
     $opts->{format} = $format if $format;
 
@@ -498,7 +532,7 @@ sub blockdev_mirror {
     };
 
     my $qmp_opts = common_mirror_qmp_options(
-        $device_id, $target_node_name, $source->{bitmap}, $options->{bwlimit},
+        $vmid, $device_id, $target_node_name, $source->{bitmap}, $options->{bwlimit},
     );
 
     $qmp_opts->{'job-id'} = "$jobid";
diff --git a/src/test/MigrationTest/Shared.pm b/src/test/MigrationTest/Shared.pm
index a51e1692..c69a141a 100644
--- a/src/test/MigrationTest/Shared.pm
+++ b/src/test/MigrationTest/Shared.pm
@@ -159,6 +159,41 @@ $qemu_server_module->mock(
     },
 );
 
+our $qemu_server_blockdev_module = Test::MockModule->new("PVE::QemuServer::Blockdev");
+$qemu_server_blockdev_module->mock(
+    mon_cmd => sub {
+        my ($vmid, $command, %params) = @_;
+
+        if ($command eq 'query-block') {
+            my $vm_config = PVE::QemuConfig->load_config($vmid);
+            my $res = [];
+            PVE::QemuConfig->foreach_volume(
+                $vm_config,
+                sub {
+                    my ($ds, $drive) = @_;
+                    my $entry = {
+                        qdev => "$ds",
+                        inserted => {
+                            drv => 'throttle',
+                            'node-name' => "drive-$ds",
+                            children => [
+                                {
+                                    'node-name' => "f-$ds",
+                                    child => 'file',
+                                },
+                            ],
+                        },
+                    };
+                    push($res->@*, $entry);
+                },
+            );
+            return $res;
+        } else {
+            die "mon_cmd (mocked) - implement me: $command";
+        }
+    },
+);
+
 our $qemu_server_ovmf_module = Test::MockModule->new("PVE::QemuServer::OVMF");
 $qemu_server_ovmf_module->mock(
     get_efivars_size => sub {
diff --git a/src/test/run_qemu_migrate_tests.pl b/src/test/run_qemu_migrate_tests.pl
index 05eed1d9..9d6c495e 100755
--- a/src/test/run_qemu_migrate_tests.pl
+++ b/src/test/run_qemu_migrate_tests.pl
@@ -1367,8 +1367,8 @@ my $tests = [
         target_volids => local_volids_for_vm(105),
         expected_calls => {
             %{$replicated_expected_calls_online},
-            'block-dirty-bitmap-add-drive-scsi0' => 1,
-            'block-dirty-bitmap-add-drive-ide0' => 1,
+            'block-dirty-bitmap-add-f-scsi0' => 1,
+            'block-dirty-bitmap-add-f-ide0' => 1,
         },
         expected => {
             source_volids => local_volids_for_vm(105),
@@ -1416,8 +1416,8 @@ my $tests = [
         target_volids => local_volids_for_vm(105),
         expected_calls => {
             %{$replicated_expected_calls_online},
-            'block-dirty-bitmap-add-drive-scsi0' => 1,
-            'block-dirty-bitmap-add-drive-ide0' => 1,
+            'block-dirty-bitmap-add-f-scsi0' => 1,
+            'block-dirty-bitmap-add-f-ide0' => 1,
         },
         expected => {
             source_volids => local_volids_for_vm(105),
-- 
2.47.3

[PATCH qemu-server 3/9] partially fix #7299: migrate: fix regression with guest IO limits applying to NBD export

[Fiona Ebner]

Related to commit "partially fix #7299: fix regression with guest IO
limits applying to mirror job", but taking care of the NBD export on
the target side. Since the switch to -blockdev with version 10.0,
write limits intended for the guest would apply to NBD exports as
well. This is because the top throttle node was exported. Export the
node below instead to avoid the issue.

For the test, change to using the version of query-block from
the MigrationTest::Shared module to make it work.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer.pm            | 13 ++++++++++++-
 src/test/MigrationTest/QmMock.pm |  7 -------
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 9aec7f9c..6b03e170 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -5929,7 +5929,18 @@ sub vm_start_nolock {
             my $drivestr = $nbd->{$opt}->{drivestr};
             my $volid = $nbd->{$opt}->{volid};
 
-            my $block_node = $block_info->{$opt}->{inserted}->{'node-name'};
+            my $top = $block_info->{$opt}->{inserted};
+            die "no block node found for drive '$opt'\n" if !$top;
+            my $block_node = $top->{'node-name'};
+            my $machine_type = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
+            if (PVE::QemuServer::Machine::is_machine_version_at_least($machine_type, 10, 0)) {
+                # Since the switch to -blockdev with machine version 10, the top node is a throttle
+                # node. Use the node below it to avoid the guest limits applying to the NBD export.
+                die "top node for drive '$opt' is not a throttle node" if $top->{drv} ne 'throttle';
+                my $children = { map { $_->{child} => $_ } $top->{children}->@* };
+                $block_node = $children->{file}->{'node-name'}
+                    or die "drive '$opt': throttle node without file child node name\n";
+            }
 
             mon_cmd(
                 $vmid,
diff --git a/src/test/MigrationTest/QmMock.pm b/src/test/MigrationTest/QmMock.pm
index 8f8899dc..aa5fac4f 100644
--- a/src/test/MigrationTest/QmMock.pm
+++ b/src/test/MigrationTest/QmMock.pm
@@ -50,8 +50,6 @@ my sub mocked_mon_cmd {
         return;
     } elsif ($command eq 'block-export-add') {
         return;
-    } elsif ($command eq 'query-block') {
-        return [];
     } elsif ($command eq 'qom-set') {
         return;
     } elsif ($command eq 'query-version') {
@@ -70,11 +68,6 @@ $inotify_module->mock(
     },
 );
 
-my $qemu_server_blockdev_module = Test::MockModule->new("PVE::QemuServer::Blockdev");
-$qemu_server_blockdev_module->mock(
-    mon_cmd => \&mocked_mon_cmd,
-);
-
 my $qemu_server_helpers_module = Test::MockModule->new("PVE::QemuServer::Helpers");
 $qemu_server_helpers_module->mock(
     vm_running_locally => sub {
-- 
2.47.3

[PATCH qemu-server 4/9] blockdev: attach: support explicitly attaching as writeable

[Fiona Ebner]

If the read-only option is explicitly set to 0, override the drive
configuration and attach the drive as writeable. This is useful for
making exports and mirror targets below throttle writeable even if the
drive itself is configured as read-only.

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

diff --git a/src/PVE/QemuServer/Blockdev.pm b/src/PVE/QemuServer/Blockdev.pm
index 101c747c..879e1aab 100644
--- a/src/PVE/QemuServer/Blockdev.pm
+++ b/src/PVE/QemuServer/Blockdev.pm
@@ -202,7 +202,9 @@ sub get_node_name_below_throttle {
 my sub read_only_json_option {
     my ($drive, $options) = @_;
 
-    return json_bool($drive->{ro} || drive_is_cdrom($drive) || $options->{'read-only'});
+    return json_bool($options->{'read-only'}) if defined($options->{'read-only'});
+
+    return json_bool($drive->{ro} || drive_is_cdrom($drive));
 }
 
 # Common blockdev options that need to be set across the whole throttle->fmt->file chain.
@@ -571,8 +573,8 @@ Parameters:
 
 =item C<< $options->{'no-throttle'} >>: Do not insert a throttle node as the top node.
 
-=item C<< $options->{'read-only'} >>: Attach the image as read-only irrespective of the
-configuration in C<$drive>.
+=item C<< $options->{'read-only'} >>: Attach the image as read-only or as writeable irrespective of
+the configuration in C<$drive>.
 
 =item C<< $options->{size} >>: Attach the image with this virtual size. Must be smaller than the
 actual size of the image. The image format must be C<raw>.
-- 
2.47.3

[PATCH qemu-server 5/9] blockdev: implement reopen function

[Fiona Ebner]

Similar to the attach() function, but for re-opening an already
attached block node. Currently limited to non-throttle nodes.

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

diff --git a/src/PVE/QemuServer/Blockdev.pm b/src/PVE/QemuServer/Blockdev.pm
index 879e1aab..dea46ca0 100644
--- a/src/PVE/QemuServer/Blockdev.pm
+++ b/src/PVE/QemuServer/Blockdev.pm
@@ -548,6 +548,33 @@ my sub blockdev_add {
 
 =pod
 
+=head3 generate_drive_blockdev_for_peer
+
+Helper for the C<attach> and C<reopen> functions, see C<attach> for the parameter descriptions.
+Returns the generated blockdev and the QMP peer information.
+
+=cut
+
+my sub generate_drive_blockdev_for_peer {
+    my ($storecfg, $id, $drive, $options) = @_;
+
+    my $qmp_peer = $options->{qsd} ? qsd_qmp_peer($id) : vm_qmp_peer($id);
+
+    my $machine_version;
+    if ($options->{qsd}) { # qemu-storage-daemon runs with the installed binary version
+        $machine_version =
+            'pc-i440fx-' . PVE::QemuServer::Machine::latest_installed_machine_version();
+    } else {
+        $machine_version = PVE::QemuServer::Machine::get_current_qemu_machine($id);
+    }
+
+    my $blockdev = generate_drive_blockdev($storecfg, $drive, $machine_version, $options);
+
+    return ($blockdev, $qmp_peer);
+}
+
+=pod
+
 =head3 attach
 
     my ($node_name, $read_only) = attach($storecfg, $id, $drive, $options);
@@ -596,17 +623,7 @@ state image.
 sub attach {
     my ($storecfg, $id, $drive, $options) = @_;
 
-    my $qmp_peer = $options->{qsd} ? qsd_qmp_peer($id) : vm_qmp_peer($id);
-
-    my $machine_version;
-    if ($options->{qsd}) { # qemu-storage-daemon runs with the installed binary version
-        $machine_version =
-            'pc-i440fx-' . PVE::QemuServer::Machine::latest_installed_machine_version();
-    } else {
-        $machine_version = PVE::QemuServer::Machine::get_current_qemu_machine($id);
-    }
-
-    my $blockdev = generate_drive_blockdev($storecfg, $drive, $machine_version, $options);
+    my ($blockdev, $qmp_peer) = generate_drive_blockdev_for_peer($storecfg, $id, $drive, $options);
 
     my $throttle_group_id;
     if (parse_top_node_name($blockdev->{'node-name'})) { # device top nodes need a throttle group
@@ -713,6 +730,35 @@ sub detach_fleecing_block_nodes {
     }
 }
 
+=pod
+
+=head3 reopen
+
+    reopen($storecfg, $id, $drive, $options);
+
+Re-open the already attached drive C<$drive> of the VM C<$id> while considering the additional
+options C<$options>. See C<attach> for the description of parameters and options. The C<no-throttle>
+option is currently required to be set.
+
+=cut
+
+sub reopen {
+    my ($storecfg, $id, $drive, $options) = @_;
+
+    # Will need additional consideration regarding the throttle group objects.
+    die "reopen for a throttle node is not implemented yet\n" if !$options->{'no-throttle'};
+
+    my ($blockdev, $qmp_peer) = generate_drive_blockdev_for_peer($storecfg, $id, $drive, $options);
+
+    if ($blockdev->{driver} eq 'zeroinit') { # zero-init does not support re-open, defer to child
+        $blockdev = $blockdev->{file};
+    }
+
+    qmp_cmd($qmp_peer, 'blockdev-reopen', options => [$blockdev]);
+
+    return;
+}
+
 sub resize {
     my ($vmid, $deviceid, $storecfg, $volid, $size) = @_;
 
-- 
2.47.3

[PATCH qemu-server 6/9] partially fix #7836: allow mirror for drive with read-only flag when using blockdev

[Fiona Ebner]

The switch to -blockdev in Proxmox VE 9 introduced a regression,
making clone of a VM with a read-only drive fail. The reason is that
the target node is also attached as read-only and thus cannot serve as
the mirror target. To fix the issue, open the mirror target as
writeable initially and in case the VM switched to the new drive,
re-open as read-only later. Note that the drive device is always
read-only for the guest, because the top throttle node is.

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

diff --git a/src/PVE/QemuServer/BlockJob.pm b/src/PVE/QemuServer/BlockJob.pm
index 7be5a1e4..bce350da 100644
--- a/src/PVE/QemuServer/BlockJob.pm
+++ b/src/PVE/QemuServer/BlockJob.pm
@@ -80,6 +80,25 @@ sub qemu_blockjobs_cancel {
     }
 }
 
+sub check_reopen_after_complete {
+    my ($job_info) = @_;
+
+    my $reopen_info = $job_info->{'target-reopen-upon-complete'} or return;
+
+    my ($storecfg, $vmid, $drive, $options) = $reopen_info->@{qw(storecfg vmid drive options)};
+
+    # Re-opening is currently only done to restore read-only below the throttle node. For the guest,
+    # the drive will be seen as read-only if the throttle node already is, so a failure here is not
+    # too bad.
+    eval { PVE::QemuServer::Blockdev::reopen($storecfg, $vmid, $drive, $options); };
+    if (my $err = $@) {
+        my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
+        print "$drive_id: failed to re-apply read-only flag below throttle - $err\n";
+    }
+
+    return;
+}
+
 # $completion can be either
 # 'complete': wait until all jobs are ready, job-complete them (default)
 # 'cancel': wait until all jobs are ready, block-job-cancel them
@@ -210,6 +229,7 @@ sub monitor {
                             if ($completion eq 'complete') {
                                 $detach_node_name = $jobs->{$job_id}->{'source-node-name'};
                                 qmp_cmd($qmp_peer, 'job-complete', id => $job_id);
+                                check_reopen_after_complete($jobs->{$job_id});
                             } elsif ($completion eq 'cancel') {
                                 $detach_node_name = $jobs->{$job_id}->{'target-node-name'};
                                 qmp_cmd($qmp_peer, 'block-job-cancel', device => $job_id);
@@ -519,6 +539,14 @@ sub blockdev_mirror {
         }
     }
 
+    # If the drive is configured as read-only, attach the mirror target as writeable initially and
+    # re-open it as read-only later. For the guest, the drive will be seen as read-only if the
+    # throttle node is, even in case mirror switches to the new target.
+    my $dest_is_read_only = $dest_drive->{ro};
+    if ($dest_is_read_only) {
+        $attach_dest_opts->{'read-only'} = 0;
+    }
+
     # Note that if 'aio' is not explicitly set, i.e. default, it can change if source and target
     # don't both allow or both not allow 'io_uring' as the default.
     my ($target_node_name) =
@@ -531,6 +559,16 @@ sub blockdev_mirror {
         'target-node-name' => $target_node_name,
     };
 
+    if ($dest_is_read_only) {
+        delete($attach_dest_opts->{'read-only'}); # remove the override for re-opening later
+        $jobs->{$jobid}->{'target-reopen-upon-complete'} = {
+            storecfg => $storecfg,
+            vmid => $vmid,
+            drive => $dest_drive,
+            options => $attach_dest_opts,
+        };
+    }
+
     my $qmp_opts = common_mirror_qmp_options(
         $vmid, $device_id, $target_node_name, $source->{bitmap}, $options->{bwlimit},
     );
-- 
2.47.3

[PATCH qemu-server 7/9] qm: schema: declare 'nbdstop' command as being for internal use only

[Fiona Ebner]

Declare the 'qm nbdstop' command as being for internal use only. There
is no way to start an NBD server via the CLI/API, so the command only
makes sense in the context of migration when issued by the Proxmox VE
stack itself.

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

diff --git a/src/PVE/CLI/qm.pm b/src/PVE/CLI/qm.pm
index b903c1f1..9a53a4f2 100755
--- a/src/PVE/CLI/qm.pm
+++ b/src/PVE/CLI/qm.pm
@@ -414,7 +414,7 @@ __PACKAGE__->register_method({
     name => 'nbdstop',
     path => 'nbdstop',
     method => 'PUT',
-    description => "Stop embedded nbd server.",
+    description => "Stop embedded nbd server. For internal use only!",
     parameters => {
         additionalProperties => 0,
         properties => {
-- 
2.47.3

[PATCH qemu-server 8/9] migrate: log messages from remote nbdstop command

[Fiona Ebner]

The output was always ignored since the introduction of the command in
2016 with commit b74cad8a ("add live storage migration with vm
migration"). But it's nicer to show messages from the remote side to
detect failures and for debugging purposes.

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

diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
index ab930955..a252f848 100644
--- a/src/PVE/QemuMigrate.pm
+++ b/src/PVE/QemuMigrate.pm
@@ -1738,7 +1738,12 @@ sub phase3_cleanup {
                 my $cmd = [@{ $self->{rem_ssh} }, 'qm', 'nbdstop', $vmid];
 
                 eval {
-                    PVE::Tools::run_command($cmd, outfunc => sub { }, errfunc => sub { });
+                    PVE::Tools::run_command(
+                        $cmd,
+                        logfunc => sub {
+                            $self->log('info', "[$self->{node}] $_[0]");
+                        },
+                    );
                 };
                 if (my $err = $@) {
                     $self->log('err', $err);
-- 
2.47.3

[PATCH qemu-server 9/9] partially fix #7836: allow migration for drive with read-only flag when using blockdev

[Fiona Ebner]

A read-only block node cannot serve as a writeable block export for
the NBD mirror target. With -blockdev, the issue can be resolved by
re-opening the node below throttle as writeable, exporting it, and
then re-opening it as read-only after finishing the migration.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/API2/Qemu.pm           |  7 ++++--
 src/PVE/CLI/qm.pm              | 30 +++++++++++++++++++++++--
 src/PVE/QemuServer.pm          |  9 ++++++++
 src/PVE/QemuServer/BlockJob.pm | 40 ++++++++++++++++++++++++++++++++++
 4 files changed, 82 insertions(+), 4 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 28cbb9b0..7262c221 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -43,7 +43,6 @@ use PVE::QemuServer::MetaInfo;
 use PVE::QemuServer::Network;
 use PVE::QemuServer::OVMF;
 use PVE::QemuServer::PCI;
-use PVE::QemuServer::QMPHelpers;
 use PVE::QemuServer::RNG;
 use PVE::QemuServer::RunState;
 use PVE::QemuServer::USB;
@@ -6957,7 +6956,11 @@ __PACKAGE__->register_method({
                     return;
                 },
                 'nbdstop' => sub {
-                    PVE::QemuServer::QMPHelpers::nbd_stop($state->{vmid});
+                    PVE::QemuServer::BlockJob::nbd_stop_after_mirror(
+                        $state->{storecfg},
+                        $state->{vmid},
+                        $state->{conf},
+                    );
                     return;
                 },
                 'resume' => sub {
diff --git a/src/PVE/CLI/qm.pm b/src/PVE/CLI/qm.pm
index 9a53a4f2..af6b3170 100755
--- a/src/PVE/CLI/qm.pm
+++ b/src/PVE/CLI/qm.pm
@@ -428,8 +428,34 @@ __PACKAGE__->register_method({
 
         my $vmid = $param->{vmid};
 
-        eval { PVE::QemuServer::QMPHelpers::nbd_stop($vmid) };
-        warn $@ if $@;
+        my $obtained_lock;
+
+        eval {
+            PVE::QemuConfig->lock_config(
+                $vmid,
+                sub {
+                    $obtained_lock = 1;
+                    my $storecfg = PVE::Storage::config();
+                    my $conf = PVE::QemuConfig->load_config($vmid);
+                    eval {
+                        PVE::QemuServer::BlockJob::nbd_stop_after_mirror(
+                            $storecfg, $vmid, $conf,
+                        );
+                    };
+                    warn $@ if $@;
+                },
+            );
+        };
+        if (my $err = $@) {
+            if (!$obtained_lock) {
+                # Fall back to just stopping the NBD server without checking disks.
+                warn "nbdstop: failed to obtain lock, skipping disk check - $@\n";
+                eval { PVE::QemuServer::QMPHelpers::nbd_stop($vmid) };
+                warn $@ if $@;
+            } else {
+                die $err;
+            }
+        }
 
         return;
     },
diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 6b03e170..7470a4a1 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -5929,6 +5929,10 @@ sub vm_start_nolock {
             my $drivestr = $nbd->{$opt}->{drivestr};
             my $volid = $nbd->{$opt}->{volid};
 
+            my $drive = parse_drive($opt, $drivestr);
+            die "$opt: internal error - unable to parse drive '$drivestr'\n" if !$drive;
+            my $drive_is_readonly = $drive->{ro};
+
             my $top = $block_info->{$opt}->{inserted};
             die "no block node found for drive '$opt'\n" if !$top;
             my $block_node = $top->{'node-name'};
@@ -5940,6 +5944,11 @@ sub vm_start_nolock {
                 my $children = { map { $_->{child} => $_ } $top->{children}->@* };
                 $block_node = $children->{file}->{'node-name'}
                     or die "drive '$opt': throttle node without file child node name\n";
+
+                if ($drive_is_readonly) { # re-open the node below throttle as writeable
+                    my $reopen_opts = { 'no-throttle' => 1, 'read-only' => 0 };
+                    PVE::QemuServer::Blockdev::reopen($storecfg, $vmid, $drive, $reopen_opts);
+                }
             }
 
             mon_cmd(
diff --git a/src/PVE/QemuServer/BlockJob.pm b/src/PVE/QemuServer/BlockJob.pm
index bce350da..1cf9149e 100644
--- a/src/PVE/QemuServer/BlockJob.pm
+++ b/src/PVE/QemuServer/BlockJob.pm
@@ -619,4 +619,44 @@ sub mirror {
     }
 }
 
+sub nbd_stop_after_mirror {
+    my ($storecfg, $vmid, $conf) = @_;
+
+    my $exports = [];
+
+    my $machine_type = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
+    if (PVE::QemuServer::Machine::is_machine_version_at_least($machine_type, 10, 0)) {
+        eval { $exports = mon_cmd($vmid, 'query-block-exports'); };
+        log_warn("stop nbd: unable to query block exports $@") if $@;
+    }
+
+    PVE::QemuServer::QMPHelpers::nbd_stop($vmid);
+
+    for my $export ($exports->@*) {
+        my ($opt) = $export->{id} =~ m/^drive-(.*)$/;
+        if (!defined($opt)) {
+            warn("stop nbd: got unexpected export ID '$export->{id}'");
+        } elsif (!PVE::QemuServer::Drive::is_valid_drivename($opt)) {
+            warn("stop nbd: got unexpected export ID '$export->{id}' - not a drive key");
+        } else {
+            my $drive = PVE::QemuServer::Drive::parse_drive($opt, $conf->{$opt});
+            if (!$drive) {
+                warn("stop nbd: $opt: unable to parse drive '$conf->{$opt}'");
+                next;
+            }
+            if ($drive->{ro}) {
+                # The node below throttle was opened as writeable to be able to serve as a target
+                # NBD export for mirror. Re-open as read-only now. Failure is not too important,
+                # because the guest sees the drive as read-only anways, because the top throttle
+                # node is.
+                my $reopen_opts = { 'no-throttle' => 1 };
+                eval { PVE::QemuServer::Blockdev::reopen($storecfg, $vmid, $drive, $reopen_opts); };
+                print "drive-$opt failed to re-apply read-only flag below throttle - $@\n" if $@;
+            }
+        }
+    }
+
+    return;
+}
+
 1;
-- 
2.47.3