Re: [PATCH v4 6/7] migration/dirtyrate: Implement qemuDomainGetDirtyRateInfo
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
>> Reviewed-by: Chuan Zheng
>> ---
>> 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 */
>> + .domainGet
Re: [PATCH v4 6/7] migration/dirtyrate: Implement qemuDomainGetDirtyRateInfo
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
Reviewed-by: Chuan Zheng
---
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_PERIOD1 /* supported min dirtyrate calc time: 1s */
+#define MAX_DIRTYRATE_CALCULATION_PERIOD60 /* 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;
virQEMU
[PATCH v4 6/7] migration/dirtyrate: Implement qemuDomainGetDirtyRateInfo
Implement qemuDomainGetDirtyRateInfo:
using flags to control behaviors -- calculate and/or query dirtyrate.
Signed-off-by: Hao Wang
Reviewed-by: Chuan Zheng
---
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:
*
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_PERIOD1 /* supported min dirtyrate calc
time: 1s */
+#define MAX_DIRTYRATE_CALCULATION_PERIOD60 /* 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;
+
+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"));
+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);
+}
+
+if (qemuDomainQueryDirtyRate(dom, vm, info) < 0) {
+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("can't query domain's dirty rate"));
+goto endjob;
+}
+}
+
+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 */
};
--
2.23.0
