New subject: [libvirt] [PATCH 01/23] Change function signature for creating new lock manager instances

9 Aug 2012

On 08/09/2012 09:20 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> The previously introduced virFile{Lock,Unlock} APIs provide a
> way to acquire/release fcntl() locks on individual files. For
> unknown reason though, the POSIX spec says that fcntl() locks
> are released when *any* file handle refering to the same path

s/refering/referring/

> is closed. In the following sequence
> 
>   threadA: fd1 = open("foo")
>   threadB: fd2 = open("foo")
>   threadA: virFileLock(fd1)
>   threadB: virFileLock(fd2)
>   threadB: close(fd2)
> 
> you'd expect threadA to come out holding a lock on 'foo', and
> indeed it does hold a lock for a very short time. Unfortunately
> when threadB does close(fd2) this releases the lock associated
> with fd1. For the current libvirt use case for virFileLock -
> pidfiles - this doesn't matter since the lock is acquired
> at startup while single threaded an never released until
> exit.
> 
> To provide a more generally useful API though, it is neccessary

s/neccessary/necessary/

> to introduce a slightly higher level abstraction, which is to
> be referred to as a "lockspace".  This is to be provided by
> a virLockSpacePtr object in src/util/virlockspace.{c,h}. The
> core idea is that the lockspace keeps track of what files are
> already open+locked. This means that when a 2nd thread comes
> along and tries to acquire a lock, it doesn't end up opening
> and closing a new FD. The lockspace just checks the current
> list of held locks and immediately returns VIR_ERR_RESOURCE_BUSY.
> 
> NB, the API as it stands is designed on the basis that the
> files being locked are not being otherwise opened and used
> by the application code. One approach to using this API is to
> acquire locks based on a hash of the filepath.
> 
> eg to lock /var/lib/libvirt/images/foo.img the application
> might do
> 
>    virLockSpacePtr lockspace = virLockSpaceNew("/var/lib/libvirt/imagelocks");
>    lockname = md5sum("/var/lib/libvirt/images/foo.img")
>    virLockSpaceAcquireLock(lockspace, lockname)

s/)$/),/g

Don't you want to ensure that the canonical name is used (that is,
symlinks can allow two different names to resolve to the same file, but
you want to hash the name without symlinks).

> 
> It is also possible to do locks directly on resources by
> using a NULL lockspace directory and then using the file
> path as the lock name eg
> 
>    virLockSpacePtr lockspace = virLockSpaceNew(NULL)
>    virLockSpaceAcquireLock(lockspace, "/var/lib/libvirt/images/foo.img")

more missing semicolons

> 
> This is only safe todo though if no other part of the proces

s/todo/to do/; s/proces/process/

> will be opening the files. This will be the case when this
> code is used inside the soon-to-be-reposted virlockd daemon
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---

This patch failed to apply directly for me; you'll have to rebase it
again.  I was able to figure out the changes, though (a new error in
virterror.[ch]).

> +
> +
> +static char *virLockSpaceGetResourcePath(virLockSpacePtr lockspace,
> +                                         const char *resname)
> +{
> +    char *ret;
> +    if (lockspace->dir) {
> +        if (virAsprintf(&ret, "%s/%s", lockspace->dir, resname) < 0) {
> +            virReportOOMError();
> +            return NULL;
> +        }
> +    } else {
> +        if (!(ret = strdup(resname))) {
> +            virReportOOMError();
> +            return NULL;
> +        }
> +    }

Fine as-is, but could be written shorter as:

if (lockspace->dir)
    ignore_value(virAsprintf(&ret, "%s/%s", lockspace->dir, resname));
else
    ret = strdup(resname);
if (!ret) {
    virReportOOMError();
    return NULL;
}

> +static void virLockSpaceResourceFree(virLockSpaceResourcePtr res)
> +{
> +    if (!res)
> +        return;
> +
> +    if (res->lockHeld &&
> +        (res->flags & VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE)) {
> +        if (res->flags & VIR_LOCK_SPACE_ACQUIRE_SHARED) {
> +            /* We must upgrade to an exclusive lock to ensure
> +             * no one else still has it before trying to delete */
> +            if (virFileLock(res->fd, false, 0, 1) < 0) {
> +                VIR_DEBUG("Could not upgrade shared lease to exclusive, not deleting");
> +            } else {
> +                unlink(res->path);

Should we log failures to unlink()?

> +static virLockSpaceResourcePtr
> +virLockSpaceResourceNew(virLockSpacePtr lockspace,

> +                VIR_DEBUG("Resource '%s' disappeared: %s",
> +                          res->path, virStrerror(errno, ebuf, sizeof(ebuf)));
> +                VIR_FORCE_CLOSE(res->fd);
> +                /* Someone else must be racing with us, so try agin */

s/agin/again/

> +
> +void virLockSpaceFree(virLockSpacePtr lockspace)
> +{
 +    if (!lockspace)
> +        return;
> +
> +    virHashFree(lockspace->resources);
> +    VIR_FREE(lockspace->dir);

Should we first try to rmdir() the directory, and silently ignore errors
related to the directory still being full?

> +++ b/tests/virlockspacetest.c

> +
> +static int testLockSpaceCreate(const void *args ATTRIBUTE_UNUSED)
> +{
> +    virLockSpacePtr lockspace;
> +    int ret = -1;
> +
> +    rmdir(LOCKSPACE_DIR);

No checks for errors?

> +
> +    lockspace = virLockSpaceNew(LOCKSPACE_DIR);
> +
> +    if (!virFileIsDir(LOCKSPACE_DIR))
> +        goto cleanup;

Although this will still fail if we ignored an earlier failure to
rmdir() a non-directory, so I guess you're okay for test purposes.

ACK with nits addressed.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[libvirt] [PATCH 00/23] Introduce a virtlockd daemon for disk locking

tags

participants (2)