Global variables are bad, we should avoid using them. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/locking/lock_driver_sanlock.c | 42 ++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 579f696..1ff1abd 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -80,7 +80,7 @@ struct _virLockManagerSanlockDriver { gid_t group; }; -static virLockManagerSanlockDriver *driver; +static virLockManagerSanlockDriverPtr sanlockDriver; struct _virLockManagerSanlockPrivate { const char *vm_uri; @@ -100,7 +100,8 @@ struct _virLockManagerSanlockPrivate { /* * sanlock plugin for the libvirt virLockManager API */ -static int virLockManagerSanlockLoadConfig(const char *configFile) +static int virLockManagerSanlockLoadConfig(virLockManagerSanlockDriverPtr driver, + const char *configFile) { virConfPtr conf; int ret = -1; @@ -161,7 +162,8 @@ static int virLockManagerSanlockLoadConfig(const char *configFile) /* How many times try adding a lockspace? */ #define LOCKSPACE_RETRIES 10 -static int virLockManagerSanlockSetupLockspace(void) +static int +virLockManagerSanlockSetupLockspace(virLockManagerSanlockDriverPtr driver) { int fd = -1; struct stat st; @@ -371,16 +373,20 @@ static int virLockManagerSanlockInit(unsigned int version, const char *configFile, unsigned int flags) { + virLockManagerSanlockDriverPtr driver; + VIR_DEBUG("version=%u configFile=%s flags=%x", version, NULLSTR(configFile), flags); virCheckFlags(0, -1); - if (driver) + if (sanlockDriver) return 0; - if (VIR_ALLOC(driver) < 0) + if (VIR_ALLOC(sanlockDriver) < 0) return -1; + driver = sanlockDriver; + driver->requireLeaseForDisks = true; driver->hostID = 0; driver->autoDiskLease = false; @@ -392,7 +398,7 @@ static int virLockManagerSanlockInit(unsigned int version, goto error; } - if (virLockManagerSanlockLoadConfig(configFile) < 0) + if (virLockManagerSanlockLoadConfig(driver, configFile) < 0) goto error; if (driver->autoDiskLease && !driver->hostID) { @@ -402,7 +408,7 @@ static int virLockManagerSanlockInit(unsigned int version, } if (driver->autoDiskLease) { - if (virLockManagerSanlockSetupLockspace() < 0) + if (virLockManagerSanlockSetupLockspace(driver) < -1) goto error; } @@ -415,11 +421,11 @@ static int virLockManagerSanlockInit(unsigned int version, static int virLockManagerSanlockDeinit(void) { - if (!driver) + if (!sanlockDriver) return 0; - VIR_FREE(driver->autoDiskLeasePath); - VIR_FREE(driver); + VIR_FREE(sanlockDriver->autoDiskLeasePath); + VIR_FREE(sanlockDriver); return 0; } @@ -438,7 +444,7 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock, virCheckFlags(VIR_LOCK_MANAGER_NEW_STARTED, -1); - if (!driver) { + if (!sanlockDriver) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Sanlock plugin is not initialized")); return -1; @@ -566,7 +572,8 @@ static int virLockManagerSanlockAddLease(virLockManagerPtr lock, -static int virLockManagerSanlockAddDisk(virLockManagerPtr lock, +static int virLockManagerSanlockAddDisk(virLockManagerSanlockDriverPtr driver, + virLockManagerPtr lock, const char *name, size_t nparams, virLockManagerParamPtr params ATTRIBUTE_UNUSED, @@ -631,7 +638,8 @@ static int virLockManagerSanlockAddDisk(virLockManagerPtr lock, } -static int virLockManagerSanlockCreateLease(struct sanlk_resource *res) +static int virLockManagerSanlockCreateLease(virLockManagerSanlockDriverPtr driver, + struct sanlk_resource *res) { int fd = -1; struct stat st; @@ -719,6 +727,7 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, virLockManagerParamPtr params, unsigned int flags) { + virLockManagerSanlockDriverPtr driver = sanlockDriver; virLockManagerSanlockPrivatePtr priv = lock->privateData; virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY | @@ -738,11 +747,12 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, switch (type) { case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: if (driver->autoDiskLease) { - if (virLockManagerSanlockAddDisk(lock, name, nparams, params, + if (virLockManagerSanlockAddDisk(driver, lock, name, nparams, params, !!(flags & VIR_LOCK_MANAGER_RESOURCE_SHARED)) < 0) return -1; - if (virLockManagerSanlockCreateLease(priv->res_args[priv->res_count-1]) < 0) + if (virLockManagerSanlockCreateLease(driver, + priv->res_args[priv->res_count-1]) < 0) return -1; } else { if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED | @@ -882,7 +892,7 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock, if (priv->res_count == 0 && priv->hasRWDisks && - driver->requireLeaseForDisks) { + sanlockDriver->requireLeaseForDisks) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Read/write, exclusive access, disks were present, but no leases specified")); return -1; -- 2.8.4

Currently, we are checking for sanlock_add_lockspace_timeout which is good for now. But in the next patches we are gonna use sanlock_write_lockspace (which sets an initial value for io timeout for sanlock). Now, there is no reason to check for both functions in sanlock library as the sanlock_write_lockspace was introduced in 2.7 release and the one we are currently checking for in the 2.5 release. Therefore it is safe to assume presence of sanlock_add_lockspace_timeout when sanlock_write_lockspace is detected. Moreover, the macro for conditional compilation is renamed to HAVE_SANLOCK_IO_TIMEOUT (as it now encapsulates two functions). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- m4/virt-sanlock.m4 | 14 +++++++++----- src/locking/lock_driver_sanlock.c | 2 +- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/m4/virt-sanlock.m4 b/m4/virt-sanlock.m4 index d2a607d..3c30cbf 100644 --- a/m4/virt-sanlock.m4 +++ b/m4/virt-sanlock.m4 @@ -46,11 +46,15 @@ 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]) + dnl Ideally, we would check for sanlock_add_lockspace_timeout here too, but + dnl sanlock_write_lockspace has been introduced 2 releases after + dnl sanlock_add_lockspace_timeout therefore if sanlock_write_lockspace is found + dnl it is safe to assume sanlock_add_lockspace_timeout is present too. + AC_CHECK_LIB([sanlock_client], [sanlock_write_lockspace], + [sanlock_write_lockspace=yes], [sanlock_write_lockspace=no]) + if test "x$sanlock_write_lockspace" = "xyes" ; then + AC_DEFINE_UNQUOTED([HAVE_SANLOCK_IO_TIMEOUT], 1, + [whether sanlock supports sanlock_write_lockspace]) fi CPPFLAGS="$old_cppflags" diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 1ff1abd..f09027f 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -310,7 +310,7 @@ virLockManagerSanlockSetupLockspace(virLockManagerSanlockDriverPtr driver) * or we can fallback to polling. */ retry: -#ifdef HAVE_SANLOCK_ADD_LOCKSPACE_TIMEOUT +#ifdef HAVE_SANLOCK_IO_TIMEOUT rv = sanlock_add_lockspace_timeout(&ls, 0, driver->io_timeout); #else if (driver->io_timeout) { -- 2.8.4

https://bugzilla.redhat.com/show_bug.cgi?id=1292984 Hold on to your hats, because this is gonna be wild. In bd3e16a3 I've tried to expose sanlock io_timeout. What I had not realized (because there is like no documentation for sanlock at all) was very unusual way their APIs work. Basically, what we do currently is: sanlock_add_lockspace_timeout(&ls, io_timeout); which adds a lockspace to sanlock daemon. One would expect that io_timeout sets the io_timeout for it. Nah! That's where you are completely off the tracks. It sets timeout for next lockspace you will probably add later. Therefore: sanlock_add_lockspace_timeout(&ls, io_timeout = 10); /* adds new lockspace with default io_timeout */ sanlock_add_lockspace_timeout(&ls, io_timeout = 20); /* adds new lockspace with io_timeout = 10 */ sanlock_add_lockspace_timeout(&ls, io_timeout = 40); /* adds new lockspace with io_timeout = 20 */ And so on. You get the picture. Fortunately, we don't allow setting io_timeout per domain or per domain disk. So we just need to set the default used in the very first step and hope for the best (as all the io_timeout-s used later will have the same value). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/locking/lock_driver_sanlock.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index f09027f..ee1ee67 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -157,6 +157,29 @@ static int virLockManagerSanlockLoadConfig(virLockManagerSanlockDriverPtr driver return ret; } +static int +virLockManagerSanlockInitLockspace(virLockManagerSanlockDriverPtr driver, + struct sanlk_lockspace *ls) +{ + int ret; + +#ifdef HAVE_SANLOCK_IO_TIMEOUT + const int max_hosts = 0; /* defaults used in sanlock_init() implementation */ + const unsigned int lockspaceFlags = 0; + + ret = sanlock_write_lockspace(ls, max_hosts, lockspaceFlags, driver->io_timeout); +#else + if (driver->io_timeout) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("unable to use io_timeout with this version of sanlock")); + return -ENOTSUP; + } + + ret = sanlock_init(ls, NULL, 0, 0); +#endif + return ret; +} + /* How much ms sleep before retrying to add a lockspace? */ #define LOCKSPACE_SLEEP 100 /* How many times try adding a lockspace? */ @@ -267,7 +290,7 @@ virLockManagerSanlockSetupLockspace(virLockManagerSanlockDriverPtr driver) goto error_unlink; } - if ((rv = sanlock_init(&ls, NULL, 0, 0)) < 0) { + if ((rv = virLockManagerSanlockInitLockspace(driver, &ls) < 0)) { if (rv <= -200) virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to initialize lockspace %s: error %d"), -- 2.8.4