Re: [libvirt] Use flock() instead of fcntl()

Am Donnerstag, 25. Juli 2013, 11:29:37 schrieb Daniel P. Berrange: It's true that flock() doesn't support locking of ranges but I can't see how this is necessary. I attached a patch with extends virtlockd so it can either use flock() *or* fcntl(). Configurable in the configuration file. From: David Weber <wb(a)munzinger.de&gt; Date: Thu, 25 Jul 2013 14:10:23 +0200 Subject: [PATCH] Add flock() support for locking Configurable in the virtlockd config --- src/locking/libvirt_lockd.aug | 1 + src/locking/lock_daemon_dispatch.c | 6 ++- src/locking/lock_driver_lockd.c | 14 ++++++- src/locking/lock_protocol.x | 3 +- src/locking/lockd.conf | 7 ++++ src/util/virfile.c | 59 +++++++++++++++++++++++++++- src/util/virfile.h | 4 ++ src/util/virlockspace.c | 80 ++++++++++++++++++++++++++++---------- src/util/virlockspace.h | 1 + 9 files changed, 148 insertions(+), 27 deletions(-) diff --git a/src/locking/libvirt_lockd.aug b/src/locking/libvirt_lockd.aug index 6a3bcba..7df715f 100644 --- a/src/locking/libvirt_lockd.aug +++ b/src/locking/libvirt_lockd.aug @@ -19,6 +19,7 @@ module Libvirt_lockd = (* Each enty in the config is one of the following three ... *) let entry = bool_entry "auto_disk_leases" | bool_entry "require_lease_for_disks" + | bool_entry "useFlock" | str_entry "file_lockspace_dir" | str_entry "lvm_lockspace_dir" | str_entry "scsi_lockspace_dir" diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c index c2e1000..930018c 100644 --- a/src/locking/lock_daemon_dispatch.c +++ b/src/locking/lock_daemon_dispatch.c @@ -50,7 +50,8 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU virMutexLock(&priv->lock); virCheckFlagsGoto(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED | - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE, cleanup); + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE | + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_USE_FLOCK, cleanup); if (priv->restricted) { virReportError(VIR_ERR_OPERATION_DENIED, "%s", @@ -76,7 +77,8 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU newFlags |= VIR_LOCK_SPACE_ACQUIRE_SHARED; if (flags & VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE) newFlags |= VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE; - + if (flags & VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_USE_FLOCK) + newFlags |= VIR_LOCK_SPACE_ACQUIRE_FLOCK; if (virLockSpaceAcquireResource(lockspace, args->name, priv->ownerPid, diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index b115799..1e07162 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -71,6 +71,7 @@ struct _virLockManagerLockDaemonPrivate { struct _virLockManagerLockDaemonDriver { bool autoDiskLease; bool requireLeaseForDisks; + bool useFlock; char *fileLockSpaceDir; char *lvmLockSpaceDir; @@ -125,6 +126,10 @@ static int virLockManagerLockDaemonLoadConfig(const char *configFile) CHECK_TYPE("auto_disk_leases", VIR_CONF_LONG); if (p) driver->autoDiskLease = p->l; + p = virConfGetValue(conf, "use_flock"); + CHECK_TYPE("use_flock", VIR_CONF_LONG); + if (p) driver->useFlock = p->l; + p = virConfGetValue(conf, "file_lockspace_dir"); CHECK_TYPE("file_lockspace_dir", VIR_CONF_STRING); if (p && p->str) { @@ -379,6 +384,7 @@ static int virLockManagerLockDaemonInit(unsigned int version, driver->requireLeaseForDisks = true; driver->autoDiskLease = true; + driver->useFlock = false; if (virLockManagerLockDaemonLoadConfig(configFile) < 0) goto error; @@ -724,6 +730,8 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, args.name = priv->resources[i].name; args.flags = priv->resources[i].flags; + if (driver->useFlock) + args.flags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_USE_FLOCK; if (virNetClientProgramCall(program, client, counter++, @@ -780,10 +788,12 @@ static int virLockManagerLockDaemonRelease(virLockManagerPtr lock, args.name = priv->resources[i].name; args.flags = priv->resources[i].flags; - args.flags &= - ~(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED | + args.flags &=~(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED | VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE); + if (driver->useFlock) + args.flags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_USE_FLOCK; + if (virNetClientProgramCall(program, client, counter++, diff --git a/src/locking/lock_protocol.x b/src/locking/lock_protocol.x index 354d51a..7828674 100644 --- a/src/locking/lock_protocol.x +++ b/src/locking/lock_protocol.x @@ -52,7 +52,8 @@ struct virLockSpaceProtocolDeleteResourceArgs { enum virLockSpaceProtocolAcquireResourceFlags { VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED = 1, - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE = 2 + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE = 2, + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_USE_FLOCK = 4 }; struct virLockSpaceProtocolAcquireResourceArgs { diff --git a/src/locking/lockd.conf b/src/locking/lockd.conf index 85edb91..944c808 100644 --- a/src/locking/lockd.conf +++ b/src/locking/lockd.conf @@ -17,6 +17,13 @@ # #require_lease_for_disks = 1 +# +# The default lockd behaviour is to acquire locks via their +# the fnctl(2) syscall. This isn't aviable on all filesystems. +# In this case you can use this flag to use flock(2) instead. +# +#use_flock = 0 + # # The default lockd behaviour is to use the "direct" diff --git a/src/util/virfile.c b/src/util/virfile.c index 8f0eec3..56900f0 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -378,7 +378,7 @@ int virFileLock(int fd, bool shared, off_t start, off_t len) * @start: byte offset to start unlock * @len: length of lock (0 to release entire remaining file from @start) * - * Release a lock previously acquired with virFileUnlock(). + * Release a lock previously acquired with virFileLock(). * NB the lock will also be released if any open file descriptor * pointing to the same file as @fd is closed * @@ -398,6 +398,63 @@ int virFileUnlock(int fd, off_t start, off_t len) return 0; } + +/** + * virFileLockFlock: + * @fd: file descriptor to acquire the lock on + * @shared: type of lock to acquire + * + * Attempt to acquire a lock on the file @fd. If @shared + * is true, then a shared lock will be acquired, + * otherwise an exclusive lock will be acquired. If + * the lock cannot be acquired, an error will be + * returned. This will not wait to acquire the lock if + * another process already holds it. + * + * The lock will be released when @fd is closed. The lock + * will also be released if *any* other open file descriptor + * pointing to the same underlying file is closed. As such + * this function should not be relied on in multi-threaded + * apps where other threads can be opening/closing arbitrary + * files. + * + * Returns 0 on success, or -errno otherwise + */ +int virFileLockFlock(int fd, bool shared) +{ + if (shared) { + if (flock(fd, LOCK_SH | LOCK_NB) < 0) + return -errno; + } else { + if (flock(fd, LOCK_EX | LOCK_NB) < 0) + return -errno; + } + + return 0; +} + + +/** + * virFileUnlockFlock: + * @fd: file descriptor to release the lock on + * @start: byte offset to start unlock + * + * Release a lock previously acquired with virFileLockFlock(). + * NB the lock will also be released if any open file descriptor + * pointing to the same file as @fd is closed + * + * Returns 0 on succcess, or -errno on error + */ +int virFileUnlockFlock(int fd) +{ + if (flock(fd, LOCK_UN) < 0) + return -errno; + + return 0; +} + + + #else int virFileLock(int fd ATTRIBUTE_UNUSED, bool shared ATTRIBUTE_UNUSED, diff --git a/src/util/virfile.h b/src/util/virfile.h index 72d35ce..e8ced5c 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -100,6 +100,10 @@ void virFileWrapperFdFree(virFileWrapperFdPtr dfd); int virFileLock(int fd, bool shared, off_t start, off_t len); int virFileUnlock(int fd, off_t start, off_t len); +int virFileLockFlock(int fd, bool shared); +int virFileUnlockFlock(int fd); + + typedef int (*virFileRewriteFunc)(int fd, void *opaque); int virFileRewrite(const char *path, mode_t mode, diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c index afb1abb..ef95672 100644 --- a/src/util/virlockspace.c +++ b/src/util/virlockspace.c @@ -154,17 +154,32 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace, goto error; } - if (virFileLock(res->fd, shared, 0, 1) < 0) { - if (errno == EACCES || errno == EAGAIN) { - virReportError(VIR_ERR_RESOURCE_BUSY, - _("Lockspace resource '%s' is locked"), - resname); - } else { - virReportSystemError(errno, - _("Unable to acquire lock on '%s'"), - res->path); + if (flags & VIR_LOCK_SPACE_ACQUIRE_FLOCK) { + if (virFileLockFlock(res->fd, shared) < 0) { + if (errno == EACCES || errno == EAGAIN) { + virReportError(VIR_ERR_RESOURCE_BUSY, + _("Lockspace resource '%s' is locked"), + resname); + } else { + virReportSystemError(errno, + _("Unable to acquire lock on '%s'"), + res->path); + } + goto error; + } + } else { + if (virFileLock(res->fd, shared, 0, 1) < 0) { + if (errno == EACCES || errno == EAGAIN) { + virReportError(VIR_ERR_RESOURCE_BUSY, + _("Lockspace resource '%s' is locked"), + resname); + } else { + virReportSystemError(errno, + _("Unable to acquire lock on '%s'"), + res->path); + } + goto error; } - goto error; } /* Now make sure the pidfile we locked is the same @@ -201,17 +216,32 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace, goto error; } - if (virFileLock(res->fd, shared, 0, 1) < 0) { - if (errno == EACCES || errno == EAGAIN) { - virReportError(VIR_ERR_RESOURCE_BUSY, - _("Lockspace resource '%s' is locked"), - resname); - } else { - virReportSystemError(errno, - _("Unable to acquire lock on '%s'"), - res->path); + if (flags & VIR_LOCK_SPACE_ACQUIRE_FLOCK) { + if (virFileLockFlock(res->fd, shared) < 0) { + if (errno == EACCES || errno == EAGAIN) { + virReportError(VIR_ERR_RESOURCE_BUSY, + _("Lockspace resource '%s' is locked"), + resname); + } else { + virReportSystemError(errno, + _("Unable to acquire lock on '%s'"), + res->path); + } + goto error; + } + } else { + if (virFileLock(res->fd, shared, 0, 1) < 0) { + if (errno == EACCES || errno == EAGAIN) { + virReportError(VIR_ERR_RESOURCE_BUSY, + _("Lockspace resource '%s' is locked"), + resname); + } else { + virReportSystemError(errno, + _("Unable to acquire lock on '%s'"), + res->path); + } + goto error; } - goto error; } } res->lockHeld = true; @@ -616,9 +646,17 @@ int virLockSpaceAcquireResource(virLockSpacePtr lockspace, VIR_DEBUG("lockspace=%p resname=%s flags=%x owner=%lld", lockspace, resname, flags, (unsigned long long)owner); + + if (flags & VIR_LOCK_SPACE_ACQUIRE_SHARED) + VIR_DEBUG("David: shared"); + if (flags & VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) + VIR_DEBUG("David: autocreate"); + if (flags & VIR_LOCK_SPACE_ACQUIRE_FLOCK) + VIR_DEBUG("David: flock"); virCheckFlags(VIR_LOCK_SPACE_ACQUIRE_SHARED | - VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE, -1); + VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE | + VIR_LOCK_SPACE_ACQUIRE_FLOCK, -1); virMutexLock(&lockspace->lock); diff --git a/src/util/virlockspace.h b/src/util/virlockspace.h index 041cf20..a6ba3dd 100644 --- a/src/util/virlockspace.h +++ b/src/util/virlockspace.h @@ -45,6 +45,7 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace, typedef enum { VIR_LOCK_SPACE_ACQUIRE_SHARED = (1 << 0), VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE = (1 << 1), + VIR_LOCK_SPACE_ACQUIRE_FLOCK = (1 << 2), } virLockSpaceAcquireFlags; int virLockSpaceAcquireResource(virLockSpacePtr lockspace, -- 1.8.1.2