[PATCH-SERIES qemu-server v2 0/3] small block job related improvements

[PATCH-SERIES qemu-server v2 0/3] small block job related improvements

[Fiona Ebner]

Changes in v2:
* add patch to improve name of block job monitor function
* add path to avoid potentially misleading error messages

See the individual patches for more information.

qemu-server:

Fiona Ebner (3):
  blockdev: commit: improve comment about completion mode
  block job: rename qemu_drive_mirror_monitor() to monitor()
  blockdev: avoid potentially misleading error messages when block job
    monitor fails

 src/PVE/QemuMigrate.pm                    |  2 +-
 src/PVE/QemuServer.pm                     | 12 +++-------
 src/PVE/QemuServer/BlockJob.pm            |  8 +++----
 src/PVE/QemuServer/Blockdev.pm            | 28 +++++++++--------------
 src/test/MigrationTest/QemuMigrateMock.pm |  8 +++----
 src/test/run_qemu_migrate_tests.pl        |  4 ++--
 6 files changed, 24 insertions(+), 38 deletions(-)


Summary over all repositories:
  6 files changed, 24 insertions(+), 38 deletions(-)

-- 
Generated by git-murpp 0.5.0

[PATCH qemu-server v2 1/3] blockdev: commit: improve comment about completion mode

[Fiona Ebner]

For reference, see the QMP docs [0][1].

[0]: https://qemu.readthedocs.io/en/master/interop/qemu-qmp-ref.html#command-QMP-block-core.block-commit
[1]: https://qemu.readthedocs.io/en/master/interop/qemu-qmp-ref.html#command-QMP-job.job-complete

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

