[libvirt] [PATCH v2] qemu: Introduce state_lock_timeout to qemu.conf

[Michal Prívozník]

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 at zte.com.cn>
> Reviewed-by: Xi Xu <xu.xi8 at 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