On Mon, Nov 22, 2010 at 06:09:21PM +0000, Daniel P. Berrange wrote:
> +/*
> + * Flags to pass to 'load_drv' and also 'new_drv' method
> + * Plugins must support at least one of the modes. If a
> + * mode is unsupported, it must return an error
> + */
> +enum {
> + VIR_LOCK_MANAGER_MODE_CONTENT = (1 << 0),
> + VIR_LOCK_MANAGER_MODE_METADATA = (1 << 1),
> +} virLockManagerFlags;
These terms indicate what libvirt is using the locks for, but from a lock
manager perspective, they don't have any meaning. It's not clear what the
lock manager should do differently for one type vs the other.
Is it the scope of the locks that differs? i.e. locking between local
processes on a single host, vs distributed locking between hosts? If
that's the case, then maybe name them LOCAL and DISTRIBUTED?
> +enum {
> + /* The resource to be locked is a virtual disk */
> + VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK = 0,
> +} virLockManagerResourceType;
Generic is good, and there are parts where it could still be more like a
generic, traditional lock manager API...
> +typedef enum {
> + /* The resource is assigned in readonly mode */
> + VIR_LOCK_MANAGER_RESOURCE_READONLY = (1 << 0),
> + /* The resource is assigned in shared, writable mode */
> + VIR_LOCK_MANAGER_RESOURCE_SHARED = (1 << 1),
> +} virLockManagerResourceFlags;
I really think you should adopt traditional lock modes instead:
LOCK_MODE_EXCLUSIVE (aka WRITE) - incompatible with other SH and EX locks.
LOCK_MODE_SHARED (aka READ) - compatible with SH locks, not with EX locks.
LOCK_MODE_NONE - the lock manager is unused, and it's up to the
application to do its own locking or coordination when accessing the
resource, e.g. if there's a clustered db or fs on the device that does its
own coordination. You seem to be calling this SHARED, but there's
actually no lock or exclusion that a lock manager would implement for this
kind of usage.
Each resource should have its own lock mode, and a mode should always be
specified (rather than defaulting to EX). libvirt would use the lock mode
based on how the resource will be used, it would take a SH lock on a
readonly resource.
non-blocking and force options would probably be quite useful to have
right from the beginning for testing.
> +/**
> + * virLockManagerNew:
> + * @man: the lock manager context
> + * @type: the type of process to be supervised
> + * @flags: optional flags, currently unused
> + *
> + * Initialize a new context to supervise a process, usually
> + * a virtual machine. If the lock driver requested a
> + * private data, the <code>privateData</code> field of
> + * <code>man</code> will be initialized with a suitable
> + * pointer.
> + *
> + * Returns 0 if successful initialized a new context, -1 on error
> + */
> +typedef int (*virLockDriverNew)(virLockManagerPtr man,
> + unsigned int type,
> + unsigned int flags);
This is fundamentally about creating/defining a new process or new lock
owner. A name like NewOwner or NewPid or NewVM would make that more
clear. You refer to it sometimes as a "managed object".
> +/**
> + * virLockDriverAddResource:
> + * @manager: the lock manager context
> + * @type: the resource type virLockManagerResourceType
> + * @name: the resource name
> + * @flags: the resource access flags
> + *
> + * Assign a resource to a managed object. This will
> + * only be called prior to the object is being locked
> + * when it is inactive. eg, to set the initial boot
> + * time disk assignments on a VM
> + * The format of @name varies according to
> + * the resource @type. A VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK
> + * will have a fully qualified file path.
> + *
> + * If no flags are given, the resource is assumed to be
> + * used in exclusive, read-write mode. Access can be
> + * relaxed to readonly, or shared read-write.
> + *
> + * Returns 0 on success, or -1 on failure
> + */
> +typedef int (*virLockDriverAddResource)(virLockManagerPtr man,
> + unsigned int type,
> + const char *name,
> + unsigned int flags);
As mentioned above, I think we want a lock mode arg.
> +/**
> + * virLockDriverAcquireObject:
> + * @manager: the lock manager context
> + * @state: the current lock state
> + * @flags: optional flags, currently unused
> + *
> + * Start managing resources for the object. If the
> + * object is being transferred from another location
> + * the current lock state may be passed in. This
> + * must be called from the PID that represents the
> + * object to be managed. If the lock is lost at any
> + * time, the PID will be killed off by the lock manager.
> + *
> + * Returns 0 on success, or -1 on failure
> + */
> +typedef int (*virLockDriverAcquireObject)(virLockManagerPtr man,
> + const char *state,
> + unsigned int flags);
AcquireResources?
> +/**
> + * virLockDriverAttachObject:
> + * @manager: the lock manager context
> + * @pid: the managed object PID
> + * @flags: optional flags, currently unused
> + *
> + * Re-attach to an existing lock manager instance managing
> + * PID @pid.
> + *
> + * Returns 0 on success, or -1 on failure
> + */
> +typedef int (*virLockDriverAttachObject)(virLockManagerPtr man,
> + pid_t pid,
> + unsigned int flags);
> +
> +/**
> + * virLockDriverDetachObject:
> + * @manager: the lock manager context
> + * @pid: the managed object PID
> + * @flags: optional flags, currently unused
> + *
> + * Deattach from an existing lock manager instance managing
> + * PID @pid.
> + *
> + * Returns 0 on success, or -1 on failure
> + */
> +typedef int (*virLockDriverDetachObject)(virLockManagerPtr man,
> + pid_t pid,
> + unsigned int flags);
I guess these two are to allow libvirtd to be restarted while the lock
manager remains running and holding locks? Again, Pid/Owner/VM would be
less obtuse than Object.
> +/**
> + * virLockDriverAcquireResource:
> + * @manager: the lock manager context
> + * @type: the resource type virLockDriverResourceType
> + * @name: the resource name
> + * @flags: the resource access flags
> + *
> + * Assign a resource to a managed object. This will
> + * only be called when the object is already locked
> + * and active. eg, to hotplug a disk into a VM.
> + * The format of @name varies according to
> + * the resource @type. A VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK
> + * will have a fully qualified file path.
> + *
> + * If no flags are given, the resource is assumed to be
> + * used in exclusive, read-write mode. Access can be
> + * relaxed to readonly, or shared read-write.
> + *
> + * Returns 0 on success, or -1 on failure
> + */
> +typedef int (*virLockDriverAcquireResource)(virLockManagerPtr man,
> + unsigned int type,
> + const char *name,
> + unsigned int flags);
Since I suggested AddResource+AcquireResources above, Acquire would a bit
inconsistent here; perhaps AttachResource or AssignResource?
Dave
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list