Re: [libvirt] [PATCH v2] qemu: Introduce state_lock_timeout to qemu.conf
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(a)zte.com.cn>
> Reviewed-by: Xi Xu <xu.xi8(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list