diff --git a/src/PVE/QemuServer/Blockdev.pm b/src/PVE/QemuServer/Blockdev.pm
index af7e769b..8eb01b45 100644
--- a/src/PVE/QemuServer/Blockdev.pm
+++ b/src/PVE/QemuServer/Blockdev.pm
@@ -1074,7 +1074,15 @@ sub blockdev_commit {
         mon_cmd($vmid, "block-commit", %$opts);
         $jobs->{$job_id} = {};
 
-        # if we commit the current, the blockjob need to be in 'complete' mode
+        # If the 'current' state is committed to its backing snapshot, the job will not complete
+        # automatically, because there is a writer, i.e. the guest. It is necessary to use the
+        # 'complete' completion mode, so that the 'current' block node is replaced with the backing
+        # node upon completion. Like that, IO after the commit operation will already land in the
+        # backing node, which will be renamed since it will be the new top of the chain (done by the
+        # caller).
+        #
+        # For other snapshots in the chain, it can be assumed that they have no writer, so
+        # 'block-commit' will complete automatically.
         my $complete = $src_snap && $src_snap ne 'current' ? 'auto' : 'complete';
 
         eval {
-- 
2.47.3

[PATCH qemu-server v2 2/3] block job: rename qemu_drive_mirror_monitor() to monitor()

[Fiona Ebner]

The function is not only used for mirror jobs, but also stream and
commit jobs. The fact that it is for block jobs is already clear from
the module name.

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

New in v2.

 src/PVE/QemuMigrate.pm                    |  2 +-
 src/PVE/QemuServer.pm                     | 12 +++---------
 src/PVE/QemuServer/BlockJob.pm            |  8 +++-----
 src/PVE/QemuServer/Blockdev.pm            | 10 ++--------
 src/test/MigrationTest/QemuMigrateMock.pm |  8 ++++----
 src/test/run_qemu_migrate_tests.pl        |  4 ++--
 6 files changed, 15 insertions(+), 29 deletions(-)

diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
index 5ecfc5ef..f7ec3227 100644
--- a/src/PVE/QemuMigrate.pm
+++ b/src/PVE/QemuMigrate.pm
@@ -1558,7 +1558,7 @@ sub phase2 {
         # to avoid it trying to re-establish it. We are in blockjob ready state,
         # thus, this command changes to it to blockjob complete (see qapi docs)
         eval {
-            PVE::QemuServer::BlockJob::qemu_drive_mirror_monitor(
+            PVE::QemuServer::BlockJob::monitor(
                 $vmid, undef, $self->{storage_migration_jobs}, 'cancel',
             );
         };
diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 5d2dbe03..39723c82 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -7294,9 +7294,7 @@ sub pbs_live_restore {
         }
 
         mon_cmd($vmid, 'cont');
-        PVE::QemuServer::BlockJob::qemu_drive_mirror_monitor(
-            $vmid, undef, $jobs, 'auto', 0, 'stream',
-        );
+        PVE::QemuServer::BlockJob::monitor($vmid, undef, $jobs, 'auto', 0, 'stream');
 
         print "restore-drive jobs finished successfully, removing all tracking block devices"
             . " to disconnect from Proxmox Backup Server\n";
@@ -7415,9 +7413,7 @@ sub live_import_from_files {
         }
 
         mon_cmd($vmid, 'cont');
-        PVE::QemuServer::BlockJob::qemu_drive_mirror_monitor(
-            $vmid, undef, $jobs, 'auto', 0, 'stream',
-        );
+        PVE::QemuServer::BlockJob::monitor($vmid, undef, $jobs, 'auto', 0, 'stream');
 
         print "restore-drive jobs finished successfully, removing all tracking block devices\n";
 
@@ -7925,9 +7921,7 @@ sub clone_disk {
             # if this is the case, we have to complete any block-jobs still there from
             # previous drive-mirrors
             if (($completion && $completion eq 'complete') && (scalar(keys %$jobs) > 0)) {
-                PVE::QemuServer::BlockJob::qemu_drive_mirror_monitor(
-                    $vmid, $newvmid, $jobs, $completion, $qga,
-                );
+                PVE::QemuServer::BlockJob::monitor($vmid, $newvmid, $jobs, $completion, $qga);
             }
             goto no_data_clone;
         }
diff --git a/src/PVE/QemuServer/BlockJob.pm b/src/PVE/QemuServer/BlockJob.pm
index 0b22765b..02e21146 100644
--- a/src/PVE/QemuServer/BlockJob.pm
+++ b/src/PVE/QemuServer/BlockJob.pm
@@ -83,7 +83,7 @@ sub qemu_blockjobs_cancel {
 # 'cancel': wait until all jobs are ready, block-job-cancel them
 # 'skip': wait until all jobs are ready, return with block jobs in ready state
 # 'auto': wait until all jobs disappear, only use for jobs which complete automatically
-sub qemu_drive_mirror_monitor {
+sub monitor {
     my ($vmid, $vmiddst, $jobs, $completion, $qga, $op) = @_;
 
     $completion //= 'complete';
@@ -310,7 +310,7 @@ sub qemu_drive_mirror {
         die "mirroring error: $err\n";
     }
 
-    qemu_drive_mirror_monitor($vmid, $vmiddst, $jobs, $completion, $qga);
+    monitor($vmid, $vmiddst, $jobs, $completion, $qga);
 }
 
 # Callers should version guard this (only available with a binary >= QEMU 8.2)
@@ -506,9 +506,7 @@ sub blockdev_mirror {
         log_warn("unable to delete blockdev '$target_node_name' - $@");
         die "error starting blockdev mirrror - $err";
     }
-    qemu_drive_mirror_monitor(
-        $vmid, $dest->{vmid}, $jobs, $completion, $options->{'guest-agent'}, 'mirror',
-    );
+    monitor($vmid, $dest->{vmid}, $jobs, $completion, $options->{'guest-agent'}, 'mirror');
 }
 
 sub mirror {
diff --git a/src/PVE/QemuServer/Blockdev.pm b/src/PVE/QemuServer/Blockdev.pm
index 8eb01b45..be9f588a 100644
--- a/src/PVE/QemuServer/Blockdev.pm
+++ b/src/PVE/QemuServer/Blockdev.pm
@@ -1086,9 +1086,7 @@ sub blockdev_commit {
         my $complete = $src_snap && $src_snap ne 'current' ? 'auto' : 'complete';
 
         eval {
-            PVE::QemuServer::BlockJob::qemu_drive_mirror_monitor(
-                $vmid, undef, $jobs, $complete, 0, 'commit',
-            );
+            PVE::QemuServer::BlockJob::monitor($vmid, undef, $jobs, $complete, 0, 'commit');
         };
         if ($@) {
             die "Failed to complete block commit: $@\n";
@@ -1166,11 +1164,7 @@ sub blockdev_stream {
     mon_cmd($vmid, 'block-stream', %$options);
     $jobs->{$job_id} = {};
 
-    eval {
-        PVE::QemuServer::BlockJob::qemu_drive_mirror_monitor(
-            $vmid, undef, $jobs, 'auto', 0, 'stream',
-        );
-    };
+    eval { PVE::QemuServer::BlockJob::monitor($vmid, undef, $jobs, 'auto', 0, 'stream'); };
     if ($@) {
         die "Failed to complete block stream: $@\n";
     }
diff --git a/src/test/MigrationTest/QemuMigrateMock.pm b/src/test/MigrationTest/QemuMigrateMock.pm
index 421f0bb7..8cd2da12 100644
--- a/src/test/MigrationTest/QemuMigrateMock.pm
+++ b/src/test/MigrationTest/QemuMigrateMock.pm
@@ -175,14 +175,14 @@ $qemu_server_blockjob_module->mock(
 
         common_mirror_mock($source->{vmid}, $drive_id);
     },
-    qemu_drive_mirror_monitor => sub {
+    monitor => sub {
         my ($vmid, $vmiddst, $jobs, $completion, $qga) = @_;
 
         if (
-            $fail_config->{qemu_drive_mirror_monitor}
-            && $fail_config->{qemu_drive_mirror_monitor} eq $completion
+            $fail_config->{block_job_monitor}
+            && $fail_config->{block_job_monitor} eq $completion
         ) {
-            die "qemu_drive_mirror_monitor '$completion' error\n";
+            die "block_job_monitor '$completion' error\n";
         }
         return;
     },
diff --git a/src/test/run_qemu_migrate_tests.pl b/src/test/run_qemu_migrate_tests.pl
index ed2f38ee..05eed1d9 100755
--- a/src/test/run_qemu_migrate_tests.pl
+++ b/src/test/run_qemu_migrate_tests.pl
@@ -1596,10 +1596,10 @@ my $tests = [
             unused0 => 'local-dir:149/vm-149-disk-0.qcow2',
         },
         expected_calls => {},
-        expect_die => "qemu_drive_mirror_monitor 'cancel' error",
+        expect_die => "block_job_monitor 'cancel' error",
         # note that 'cancel' is also used to finish and that's what this test is about
         fail_config => {
-            'qemu_drive_mirror_monitor' => 'cancel',
+            'block_job_monitor' => 'cancel',
         },
         expected => {
             source_volids => local_volids_for_vm(149),
-- 
2.47.3

[PATCH qemu-server v2 3/3] blockdev: avoid potentially misleading error messages when block job monitor fails

[Fiona Ebner]

The monitor() function can also fail at different times than during
completion. If it fails during completion, the error message will
already indicate that. Finally, the error messages already indicate
the job type or ID, so no information is lost by getting rid of the
added prefix.

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

New in v2.

 src/PVE/QemuServer/Blockdev.pm | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/src/PVE/QemuServer/Blockdev.pm b/src/PVE/QemuServer/Blockdev.pm
index be9f588a..5846ac69 100644
--- a/src/PVE/QemuServer/Blockdev.pm
+++ b/src/PVE/QemuServer/Blockdev.pm
@@ -1085,12 +1085,7 @@ sub blockdev_commit {
         # 'block-commit' will complete automatically.
         my $complete = $src_snap && $src_snap ne 'current' ? 'auto' : 'complete';
 
-        eval {
-            PVE::QemuServer::BlockJob::monitor($vmid, undef, $jobs, $complete, 0, 'commit');
-        };
-        if ($@) {
-            die "Failed to complete block commit: $@\n";
-        }
+        PVE::QemuServer::BlockJob::monitor($vmid, undef, $jobs, $complete, 0, 'commit');
 
         blockdev_delete(
             $storecfg, $vmid, $drive, $src_file_blockdev, $src_fmt_blockdev, $src_snap,
@@ -1164,10 +1159,7 @@ sub blockdev_stream {
     mon_cmd($vmid, 'block-stream', %$options);
     $jobs->{$job_id} = {};
 
-    eval { PVE::QemuServer::BlockJob::monitor($vmid, undef, $jobs, 'auto', 0, 'stream'); };
-    if ($@) {
-        die "Failed to complete block stream: $@\n";
-    }
+    PVE::QemuServer::BlockJob::monitor($vmid, undef, $jobs, 'auto', 0, 'stream');
 
     blockdev_delete($storecfg, $vmid, $drive, $snap_file_blockdev, $snap_fmt_blockdev, $snap);
 }
-- 
2.47.3

superseded: [PATCH-SERIES qemu-server v2 0/3] small block job related improvements

[Fiona Ebner]

superseded by:
https://lore.proxmox.com/pve-devel/20260213165307.177055-1-f.ebner@proxmox.com/T/