[libvirt] An implementation bug in qemuMigrationWaitForCompletion that introduces an unnecessary sleep of 50 ms when a migration completes
Hi, It seems that I found a bug in the qemuMigrationWaitForCompletion(). It will do a sleep of 50 ms, even when qemuMigrationUpdateJobStatus() detects that a migration has completed. Here is the original implementation of the while loop. while (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) { /* Poll every 50ms for progress & to allow cancellation */ struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull }; if (qemuMigrationUpdateJobStatus(driver, vm, job, asyncJob) == -1) break; /* cancel migration if disk I/O error is emitted while migrating */ if (abort_on_error && virDomainObjGetState(vm, &pauseReason) == VIR_DOMAIN_PAUSED && pauseReason == VIR_DOMAIN_PAUSED_IOERROR) { virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"), job, _("failed due to I/O error")); break; } if (dconn && virConnectIsAlive(dconn) <= 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Lost connection to destination host")); break; } virObjectUnlock(vm); nanosleep(&ts, NULL); virObjectLock(vm); } Even when qemuMigrationUpdateJobStatus() detects a migration has completed, the nanosleep() will be called again. When the while condition is checked, then the program breaks from the while loop. This introduces an unnecessary sleep of 50 ms which can be avoided by doing a sleep first. The code should likely be structured as following. while (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) { /* Poll every 50ms for progress & to allow cancellation */ struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull }; // do sleep here. virObjectUnlock(vm); nanosleep(&ts, NULL); virObjectLock(vm); if (qemuMigrationUpdateJobStatus(driver, vm, job, asyncJob) == -1) break; /* cancel migration if disk I/O error is emitted while migrating */ if (abort_on_error && virDomainObjGetState(vm, &pauseReason) == VIR_DOMAIN_PAUSED && pauseReason == VIR_DOMAIN_PAUSED_IOERROR) { virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"), job, _("failed due to I/O error")); break; } if (dconn && virConnectIsAlive(dconn) <= 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Lost connection to destination host")); break; } } I have filed a bug report at redhat bugzilla, with a bug id 1210502. The link is below. https://bugzilla.redhat.com/show_bug.cgi?id=1210502 I also prepared a patch for fixing it. It is attached. Any comments?
On 10.04.2015 00:32, Xing Lin wrote:
Hi,
It seems that I found a bug in the qemuMigrationWaitForCompletion(). It will do a sleep of 50 ms, even when qemuMigrationUpdateJobStatus() detects that a migration has completed.
Here is the original implementation of the while loop.
while (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) { /* Poll every 50ms for progress & to allow cancellation */ struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull };
if (qemuMigrationUpdateJobStatus(driver, vm, job, asyncJob) == -1) break;
/* cancel migration if disk I/O error is emitted while migrating */ if (abort_on_error && virDomainObjGetState(vm, &pauseReason) == VIR_DOMAIN_PAUSED && pauseReason == VIR_DOMAIN_PAUSED_IOERROR) { virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"), job, _("failed due to I/O error")); break; }
if (dconn && virConnectIsAlive(dconn) <= 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Lost connection to destination host")); break; }
virObjectUnlock(vm);
nanosleep(&ts, NULL);
virObjectLock(vm); }
Even when qemuMigrationUpdateJobStatus() detects a migration has completed, the nanosleep() will be called again. When the while condition is checked, then the program breaks from the while loop. This introduces an unnecessary sleep of 50 ms which can be avoided by doing a sleep first. The code should likely be structured as following.
while (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) { /* Poll every 50ms for progress & to allow cancellation */ struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull };
// do sleep here. virObjectUnlock(vm);
nanosleep(&ts, NULL);
virObjectLock(vm);
Yeah, I guess it's very unlikely that migration will complete within first 50ms after issuing the command on the monitor (in which case we would again wait pointlessly). The other approach would be to leave the sleep() where it is, but enclose it in if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) {}. I have no preference over these two versions though. So ACK to the patch of yours. However, I'll postpone merging the patch for a while and let others express their feelings. Michal
On Fri, Apr 10, 2015 at 09:49:43 +0200, Michal Privoznik wrote:
On 10.04.2015 00:32, Xing Lin wrote:
Hi,
It seems that I found a bug in the qemuMigrationWaitForCompletion(). It will do a sleep of 50 ms, even when qemuMigrationUpdateJobStatus() detects that a migration has completed.
Here is the original implementation of the while loop.
while (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) { /* Poll every 50ms for progress & to allow cancellation */ struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull };
if (qemuMigrationUpdateJobStatus(driver, vm, job, asyncJob) == -1) break;
/* cancel migration if disk I/O error is emitted while migrating */ if (abort_on_error && virDomainObjGetState(vm, &pauseReason) == VIR_DOMAIN_PAUSED && pauseReason == VIR_DOMAIN_PAUSED_IOERROR) { virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"), job, _("failed due to I/O error")); break; }
if (dconn && virConnectIsAlive(dconn) <= 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Lost connection to destination host")); break; }
virObjectUnlock(vm);
nanosleep(&ts, NULL);
virObjectLock(vm); }
Even when qemuMigrationUpdateJobStatus() detects a migration has completed, the nanosleep() will be called again. When the while condition is checked, then the program breaks from the while loop. This introduces an unnecessary sleep of 50 ms which can be avoided by doing a sleep first. The code should likely be structured as following.
while (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) { /* Poll every 50ms for progress & to allow cancellation */ struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull };
// do sleep here. virObjectUnlock(vm);
nanosleep(&ts, NULL);
virObjectLock(vm);
Yeah, I guess it's very unlikely that migration will complete within first 50ms after issuing the command on the monitor (in which case we would again wait pointlessly). The other approach would be to leave the sleep() where it is, but enclose it in if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) {}. I have no preference over these two versions though. So ACK to the patch of yours. However, I'll postpone merging the patch for a while and let others express their feelings.
Or change to loop to while (1) and add a break before the sleep if jobInfo->type is not VIR_DOMAIN_JOB_UNBOUNDED. Personally, I don't mind either way. The ultimate solution is to avoid any sleeps by waiting for an event. However, Juan did not implement the even in QEMU yet. Once he does that, I will make patches for libvirt that will use that instead of polling every 50ms. Jirka
I was thinking since a migration won't finish in 50 ms, we are going to do at least one sleep anyway. So, I just moved the sleep around. I agree that the ideal approach is to wait for migration completion event notification from Qemu, rather than polling every 50 ms. -Xing On Fri, Apr 10, 2015 at 2:02 AM, Jiri Denemark <jdenemar@redhat.com> wrote:
On Fri, Apr 10, 2015 at 09:49:43 +0200, Michal Privoznik wrote:
On 10.04.2015 00:32, Xing Lin wrote:
Hi,
It seems that I found a bug in the qemuMigrationWaitForCompletion(). It will do a sleep of 50 ms, even when qemuMigrationUpdateJobStatus() detects that a migration has completed.
Here is the original implementation of the while loop.
while (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) { /* Poll every 50ms for progress & to allow cancellation */ struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull };
if (qemuMigrationUpdateJobStatus(driver, vm, job, asyncJob) == -1) break;
/* cancel migration if disk I/O error is emitted while migrating */ if (abort_on_error && virDomainObjGetState(vm, &pauseReason) == VIR_DOMAIN_PAUSED && pauseReason == VIR_DOMAIN_PAUSED_IOERROR) { virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"), job, _("failed due to I/O error")); break; }
if (dconn && virConnectIsAlive(dconn) <= 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Lost connection to destination host")); break; }
virObjectUnlock(vm);
nanosleep(&ts, NULL);
virObjectLock(vm); }
Even when qemuMigrationUpdateJobStatus() detects a migration has completed, the nanosleep() will be called again. When the while condition is checked, then the program breaks from the while loop. This introduces an unnecessary sleep of 50 ms which can be avoided by doing a sleep first. The code should likely be structured as following.
while (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) { /* Poll every 50ms for progress & to allow cancellation */ struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull };
// do sleep here. virObjectUnlock(vm);
nanosleep(&ts, NULL);
virObjectLock(vm);
Yeah, I guess it's very unlikely that migration will complete within first 50ms after issuing the command on the monitor (in which case we would again wait pointlessly). The other approach would be to leave the sleep() where it is, but enclose it in if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) {}. I have no preference over these two versions though. So ACK to the patch of yours. However, I'll postpone merging the patch for a while and let others express their feelings.
Or change to loop to while (1) and add a break before the sleep if jobInfo->type is not VIR_DOMAIN_JOB_UNBOUNDED.
Personally, I don't mind either way. The ultimate solution is to avoid any sleeps by waiting for an event. However, Juan did not implement the even in QEMU yet. Once he does that, I will make patches for libvirt that will use that instead of polling every 50ms.
Jirka
On 10.04.2015 09:49, Michal Privoznik wrote:
On 10.04.2015 00:32, Xing Lin wrote:
<snip/>
Yeah, I guess it's very unlikely that migration will complete within first 50ms after issuing the command on the monitor (in which case we would again wait pointlessly). The other approach would be to leave the sleep() where it is, but enclose it in if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) {}. I have no preference over these two versions though. So ACK to the patch of yours. However, I'll postpone merging the patch for a while and let others express their feelings.
As promised, pushed now. Michal
participants (3)
-
Jiri Denemark -
Michal Privoznik -
Xing Lin