Bug 1210502 - Implementation bug in qemuMigrationWaitForCompletion that introduces an unnecessary sleep of 50 ms when a migration completes
Summary: Implementation bug in qemuMigrationWaitForCompletion that introduces an unnec...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Virtualization Tools
Classification: Community
Component: libvirt
Version: unspecified
Hardware: All
OS: All
unspecified
medium
Target Milestone: ---
Assignee: Michal Privoznik
QA Contact:
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-04-09 22:20 UTC by Xing Lin
Modified: 2015-04-13 08:52 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-04-13 08:52:42 UTC


Attachments (Terms of Use)

Description Xing Lin 2015-04-09 22:20:26 UTC
Description of problem:

The current implementation of the qemuMigrationWaitForCompletion() function will sleep for 50 ms, even when a migration has already finished. This sleep is unnecessary and only adds up to the guest pause time. We would like to remove this, to reduce the VM pause time. 


Version-Release number of selected component (if applicable):

The latest version of libvirt from git has this bug, with the most recent commit at a45ef3a9cdfe7a1f847da68ab3d7313595c0bc05. 


How reproducible:

It is within the source code. 

Steps to Reproduce:
1.
2.
3.

Actual results:

An additional sleep of 50 ms is done even when a snapshot completes. 

Expected results:

We should avoid another sleep of 50 ms when a snapshot completes. 

Additional info:

Here is my patch to fix it. 

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 29f5173..af44b69 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2389,6 +2389,12 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver,
         /* Poll every 50ms for progress & to allow cancellation */
         struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull };

+        virObjectUnlock(vm);
+
+        nanosleep(&ts, NULL);
+
+        virObjectLock(vm);
+
         if (qemuMigrationUpdateJobStatus(driver, vm, job, asyncJob) == -1)
             break;

@@ -2407,11 +2413,6 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver,
             break;
         }

-        virObjectUnlock(vm);
-
-        nanosleep(&ts, NULL);
-
-        virObjectLock(vm);
     }

     if (jobInfo->type == VIR_DOMAIN_JOB_COMPLETED) {

Comment 1 Michal Privoznik 2015-04-13 08:52:42 UTC
I've just pushed the patch upstream:

commit 522e81cbb501e9772fbafba975ac886c1b4a283d
Author:     Xing Lin <xinglin@cs.utah.edu>
AuthorDate: Thu Apr 9 16:02:02 2015 -0600
Commit:     Michal Privoznik <mprivozn@redhat.com>
CommitDate: Mon Apr 13 09:52:28 2015 +0200

    qemu_migration.c: sleep first before checking for migration status.
    
    The problem with the previous implementation is,
    even when qemuMigrationUpdateJobStatus() detects a migration job
    has completed, it will do a sleep for 50 ms (which is unnecessary
    and only adds up to the VM pause time).
    
    Signed-off-by: Xing Lin <xinglin@cs.utah.edu>
    Signed-off-by: Michal Privoznik <mprivozn@redhat.com>


v1.2.14-108-g522e81c


Note You need to log in before you can comment on or make changes to this bug.