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

[Michal Privoznik]

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 at huawei.com>
> Reviewed-by: Chuan Zheng <zhengchuan at 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