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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks for the reviews.

On 2020/11/11 4:11, Michal Privoznik wrote:
> 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@xxxxxxxxxx>
>> Reviewed-by: Chuan Zheng <zhengchuan@xxxxxxxxxx>
>> ---
>>   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?

It's indeed not necessary. I'll remove it.
> 
>> +    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
> 
> .





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux