[libvirt] [PATCH v2] locking: Add io_timeout to sanlock

[Jiri Denemark]

On Tue, Oct 27, 2015 at 16:29:51 +0100, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1251190
> 
> So, if domain loses access to storage, sanlock tries to kill it
> after some timeout. So far, the default is 80 seconds. But for
> some scenarios this might not be enough. We should allow users to
> adjust the timeout according to their needs.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
> 
> diff to v2:
> - Check if the new sanlock API is accessible. If not, forbid setting timeout in
>   the config file.
> 
>  m4/virt-sanlock.m4                      |  7 +++++++
>  src/locking/libvirt_sanlock.aug         |  1 +
>  src/locking/lock_driver_sanlock.c       | 15 +++++++++++++++
>  src/locking/sanlock.conf                |  7 +++++++
>  src/locking/test_libvirt_sanlock.aug.in |  1 +
>  5 files changed, 31 insertions(+)
> 
> diff --git a/m4/virt-sanlock.m4 b/m4/virt-sanlock.m4
> index c7c0186..d2a607d 100644
> --- a/m4/virt-sanlock.m4
> +++ b/m4/virt-sanlock.m4
> @@ -46,6 +46,13 @@ AC_DEFUN([LIBVIRT_CHECK_SANLOCK],[
>          [whether sanlock supports sanlock_inq_lockspace])
>      fi
>  
> +    AC_CHECK_LIB([sanlock_client], [sanlock_add_lockspace_timeout],
> +                 [sanlock_add_lockspace_timeout=yes], [sanlock_add_lockspace_timeout=no])
> +    if test "x$sanlock_add_lockspace_timeout" = "xyes" ; then
> +      AC_DEFINE_UNQUOTED([HAVE_SANLOCK_ADD_LOCKSPACE_TIMEOUT], 1,
> +        [whether Sanlock supports sanlock_add_lockspace_timeout])
> +    fi
> +
>      CPPFLAGS="$old_cppflags"
>      LIBS="$old_libs"
>    fi
> diff --git a/src/locking/libvirt_sanlock.aug b/src/locking/libvirt_sanlock.aug
> index a78a444..8843590 100644
> --- a/src/locking/libvirt_sanlock.aug
> +++ b/src/locking/libvirt_sanlock.aug
> @@ -22,6 +22,7 @@ module Libvirt_sanlock =
>               | int_entry "host_id"
>               | bool_entry "require_lease_for_disks"
>               | bool_entry "ignore_readonly_and_shared_disks"
> +             | int_entry "io_timeout"
>               | str_entry "user"
>               | str_entry "group"
>     let comment = [ label "#comment" . del /#[ \t]*/ "# " .  store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ]
> diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c
> index e052875..dbda915 100644
> --- a/src/locking/lock_driver_sanlock.c
> +++ b/src/locking/lock_driver_sanlock.c
> @@ -73,6 +73,7 @@ struct _virLockManagerSanlockDriver {
>      int hostID;
>      bool autoDiskLease;
>      char *autoDiskLeasePath;
> +    unsigned int io_timeout;
>  
>      /* under which permissions does sanlock run */
>      uid_t user;
> @@ -151,6 +152,10 @@ static int virLockManagerSanlockLoadConfig(const char *configFile)
>      else
>          driver->requireLeaseForDisks = !driver->autoDiskLease;
>  
> +    p = virConfGetValue(conf, "io_timeout");
> +    CHECK_TYPE("io_timeout", VIR_CONF_ULONG);
> +    if (p) driver->io_timeout = p->l;
> +
>      p = virConfGetValue(conf, "user");
>      CHECK_TYPE("user", VIR_CONF_STRING);
>      if (p) {
> @@ -338,7 +343,16 @@ static int virLockManagerSanlockSetupLockspace(void)
>       * or we can fallback to polling.
>       */
>   retry:
> +#ifdef HAVE_SANLOCK_ADD_LOCKSPACE_TIMEOUT
> +    if ((rv = sanlock_add_lockspace_timeout(&ls, 0, driver->io_timeout)) < 0) {
> +#else
> +    if (driver->io_timeout) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("unable to use io_timeout with this version of sanlock"));
> +        goto error;
> +    }
>      if ((rv = sanlock_add_lockspace(&ls, 0)) < 0) {
> +#endif

Ouch, please, don't mix #if and if blocks. The following would be much
better:

    #ifdef HAVE_SANLOCK_ADD_LOCKSPACE_TIMEOUT
        rv = sanlock_add_lockspace_timeout(&ls, 0, driver->io_timeout);
    #else
        if (driver->io_timeout) {
            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                           _("unable to use io_timeout with this version of sanlock"));
            goto error;
        }
        rv = sanlock_add_lockspace(&ls, 0);
    #endif

    if (rv < 0) {

Jirka