src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 6 ++++++ src/qemu/qemu_conf.c | 8 ++++++++ src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_domain.c | 5 +---- src/qemu/test_libvirtd_qemu.aug.in | 1 + 6 files changed, 19 insertions(+), 4 deletions(-)
When doing some job holding state lock for a long time,
we may come across error:
"Timed out during operation: cannot acquire state change lock"
Well, sometimes it's not a problem and users wanner continue
to wait, and this patch allow users decide how long time they
can wait the state lock.
Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
Reviewed-by: Xi Xu <xu.xi8@zte.com.cn>
---
changes in v2:
- change default value to 30 in qemu.conf
- set the default value in virQEMUDriverConfigNew()
---
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf | 6 ++++++
src/qemu/qemu_conf.c | 8 ++++++++
src/qemu/qemu_conf.h | 2 ++
src/qemu/qemu_domain.c | 5 +----
src/qemu/test_libvirtd_qemu.aug.in | 1 +
6 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index ddc4bbf..f7287ae 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -93,6 +93,7 @@ module Libvirtd_qemu =
| limits_entry "max_core"
| bool_entry "dump_guest_core"
| str_entry "stdio_handler"
+ | int_entry "state_lock_timeout"
let device_entry = bool_entry "mac_filter"
| bool_entry "relaxed_acs_check"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index cd57b3c..4284786 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -813,3 +813,9 @@
#
#swtpm_user = "tss"
#swtpm_group = "tss"
+
+# The timeout (in seconds) waiting for acquiring state lock.
+#
+# Default is 30
+#
+#state_lock_timeout = 60
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index a4f545e..31e0013 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -129,6 +129,9 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def)
#endif
+/* Give up waiting for mutex after 30 seconds */
+#define QEMU_JOB_WAIT_TIME (1000ull * 30)
+
virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
{
virQEMUDriverConfigPtr cfg;
@@ -346,6 +349,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
cfg->glusterDebugLevel = 4;
cfg->stdioLogD = true;
+ cfg->stateLockTimeout = QEMU_JOB_WAIT_TIME;
+
if (!(cfg->namespaces = virBitmapNew(QEMU_DOMAIN_NS_LAST)))
goto error;
@@ -863,6 +868,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
if (virConfGetValueUInt(conf, "keepalive_count", &cfg->keepAliveCount) < 0)
goto cleanup;
+ if (virConfGetValueInt(conf, "state_lock_timeout", &cfg->stateLockTimeout) < 0)
+ goto cleanup;
+
if (virConfGetValueInt(conf, "seccomp_sandbox", &cfg->seccompSandbox) < 0)
goto cleanup;
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index a8d84ef..97cf2e1 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -190,6 +190,8 @@ struct _virQEMUDriverConfig {
int keepAliveInterval;
unsigned int keepAliveCount;
+ int stateLockTimeout;
+
int seccompSandbox;
char *migrateHost;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 886e3fb..5a2ca52 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6652,9 +6652,6 @@ qemuDomainObjCanSetJob(qemuDomainObjPrivatePtr priv,
priv->job.agentActive == QEMU_AGENT_JOB_NONE));
}
-/* Give up waiting for mutex after 30 seconds */
-#define QEMU_JOB_WAIT_TIME (1000ull * 30)
-
/**
* qemuDomainObjBeginJobInternal:
* @driver: qemu driver
@@ -6714,7 +6711,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
}
priv->jobs_queued++;
- then = now + QEMU_JOB_WAIT_TIME;
+ then = now + cfg->stateLockTimeout;
retry:
if ((!async && job != QEMU_JOB_DESTROY) &&
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index f1e8806..dc5de96 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -105,3 +105,4 @@ module Test_libvirtd_qemu =
{ "pr_helper" = "/usr/bin/qemu-pr-helper" }
{ "swtpm_user" = "tss" }
{ "swtpm_group" = "tss" }
+{ "state_lock_timeout" = "60" }
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 08/27/2018 12:57 PM, Yi Wang wrote:
> When doing some job holding state lock for a long time,
> we may come across error:
> "Timed out during operation: cannot acquire state change lock"
> Well, sometimes it's not a problem and users wanner continue
> to wait, and this patch allow users decide how long time they
> can wait the state lock.
>
> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> Reviewed-by: Xi Xu <xu.xi8@zte.com.cn>
> ---
> changes in v2:
> - change default value to 30 in qemu.conf
> - set the default value in virQEMUDriverConfigNew()
> ---
> src/qemu/libvirtd_qemu.aug | 1 +
> src/qemu/qemu.conf | 6 ++++++
> src/qemu/qemu_conf.c | 8 ++++++++
> src/qemu/qemu_conf.h | 2 ++
> src/qemu/qemu_domain.c | 5 +----
> src/qemu/test_libvirtd_qemu.aug.in | 1 +
> 6 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> index ddc4bbf..f7287ae 100644
> --- a/src/qemu/libvirtd_qemu.aug
> +++ b/src/qemu/libvirtd_qemu.aug
> @@ -93,6 +93,7 @@ module Libvirtd_qemu =
> | limits_entry "max_core"
> | bool_entry "dump_guest_core"
> | str_entry "stdio_handler"
> + | int_entry "state_lock_timeout"
>
> let device_entry = bool_entry "mac_filter"
> | bool_entry "relaxed_acs_check"
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index cd57b3c..4284786 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -813,3 +813,9 @@
> #
> #swtpm_user = "tss"
> #swtpm_group = "tss"
> +
> +# The timeout (in seconds) waiting for acquiring state lock.
This is rather sparse description. I know that state change lock is,
because I'm a libvirt devel. However, I don't expect our users to know
that. How about adding the following description:
When two or more threads want to work with the same domain they use a
job lock to mutually exclude each other. However, waiting for the lock
is limited up to state_lock_timeout seconds.
Also, could you move this close to max_queued variable since they both
refer to the same area?
> +#
> +# Default is 30
> +#
> +#state_lock_timeout = 60
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index a4f545e..31e0013 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -129,6 +129,9 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def)
> #endif
>
>
> +/* Give up waiting for mutex after 30 seconds */
> +#define QEMU_JOB_WAIT_TIME (1000ull * 30)
> +
> virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
> {
> virQEMUDriverConfigPtr cfg;
> @@ -346,6 +349,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
> cfg->glusterDebugLevel = 4;
> cfg->stdioLogD = true;
>
> + cfg->stateLockTimeout = QEMU_JOB_WAIT_TIME;
> +
> if (!(cfg->namespaces = virBitmapNew(QEMU_DOMAIN_NS_LAST)))
> goto error;
>
> @@ -863,6 +868,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
> if (virConfGetValueUInt(conf, "keepalive_count", &cfg->keepAliveCount) < 0)
> goto cleanup;
>
> + if (virConfGetValueInt(conf, "state_lock_timeout", &cfg->stateLockTimeout) < 0)
> + goto cleanup;
> +
Almost, you need to check if cfg->stateLockTimeout is not zero. Such
code could go into virQEMUDriverConfigValidate().
Otherwise looking good.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 08/27/2018 09:29 AM, Michal Prívozník wrote:
> On 08/27/2018 12:57 PM, Yi Wang wrote:
>> When doing some job holding state lock for a long time,
>> we may come across error:
>> "Timed out during operation: cannot acquire state change lock"
>> Well, sometimes it's not a problem and users wanner continue
s/wanner/want to/
or "prefer to"... Perhaps users is too vague - this essentially sets
the "default timeout policy" for everyone. Setting it too short could be
dangerous considering it's impact.
If "a user" wanted or preferred to wait longer than the default job
timeout for a specific command, then we'd need to add some option for
various commands/API's that allowed setting a unique timeout value for a
specific job.
Another thought would be to have an API that allowed changing the
timeout programmatically. Of course there's the admin interface that is
another area to consider altering since you're adjusting a configuration
value like this.
John
>> to wait, and this patch allow users decide how long time they
>> can wait the state lock.
>>
>> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
>> Reviewed-by: Xi Xu <xu.xi8@zte.com.cn>
>> ---
>> changes in v2:
>> - change default value to 30 in qemu.conf
>> - set the default value in virQEMUDriverConfigNew()
>> ---
>> src/qemu/libvirtd_qemu.aug | 1 +
>> src/qemu/qemu.conf | 6 ++++++
>> src/qemu/qemu_conf.c | 8 ++++++++
>> src/qemu/qemu_conf.h | 2 ++
>> src/qemu/qemu_domain.c | 5 +----
>> src/qemu/test_libvirtd_qemu.aug.in | 1 +
>> 6 files changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
>> index ddc4bbf..f7287ae 100644
>> --- a/src/qemu/libvirtd_qemu.aug
>> +++ b/src/qemu/libvirtd_qemu.aug
>> @@ -93,6 +93,7 @@ module Libvirtd_qemu =
>> | limits_entry "max_core"
>> | bool_entry "dump_guest_core"
>> | str_entry "stdio_handler"
>> + | int_entry "state_lock_timeout"
>>
>> let device_entry = bool_entry "mac_filter"
>> | bool_entry "relaxed_acs_check"
>> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
>> index cd57b3c..4284786 100644
>> --- a/src/qemu/qemu.conf
>> +++ b/src/qemu/qemu.conf
>> @@ -813,3 +813,9 @@
>> #
>> #swtpm_user = "tss"
>> #swtpm_group = "tss"
>> +
>> +# The timeout (in seconds) waiting for acquiring state lock.
>
> This is rather sparse description. I know that state change lock is,
> because I'm a libvirt devel. However, I don't expect our users to know
> that. How about adding the following description:
>
> When two or more threads want to work with the same domain they use a
> job lock to mutually exclude each other. However, waiting for the lock
> is limited up to state_lock_timeout seconds.
>
> Also, could you move this close to max_queued variable since they both
> refer to the same area?
>
>> +#
>> +# Default is 30
>> +#
>> +#state_lock_timeout = 60
>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>> index a4f545e..31e0013 100644
>> --- a/src/qemu/qemu_conf.c
>> +++ b/src/qemu/qemu_conf.c
>> @@ -129,6 +129,9 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def)
>> #endif
>>
>>
>> +/* Give up waiting for mutex after 30 seconds */
>> +#define QEMU_JOB_WAIT_TIME (1000ull * 30)
>> +
>> virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
>> {
>> virQEMUDriverConfigPtr cfg;
>> @@ -346,6 +349,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
>> cfg->glusterDebugLevel = 4;
>> cfg->stdioLogD = true;
>>
>> + cfg->stateLockTimeout = QEMU_JOB_WAIT_TIME;
>> +
>> if (!(cfg->namespaces = virBitmapNew(QEMU_DOMAIN_NS_LAST)))
>> goto error;
>>
>> @@ -863,6 +868,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
>> if (virConfGetValueUInt(conf, "keepalive_count", &cfg->keepAliveCount) < 0)
>> goto cleanup;
>>
>> + if (virConfGetValueInt(conf, "state_lock_timeout", &cfg->stateLockTimeout) < 0)
>> + goto cleanup;
>> +
>
> Almost, you need to check if cfg->stateLockTimeout is not zero. Such
> code could go into virQEMUDriverConfigValidate().
>
> Otherwise looking good.
>
> Michal
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 08/27/2018 04:29 PM, John Ferlan wrote: > > > On 08/27/2018 09:29 AM, Michal Prívozník wrote: >> On 08/27/2018 12:57 PM, Yi Wang wrote: >>> When doing some job holding state lock for a long time, >>> we may come across error: >>> "Timed out during operation: cannot acquire state change lock" >>> Well, sometimes it's not a problem and users wanner continue > > s/wanner/want to/ > > or "prefer to"... Perhaps users is too vague - this essentially sets > the "default timeout policy" for everyone. Setting it too short could be > dangerous considering it's impact. > > If "a user" wanted or preferred to wait longer than the default job > timeout for a specific command, then we'd need to add some option for > various commands/API's that allowed setting a unique timeout value for a > specific job. > > Another thought would be to have an API that allowed changing the > timeout programmatically. Of course there's the admin interface that is > another area to consider altering since you're adjusting a configuration > value like this. If anything, this is a job for virt-admin. I don't think we need an API that would live next to virDomainDeviceUpdate() for instance. We don't have one for max_jobs, nor for lock_manager, ... In fact, I don't think we have any API to change anything from qemu.conf. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
> On 08/27/2018 09:29 AM, Michal Prívozník wrote: > > On 08/27/2018 12:57 PM, Yi Wang wrote: > >> When doing some job holding state lock for a long time, > >> we may come across error: > >> "Timed out during operation: cannot acquire state change lock" > >> Well, sometimes it's not a problem and users wanner continue > > s/wanner/want to/ > > or "prefer to"... Perhaps users is too vague - this essentially sets > the "default timeout policy" for everyone. Setting it too short could be > dangerous considering it's impact. That's true, maybe I need add a NB in qemu.conf? --- Best wishes Yi Wang-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
> On 08/27/2018 12:57 PM, Yi Wang wrote: > > When doing some job holding state lock for a long time, > > we may come across error: > > #swtpm_user = "tss" > > #swtpm_group = "tss" > > + > > +# The timeout (in seconds) waiting for acquiring state lock. > > This is rather sparse description. I know that state change lock is, > because I'm a libvirt devel. However, I don't expect our users to know > that. How about adding the following description: > > When two or more threads want to work with the same domain they use a > job lock to mutually exclude each other. However, waiting for the lock > is limited up to state_lock_timeout seconds. That's much better, thanks. > > Also, could you move this close to max_queued variable since they both > refer to the same area? Of course, I will adjust it. > > + if (virConfGetValueInt(conf, "state_lock_timeout", &cfg->stateLockTimeout) < 0) > > + goto cleanup; > > + > > Almost, you need to check if cfg->stateLockTimeout is not zero. Such > code could go into virQEMUDriverConfigValidate(). Thanks again for your time and patience, Michal. --- Best wishes Yi Wang-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.