Re: [PATCH v4 6/7] migration/dirtyrate: Implement qemuDomainGetDirtyRateInfo

10 Nov 2020

On 11/7/20 10:41 AM, Hao Wang wrote:
...
Implement qemuDomainGetDirtyRateInfo:
using flags to control behaviors -- calculate and/or query dirtyrate.
Signed-off-by: Hao Wang <wanghao232@huawei.com>
Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>
---
  include/libvirt/libvirt-domain.h | 11 ++++++
  src/qemu/qemu_driver.c           | 68 ++++++++++++++++++++++++++++++++
  2 files changed, 79 insertions(+)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 51d8685086..fc45f42dcf 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -5096,6 +5096,17 @@ int virDomainBackupBegin(virDomainPtr domain,
  char *virDomainBackupGetXMLDesc(virDomainPtr domain,
                                  unsigned int flags);
+/**
+ * virDomainDirtyRateFlags:
+ *
+ * Details on the flags used by getdirtyrate api.
+ */
+
+typedef enum {
+    VIR_DOMAIN_DIRTYRATE_CALC = 1 << 0,  /* calculate domain's dirtyrate */
+    VIR_DOMAIN_DIRTYRATE_QUERY = 1 << 1, /* query domain's dirtyrate */
+} virDomainDirtyRateFlags;
+
  /**
   * virDomainDirtyRateStatus:
   *
Again, doesn't belong here.
...
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index cb56fbbfcf..93d5a23630 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20121,6 +20121,73 @@ qemuDomainAgentSetResponseTimeout(virDomainPtr dom,
  }
+#define MIN_DIRTYRATE_CALCULATION_PERIOD    1  /* supported min dirtyrate calc time: 1s */
+#define MAX_DIRTYRATE_CALCULATION_PERIOD    60 /* supported max dirtyrate calc time: 60s */
+
+static int
+qemuDomainGetDirtyRateInfo(virDomainPtr dom,
+                           virDomainDirtyRateInfoPtr info,
+                           long long sec,
+                           unsigned int flags)
+{
+    virDomainObjPtr vm = NULL;
+    virQEMUDriverPtr driver = dom->conn->privateData;
+    int ret = -1;
+
+    if (!(vm = qemuDomainObjFromDomain(dom)))
+        return ret;
+
+    if (virDomainGetDirtyRateInfoEnsureACL(dom->conn, vm->def) < 0)
+        goto cleanup;
+
+    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
+        goto cleanup;
+
+    if (!qemuMigrationSrcIsAllowed(driver, vm, false, 0))
+        goto endjob;
+
Why is this check needed? I don't understand it, can you please explain?
...
+    if (flags & VIR_DOMAIN_DIRTYRATE_CALC) {
+        if (sec < MIN_DIRTYRATE_CALCULATION_PERIOD || sec > MAX_DIRTYRATE_CALCULATION_PERIOD) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("seconds=%lld is invalid, please choose value within [1, 60]."),
+                           sec);
+            goto endjob;
+        }
+
+        if (qemuDomainCalculateDirtyRate(dom, vm, sec) < 0) {
+            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+                           _("can't calculate domain's dirty rate"));
This overwrites a more accurate error reported by 
qemuDomainCalculateDirtyRate().
...
+            goto endjob;
+        }
+    }
+
+    if (flags & VIR_DOMAIN_DIRTYRATE_QUERY) {
+        if (flags & VIR_DOMAIN_DIRTYRATE_CALC) {
+            struct timespec ts = { .tv_sec = sec, .tv_nsec = 50 * 1000 * 1000ull };
+
+            virObjectUnlock(vm);
+            nanosleep(&ts, NULL);
+            virObjectLock(vm);
At first I was afraid, I was petrified that this waits for 50 seconds, 
until I realized it's nanosleep(). Perhaps g_usleep(50*1000); would look 
better?
...
+        }
+
+        if (qemuDomainQueryDirtyRate(dom, vm, info) < 0) {
+            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+                           _("can't query domain's dirty rate"));
Again, error overwrite.
...
+            goto endjob;
+        }
+    }
So if no flag is specified then nothing happens? I know you handle that 
in virsh, but I think that logic should live here. And perhaps the 
public API description should document that no flags is equal to 
specifying both flags together.
...
+
+    ret = 0;
+
+ endjob:
+    qemuDomainObjEndJob(driver, vm);
+
+ cleanup:
+    virDomainObjEndAPI(&vm);
+    return ret;
+}
+
+
  static virHypervisorDriver qemuHypervisorDriver = {
      .name = QEMU_DRIVER_NAME,
      .connectURIProbe = qemuConnectURIProbe,
@@ -20360,6 +20427,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
      .domainAgentSetResponseTimeout = qemuDomainAgentSetResponseTimeout, /* 5.10.0 */
      .domainBackupBegin = qemuDomainBackupBegin, /* 6.0.0 */
      .domainBackupGetXMLDesc = qemuDomainBackupGetXMLDesc, /* 6.0.0 */
+    .domainGetDirtyRateInfo = qemuDomainGetDirtyRateInfo, /* 6.9.0 */
6.10.0 :-(
...
};
I think the following should be squashed in:

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4cbbe8dd84..4058fb487c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20130,12 +20130,12 @@ qemuDomainGetDirtyRateInfo(virDomainPtr dom,
                             long long sec,
                             unsigned int flags)
  {
-    virDomainObjPtr vm = NULL;
      virQEMUDriverPtr driver = dom->conn->privateData;
+    virDomainObjPtr vm = NULL;
      int ret = -1;

      if (!(vm = qemuDomainObjFromDomain(dom)))
-        return ret;
+        return -1;

      if (virDomainGetDirtyRateInfoEnsureACL(dom->conn, vm->def) < 0)
          goto cleanup;
@@ -20147,18 +20147,18 @@ qemuDomainGetDirtyRateInfo(virDomainPtr dom,
          goto endjob;

      if (flags & VIR_DOMAIN_DIRTYRATE_CALC) {
-        if (sec < MIN_DIRTYRATE_CALCULATION_PERIOD || sec > 
MAX_DIRTYRATE_CALCULATION_PERIOD) {
+        if (sec < MIN_DIRTYRATE_CALCULATION_PERIOD ||
+            sec > MAX_DIRTYRATE_CALCULATION_PERIOD) {
              virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("seconds=%lld is invalid, please choose 
value within [1, 60]."),
-                           sec);
+                           _("seconds=%lld is invalid, please choose 
value within [%d, %d]."),
+                           sec,
+                           MIN_DIRTYRATE_CALCULATION_PERIOD,
+                           MAX_DIRTYRATE_CALCULATION_PERIOD);
              goto endjob;
          }

-        if (qemuDomainCalculateDirtyRate(dom, vm, sec) < 0) {
-            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-                           _("can't calculate domain's dirty rate"));
+        if (qemuDomainCalculateDirtyRate(driver, vm, sec) < 0)
              goto endjob;
-        }
      }

      if (flags & VIR_DOMAIN_DIRTYRATE_QUERY) {
@@ -20170,11 +20170,8 @@ qemuDomainGetDirtyRateInfo(virDomainPtr dom,
              virObjectLock(vm);
          }

-        if (qemuDomainQueryDirtyRate(dom, vm, info) < 0) {
-            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-                           _("can't query domain's dirty rate"));
+        if (qemuDomainQueryDirtyRate(driver, vm, info) < 0)
              goto endjob;
-        }
      }

      ret = 0;
@@ -20427,7 +20424,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
      .domainAgentSetResponseTimeout = 
qemuDomainAgentSetResponseTimeout, /* 5.10.0 */
      .domainBackupBegin = qemuDomainBackupBegin, /* 6.0.0 */
      .domainBackupGetXMLDesc = qemuDomainBackupGetXMLDesc, /* 6.0.0 */
-    .domainGetDirtyRateInfo = qemuDomainGetDirtyRateInfo, /* 6.9.0 */
+    .domainGetDirtyRateInfo = qemuDomainGetDirtyRateInfo, /* 6.10.0 */
  };


Michal

    

Re: [PATCH v4 6/7] migration/dirtyrate: Implement qemuDomainGetDirtyRateInfo

Michal Privoznik