[v1] qemu: Introduce state_lock_timeout to qemu.conf

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

Posted by Yi Wang 7 years, 5 months ago

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>
---
 src/qemu/libvirtd_qemu.aug         | 1 +
 src/qemu/qemu.conf                 | 6 ++++++
 src/qemu/qemu_conf.c               | 3 +++
 src/qemu/qemu_conf.h               | 2 ++
 src/qemu/qemu_domain.c             | 6 +++++-
 src/qemu/test_libvirtd_qemu.aug.in | 1 +
 6 files changed, 18 insertions(+), 1 deletion(-)

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..e88db3f 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -813,3 +813,9 @@
 #
 #swtpm_user = "tss"
 #swtpm_group = "tss"
+
+# Override the timeout (in seconds) for acquiring state lock.
+#
+# Default is 0
+#
+#state_lock_timeout = 60
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index a4f545e..f452714 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -863,6 +863,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..d375a73 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6714,7 +6714,11 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
     }
 
     priv->jobs_queued++;
-    then = now + QEMU_JOB_WAIT_TIME;
+    if (cfg->stateLockTimeout > 0)
+        then = now + cfg->stateLockTimeout;
+    else
+        then = now + QEMU_JOB_WAIT_TIME;
+
 
  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

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

Posted by Michal Prívozník 7 years, 5 months ago

On 08/27/2018 08:04 AM, 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>
> ---
>  src/qemu/libvirtd_qemu.aug         | 1 +
>  src/qemu/qemu.conf                 | 6 ++++++
>  src/qemu/qemu_conf.c               | 3 +++
>  src/qemu/qemu_conf.h               | 2 ++
>  src/qemu/qemu_domain.c             | 6 +++++-
>  src/qemu/test_libvirtd_qemu.aug.in | 1 +
>  6 files changed, 18 insertions(+), 1 deletion(-)
> 
> 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..e88db3f 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -813,3 +813,9 @@
>  #
>  #swtpm_user = "tss"
>  #swtpm_group = "tss"
> +
> +# Override the timeout (in seconds) for acquiring state lock.
> +#
> +# Default is 0
> +#
> +#state_lock_timeout = 60

Firstly, the default value should be set for the variable. So if the
default is really 0 (which it is not) then this line should read:

#state_lock_timeout = 0

Secondly, the default is not zero, because if user sets it to zero your
code uses the hardcoded timeout of 30 seconds. Also, we should probably
reserve 0 for some special future use (no timeouts perhaps?)

> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index a4f545e..f452714 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -863,6 +863,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..d375a73 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6714,7 +6714,11 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
>      }
>  
>      priv->jobs_queued++;
> -    then = now + QEMU_JOB_WAIT_TIME;
> +    if (cfg->stateLockTimeout > 0)
> +        then = now + cfg->stateLockTimeout;
> +    else
> +        then = now + QEMU_JOB_WAIT_TIME;

You can set the default in virQEMUDriverConfigNew() so that it's always
set and simplify this line then.

> +
>  
>   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" }
> 

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by wang.yi59@zte.com.cn 7 years, 5 months ago

Thanks for your review, Michal.
I will send a v2 patch.

> On 08/27/2018 08:04 AM, 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>
> > ---
> >  src/qemu/libvirtd_qemu.aug         | 1 +
> >  src/qemu/qemu.conf                 | 6 ++++++
> >  src/qemu/qemu_conf.c               | 3 +++
> >  src/qemu/qemu_conf.h               | 2 ++
> >  src/qemu/qemu_domain.c             | 6 +++++-
> >  src/qemu/test_libvirtd_qemu.aug.in | 1 +
> >  6 files changed, 18 insertions(+), 1 deletion(-)
> >
> > 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..e88db3f 100644
> > --- a/src/qemu/qemu.conf
> > +++ b/src/qemu/qemu.conf
> > @@ -813,3 +813,9 @@
> >  #
> >  #swtpm_user = "tss"
> >  #swtpm_group = "tss"
> > +
> > +# Override the timeout (in seconds) for acquiring state lock.
> > +#
> > +# Default is 0
> > +#
> > +#state_lock_timeout = 60
>
> Firstly, the default value should be set for the variable. So if the
> default is really 0 (which it is not) then this line should read:


---
Best wishes
Yi Wang--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list