So far the virLockSpaceAcquireResource() locks the first byte in the underlying file. But caller might want to lock other range. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/locking/lock_daemon_dispatch.c | 3 +++ src/util/virlockspace.c | 15 ++++++++++----- src/util/virlockspace.h | 4 ++++ tests/virlockspacetest.c | 29 ++++++++++++++++++++++++----- 4 files changed, 41 insertions(+), 10 deletions(-) diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c index 1b479db55d..10248ec0b5 100644 --- a/src/locking/lock_daemon_dispatch.c +++ b/src/locking/lock_daemon_dispatch.c @@ -50,6 +50,8 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU virNetServerClientGetPrivateData(client); virLockSpacePtr lockspace; unsigned int newFlags; + off_t start = 0; + off_t len = 1; virMutexLock(&priv->lock); @@ -84,6 +86,7 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU if (virLockSpaceAcquireResource(lockspace, args->name, priv->ownerPid, + start, len, newFlags) < 0) goto cleanup; diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c index 3364c843aa..60bfef4c5f 100644 --- a/src/util/virlockspace.c +++ b/src/util/virlockspace.c @@ -115,8 +115,10 @@ static void virLockSpaceResourceFree(virLockSpaceResourcePtr res) static virLockSpaceResourcePtr virLockSpaceResourceNew(virLockSpacePtr lockspace, const char *resname, - unsigned int flags, - pid_t owner) + pid_t owner, + off_t start, + off_t len, + unsigned int flags) { virLockSpaceResourcePtr res; bool shared = !!(flags & VIR_LOCK_SPACE_ACQUIRE_SHARED); @@ -157,7 +159,7 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace, goto error; } - if (virFileLock(res->fd, shared, 0, 1, false) < 0) { + if (virFileLock(res->fd, shared, start, len, false) < 0) { if (errno == EACCES || errno == EAGAIN) { virReportError(VIR_ERR_RESOURCE_BUSY, _("Lockspace resource '%s' is locked"), @@ -204,7 +206,7 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace, goto error; } - if (virFileLock(res->fd, shared, 0, 1, false) < 0) { + if (virFileLock(res->fd, shared, start, len, false) < 0) { if (errno == EACCES || errno == EAGAIN) { virReportError(VIR_ERR_RESOURCE_BUSY, _("Lockspace resource '%s' is locked"), @@ -612,6 +614,8 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace, int virLockSpaceAcquireResource(virLockSpacePtr lockspace, const char *resname, pid_t owner, + off_t start, + off_t len, unsigned int flags) { int ret = -1; @@ -641,7 +645,8 @@ int virLockSpaceAcquireResource(virLockSpacePtr lockspace, goto cleanup; } - if (!(res = virLockSpaceResourceNew(lockspace, resname, flags, owner))) + if (!(res = virLockSpaceResourceNew(lockspace, resname, + owner, start, len, flags))) goto cleanup; if (virHashAddEntry(lockspace->resources, resname, res) < 0) { diff --git a/src/util/virlockspace.h b/src/util/virlockspace.h index 041cf20396..24f2c89be6 100644 --- a/src/util/virlockspace.h +++ b/src/util/virlockspace.h @@ -22,6 +22,8 @@ #ifndef __VIR_LOCK_SPACE_H__ # define __VIR_LOCK_SPACE_H__ +# include <sys/types.h> + # include "internal.h" # include "virjson.h" @@ -50,6 +52,8 @@ typedef enum { int virLockSpaceAcquireResource(virLockSpacePtr lockspace, const char *resname, pid_t owner, + off_t start, + off_t len, unsigned int flags); int virLockSpaceReleaseResource(virLockSpacePtr lockspace, diff --git a/tests/virlockspacetest.c b/tests/virlockspacetest.c index 75ad98a02c..2409809353 100644 --- a/tests/virlockspacetest.c +++ b/tests/virlockspacetest.c @@ -99,6 +99,8 @@ static int testLockSpaceResourceLockExcl(const void *args ATTRIBUTE_UNUSED) { virLockSpacePtr lockspace; int ret = -1; + const off_t start = 0; + const off_t len = 1; rmdir(LOCKSPACE_DIR); @@ -111,13 +113,13 @@ static int testLockSpaceResourceLockExcl(const void *args ATTRIBUTE_UNUSED) if (virLockSpaceCreateResource(lockspace, "foo") < 0) goto cleanup; - if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), 0) < 0) + if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), start, len, 0) < 0) goto cleanup; if (!virFileExists(LOCKSPACE_DIR "/foo")) goto cleanup; - if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), 0) == 0) + if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), start, len, 0) == 0) goto cleanup; if (virLockSpaceDeleteResource(lockspace, "foo") == 0) @@ -145,6 +147,8 @@ static int testLockSpaceResourceLockExclAuto(const void *args ATTRIBUTE_UNUSED) { virLockSpacePtr lockspace; int ret = -1; + const off_t start = 0; + const off_t len = 1; rmdir(LOCKSPACE_DIR); @@ -158,6 +162,7 @@ static int testLockSpaceResourceLockExclAuto(const void *args ATTRIBUTE_UNUSED) goto cleanup; if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), + start, len, VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) < 0) goto cleanup; @@ -183,6 +188,8 @@ static int testLockSpaceResourceLockShr(const void *args ATTRIBUTE_UNUSED) { virLockSpacePtr lockspace; int ret = -1; + const off_t start = 0; + const off_t len = 1; rmdir(LOCKSPACE_DIR); @@ -196,13 +203,16 @@ static int testLockSpaceResourceLockShr(const void *args ATTRIBUTE_UNUSED) goto cleanup; if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), + start, len, VIR_LOCK_SPACE_ACQUIRE_SHARED) < 0) goto cleanup; - if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), 0) == 0) + if (virLockSpaceAcquireResource(lockspace, "foo", + geteuid(), start, len, 0) == 0) goto cleanup; if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), + start, len, VIR_LOCK_SPACE_ACQUIRE_SHARED) < 0) goto cleanup; @@ -237,6 +247,8 @@ static int testLockSpaceResourceLockShrAuto(const void *args ATTRIBUTE_UNUSED) { virLockSpacePtr lockspace; int ret = -1; + const off_t start = 0; + const off_t len = 1; rmdir(LOCKSPACE_DIR); @@ -250,6 +262,7 @@ static int testLockSpaceResourceLockShrAuto(const void *args ATTRIBUTE_UNUSED) goto cleanup; if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), + start, len, VIR_LOCK_SPACE_ACQUIRE_SHARED | VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) < 0) goto cleanup; @@ -258,6 +271,7 @@ static int testLockSpaceResourceLockShrAuto(const void *args ATTRIBUTE_UNUSED) goto cleanup; if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), + start, len, VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) == 0) goto cleanup; @@ -265,6 +279,7 @@ static int testLockSpaceResourceLockShrAuto(const void *args ATTRIBUTE_UNUSED) goto cleanup; if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), + start, len, VIR_LOCK_SPACE_ACQUIRE_SHARED | VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) < 0) goto cleanup; @@ -297,6 +312,8 @@ static int testLockSpaceResourceLockPath(const void *args ATTRIBUTE_UNUSED) { virLockSpacePtr lockspace; int ret = -1; + const off_t start = 0; + const off_t len = 1; rmdir(LOCKSPACE_DIR); @@ -309,13 +326,15 @@ static int testLockSpaceResourceLockPath(const void *args ATTRIBUTE_UNUSED) if (virLockSpaceCreateResource(lockspace, LOCKSPACE_DIR "/foo") < 0) goto cleanup; - if (virLockSpaceAcquireResource(lockspace, LOCKSPACE_DIR "/foo", geteuid(), 0) < 0) + if (virLockSpaceAcquireResource(lockspace, LOCKSPACE_DIR "/foo", + geteuid(), start, len, 0) < 0) goto cleanup; if (!virFileExists(LOCKSPACE_DIR "/foo")) goto cleanup; - if (virLockSpaceAcquireResource(lockspace, LOCKSPACE_DIR "/foo", geteuid(), 0) == 0) + if (virLockSpaceAcquireResource(lockspace, LOCKSPACE_DIR "/foo", + geteuid(), start, len, 0) == 0) goto cleanup; if (virLockSpaceDeleteResource(lockspace, LOCKSPACE_DIR "/foo") == 0) -- 2.16.4

Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/locking/lock_driver.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 8b7cccc521..7c8f79520a 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -56,6 +56,8 @@ typedef enum { VIR_LOCK_MANAGER_RESOURCE_READONLY = (1 << 0), /* The resource is assigned in shared, writable mode */ VIR_LOCK_MANAGER_RESOURCE_SHARED = (1 << 1), + /* The resource is locked for metadata change */ + VIR_LOCK_MANAGER_RESOURCE_METADATA = (1 << 2), } virLockManagerResourceFlags; typedef enum { -- 2.16.4

On 08/17/2018 12:24 AM, John Ferlan wrote: That's the thing. You're right, it doesn't make sense for lease type. But on the level of RPC of virtlockd both leases and disks kind of blend together. I mean, virtlockd is merely told to lock this or that file. I'm gonna save that to a follow up patch. Thanks! Michal

Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/locking/lock_daemon_dispatch.c | 12 ++++++++++-- src/locking/lock_driver_lockd.c | 31 +++++++++++++++++++++---------- src/locking/lock_driver_lockd.h | 1 + 3 files changed, 32 insertions(+), 12 deletions(-) diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c index 10248ec0b5..c24571ceac 100644 --- a/src/locking/lock_daemon_dispatch.c +++ b/src/locking/lock_daemon_dispatch.c @@ -37,6 +37,9 @@ VIR_LOG_INIT("locking.lock_daemon_dispatch"); #include "lock_daemon_dispatch_stubs.h" +#define DEFAULT_OFFSET 0 +#define METADATA_OFFSET 1 + static int virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client, @@ -50,13 +53,14 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU virNetServerClientGetPrivateData(client); virLockSpacePtr lockspace; unsigned int newFlags; - off_t start = 0; + off_t start = DEFAULT_OFFSET; off_t len = 1; 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_METADATA, cleanup); if (priv->restricted) { virReportError(VIR_ERR_OPERATION_DENIED, "%s", @@ -82,6 +86,10 @@ 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_METADATA) { + start = METADATA_OFFSET; + newFlags |= VIR_LOCK_SPACE_ACQUIRE_WAIT; + } if (virLockSpaceAcquireResource(lockspace, args->name, diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 957a963a7b..bd14ed8930 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -475,9 +475,11 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, bool autoCreate = false; virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY | - VIR_LOCK_MANAGER_RESOURCE_SHARED, -1); + VIR_LOCK_MANAGER_RESOURCE_SHARED | + VIR_LOCK_MANAGER_RESOURCE_METADATA, -1); - if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY) + if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY && + !(flags & VIR_LOCK_MANAGER_RESOURCE_METADATA)) return 0; switch (type) { @@ -489,7 +491,8 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, } if (!driver->autoDiskLease) { if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED | - VIR_LOCK_MANAGER_RESOURCE_READONLY))) + VIR_LOCK_MANAGER_RESOURCE_READONLY | + VIR_LOCK_MANAGER_RESOURCE_METADATA))) priv->hasRWDisks = true; return 0; } @@ -602,6 +605,10 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, priv->resources[priv->nresources-1].flags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE; + if (flags & VIR_LOCK_MANAGER_RESOURCE_METADATA) + priv->resources[priv->nresources-1].flags |= + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_METADATA; + return 0; error: @@ -626,12 +633,15 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY | VIR_LOCK_MANAGER_ACQUIRE_RESTRICT, -1); - if (priv->nresources == 0 && - priv->hasRWDisks && - driver->requireLeaseForDisks) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Read/write, exclusive access, disks were present, but no leases specified")); - return -1; + if (priv->nresources == 0) { + if (priv->hasRWDisks && + driver->requireLeaseForDisks) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Read/write, exclusive access, disks were present, but no leases specified")); + return -1; + } + + return 0; } if (!(client = virLockManagerLockDaemonConnect(lock, &program, &counter))) @@ -711,7 +721,8 @@ static int virLockManagerLockDaemonRelease(virLockManagerPtr lock, args.flags &= ~(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED | - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE); + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE | + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_METADATA); if (virNetClientProgramCall(program, client, diff --git a/src/locking/lock_driver_lockd.h b/src/locking/lock_driver_lockd.h index 6931fe7425..9882793260 100644 --- a/src/locking/lock_driver_lockd.h +++ b/src/locking/lock_driver_lockd.h @@ -25,6 +25,7 @@ enum virLockSpaceProtocolAcquireResourceFlags { VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED = (1 << 0), VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE = (1 << 1), + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_METADATA = (1 << 2), }; #endif /* __VIR_LOCK_DRIVER_LOCKD_H__ */ -- 2.16.4

On 08/17/2018 01:53 AM, John Ferlan wrote: Yes, this is exactly the purpose. I've taken inspiration from qemu. They lock bytes from 100 to 100 + N, depending on what operations they want the disk for (checkout raw_apply_lock_bytes() form block/file-posix.c). And my idea was for virtlockd to do the same. Currently, it locks 0th byte of given file and with metadata flag it would lock the first one. Oh yeah, NFSv3 and file locks. Well, I'm not making our situation any worse here. Qemu is already locking disks, and it is doing so for the while lifetime of the domain whereas my patches would lock for only a fraction of a second (well, as long as it takes for chown() and setfilecon_raw() to run). It is very unlikely that connection to the NFSv3 server will be lost right when metadata lock is held. That should never happen (TM). The metadata lock is acquired only for time that seclabels are set on some domain paths (and as we will see in 6/7 we are not going to lock all the paths). As soon as they are set the lock is released. So far there isn't such caller, but sure I can add an explicit check that errors out if both READONLY and METADATA are set at the same time. Well, nresources == 0 can happen regardless of METADATA flag if autoDiskLease is turned off. In that case no domain disk is added into @resources and thus nresources can be zero. This is the situation where we require users to put <lease/> into domain XML which will then protect them. By the same token, when locking for METADATA, we can not ignore autoDiskLease setting (because if user has turned off disk leases they probably did not set any lockspace dir and certainly did not put it onto shared NFS, as required by our docs https://libvirt.org/locking-lockd.html#lockdplugin) and we can't lock files automatically. We must rely on <lease/>. However, we shouldn't fail because from METADATA POV we don't care if a disk is RO or RW. Long story short, if users want to rely on <lease/> solely we should honour that. Oh, okay. Michal

No real support implemented here. But hey, at least we will not fail. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/locking/lock_driver_sanlock.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 3e5f0e37b0..c1996fb937 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -791,7 +791,8 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, virLockManagerSanlockPrivatePtr priv = lock->privateData; virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY | - VIR_LOCK_MANAGER_RESOURCE_SHARED, -1); + VIR_LOCK_MANAGER_RESOURCE_SHARED | + VIR_LOCK_MANAGER_RESOURCE_METADATA, -1); if (priv->res_count == SANLK_MAX_RESOURCES) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -804,6 +805,11 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY) return 0; + /* No metadata locking support for now. + * TODO: implement it. */ + if (flags & VIR_LOCK_MANAGER_RESOURCE_METADATA) + return 0; + switch (type) { case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: if (driver->autoDiskLease) { @@ -953,12 +959,17 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock, virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_RESTRICT | VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY, -1); - if (priv->res_count == 0 && - priv->hasRWDisks && - driver->requireLeaseForDisks) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Read/write, exclusive access, disks were present, but no leases specified")); - return -1; + if (priv->res_count == 0) { + if (priv->hasRWDisks && + driver->requireLeaseForDisks) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Read/write, exclusive access, disks were present, but no leases specified")); + return -1; + } + + /* We are not handling METADATA flag yet. So no resources + * case is no-op for now. */ + return 0; } /* We only initialize 'sock' if we are in the real -- 2.16.4

On 08/17/2018 04:49 PM, John Ferlan wrote: Okay, I'll provide some implementation. Even though sanlock is not really my cup of coffee :-) No we must not do this. Metadata locking is not something users can turn on or off. Okay, they can turn it off when they disable all the locking (i.e. comment out lock_manager in qemu.conf). But if they would have sanlock turned on and wanted to start a domain the virCheckFlags() would be triggered right away and starting a domain would be denied. Sure it would. Look at virDomainLockProcessStart() for instance. It calls virDomainLockManagerNew() which adds all the resources via AddResource() and then calls Acquire(). I rather keep the patches as they are now. It makes more sense to me this way. Michal

On Fri, Aug 17, 2018 at 10:49:12AM -0400, John Ferlan wrote: Yeah, this doesn't feel right to me. I think we need to treat the metadata locking as completely independant of the content locking. This implies we should have a separate configuration for metadata locking, where the only valid options are "lockd" or "nop". eg our available matrix looks like METADATA | nop lockd sanlock ---------+-------------------- nop | Y Y N CONTENT lockd | Y Y N sanlock | Y Y N Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 08/20/2018 05:04 PM, Daniel P. Berrangé wrote: Having some troubles parsing the table. Do you mean: | Content | Metadata ---------+------------------- nop | Y | Y lockd | Y | Y sanlock | Y | N Where Y says the respective type (content/metadata) can/cannot be locked by lock driver in question? I.e. we would have 'metadata_lock_manager' config value in qemu.conf and it'd accept only 'nop' and 'lockd'. Then we can have 'metadata_lockspace_dir' in /etc/libvirt/qemu-lockd.conf where the lockspace would be created. Anyway, I'm gonna state the obvious because it might not be that obvious to somebody else: we can change the RPC between libvirtd and virtlockd safely, because we require both to be the same version. I'm saying that just in case I need to alter the RPC a bit (found some unused procs there, btw but they might come handy someday). Do you mean like completely new config file? qemu-lockd-metadata.conf? Okay. So far we only need one config option (metadata_lockspace_dir) but it might turn out we need more and it would make sens to keep options separated from content locking options. Oh right. From this POV locking from secdriver seems like even better idea. Michal

In order for our drivers to lock resources for metadata change we need set of new APIs. Fortunately, we don't have to care about every possible device a domain can have. We care only about those which can live on a network filesystem and hence can be accessed by multiple daemons at the same time. These devices are covered in virDomainLockMetadataLock() and only a small fraction of those can be hotplugged (covered in the rest of the introduced APIs). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/libvirt_private.syms | 8 ++ src/locking/domain_lock.c | 304 ++++++++++++++++++++++++++++++++++++++++++---- src/locking/domain_lock.h | 28 +++++ 3 files changed, 317 insertions(+), 23 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ca4a192a4a..720ae12301 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1284,6 +1284,14 @@ virDomainLockImageAttach; virDomainLockImageDetach; virDomainLockLeaseAttach; virDomainLockLeaseDetach; +virDomainLockMetadataDiskLock; +virDomainLockMetadataDiskUnlock; +virDomainLockMetadataImageLock; +virDomainLockMetadataImageUnlock; +virDomainLockMetadataLock; +virDomainLockMetadataMemLock; +virDomainLockMetadataMemUnlock; +virDomainLockMetadataUnlock; virDomainLockProcessInquire; virDomainLockProcessPause; virDomainLockProcessResume; diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c index 705b132457..19a097fb25 100644 --- a/src/locking/domain_lock.c +++ b/src/locking/domain_lock.c @@ -69,7 +69,8 @@ static int virDomainLockManagerAddLease(virLockManagerPtr lock, static int virDomainLockManagerAddImage(virLockManagerPtr lock, - virStorageSourcePtr src) + virStorageSourcePtr src, + bool metadataOnly) { unsigned int diskFlags = 0; int type = virStorageSourceGetActualType(src); @@ -82,10 +83,14 @@ static int virDomainLockManagerAddImage(virLockManagerPtr lock, type == VIR_STORAGE_TYPE_DIR)) return 0; - if (src->readonly) - diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY; - if (src->shared) - diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED; + if (metadataOnly) { + diskFlags = VIR_LOCK_MANAGER_RESOURCE_METADATA; + } else { + if (src->readonly) + diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY; + if (src->shared) + diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED; + } VIR_DEBUG("Add disk %s", src->path); if (virLockManagerAddResource(lock, @@ -101,13 +106,68 @@ static int virDomainLockManagerAddImage(virLockManagerPtr lock, } +static int +virDomainLockManagerAddMemory(virLockManagerPtr lock, + const virDomainMemoryDef *mem) +{ + const char *path = NULL; + + switch ((virDomainMemoryModel) mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + path = mem->nvdimmPath; + break; + + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + case VIR_DOMAIN_MEMORY_MODEL_NONE: + break; + } + + if (!path) + return 0; + + VIR_DEBUG("Adding memory %s", path); + if (virLockManagerAddResource(lock, + VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK, + path, + 0, + NULL, + VIR_LOCK_MANAGER_RESOURCE_METADATA) < 0) + return -1; + + return 0; +} + + +static int +virDomainLockManagerAddFile(virLockManagerPtr lock, + const char *file) +{ + if (!file) + return 0; + + VIR_DEBUG("Adding file %s", file); + if (virLockManagerAddResource(lock, + VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK, + file, + 0, + NULL, + VIR_LOCK_MANAGER_RESOURCE_METADATA) < 0) + return -1; + + return 0; +} + + static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, const char *uri, virDomainObjPtr dom, bool withResources, + bool metadataOnly, unsigned int flags) { virLockManagerPtr lock; + const virDomainDef *def = dom->def; size_t i; virLockManagerParam params[] = { { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UUID, @@ -115,11 +175,11 @@ static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, }, { .type = VIR_LOCK_MANAGER_PARAM_TYPE_STRING, .key = "name", - .value = { .str = dom->def->name }, + .value = { .str = def->name }, }, { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UINT, .key = "id", - .value = { .iv = dom->def->id }, + .value = { .iv = def->id }, }, { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UINT, .key = "pid", @@ -133,7 +193,7 @@ static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p withResources=%d", plugin, dom, withResources); - memcpy(params[0].value.uuid, dom->def->uuid, VIR_UUID_BUFLEN); + memcpy(params[0].value.uuid, def->uuid, VIR_UUID_BUFLEN); if (!(lock = virLockManagerNew(virLockManagerPluginGetDriver(plugin), VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN, @@ -144,19 +204,46 @@ static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, if (withResources) { VIR_DEBUG("Adding leases"); - for (i = 0; i < dom->def->nleases; i++) - if (virDomainLockManagerAddLease(lock, dom->def->leases[i]) < 0) + for (i = 0; i < def->nleases; i++) + if (virDomainLockManagerAddLease(lock, def->leases[i]) < 0) goto error; + } + if (withResources || metadataOnly) { VIR_DEBUG("Adding disks"); - for (i = 0; i < dom->def->ndisks; i++) { - virDomainDiskDefPtr disk = dom->def->disks[i]; + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; - if (virDomainLockManagerAddImage(lock, disk->src) < 0) + if (virDomainLockManagerAddImage(lock, disk->src, metadataOnly) < 0) goto error; } } + if (metadataOnly) { + for (i = 0; i < def->nmems; i++) { + virDomainMemoryDefPtr mem = def->mems[i]; + + if (virDomainLockManagerAddMemory(lock, mem) < 0) + goto error; + } + + if (def->os.loader && + virDomainLockManagerAddFile(lock, def->os.loader->nvram) < 0) + goto error; + + if (virDomainLockManagerAddFile(lock, def->os.kernel) < 0) + goto error; + + if (virDomainLockManagerAddFile(lock, def->os.initrd) < 0) + goto error; + + if (virDomainLockManagerAddFile(lock, def->os.dtb) < 0) + goto error; + + if (virDomainLockManagerAddFile(lock, def->os.slic_table) < 0) + goto error; + } + return lock; error: @@ -178,7 +265,7 @@ int virDomainLockProcessStart(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p paused=%d fd=%p", plugin, dom, paused, fd); - if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true, + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true, false, VIR_LOCK_MANAGER_NEW_STARTED))) return -1; @@ -203,7 +290,7 @@ int virDomainLockProcessPause(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p state=%p", plugin, dom, state); - if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true, 0))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true, false, 0))) return -1; ret = virLockManagerRelease(lock, state, 0); @@ -223,7 +310,7 @@ int virDomainLockProcessResume(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p state=%s", plugin, dom, NULLSTR(state)); - if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true, 0))) + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true, false, 0))) return -1; ret = virLockManagerAcquire(lock, state, 0, dom->def->onLockFailure, NULL); @@ -242,7 +329,7 @@ int virDomainLockProcessInquire(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p state=%p", plugin, dom, state); - if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true, 0))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true, false, 0))) return -1; ret = virLockManagerInquire(lock, state, 0); @@ -262,10 +349,10 @@ int virDomainLockImageAttach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p src=%p", plugin, dom, src); - if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false, 0))) + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false, false, 0))) return -1; - if (virDomainLockManagerAddImage(lock, src) < 0) + if (virDomainLockManagerAddImage(lock, src, false) < 0) goto cleanup; if (virLockManagerAcquire(lock, NULL, 0, @@ -299,10 +386,10 @@ int virDomainLockImageDetach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p src=%p", plugin, dom, src); - if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, 0))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, false, 0))) return -1; - if (virDomainLockManagerAddImage(lock, src) < 0) + if (virDomainLockManagerAddImage(lock, src, false) < 0) goto cleanup; if (virLockManagerRelease(lock, NULL, 0) < 0) @@ -336,7 +423,7 @@ int virDomainLockLeaseAttach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p lease=%p", plugin, dom, lease); - if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false, 0))) + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false, false, 0))) return -1; if (virDomainLockManagerAddLease(lock, lease) < 0) @@ -364,7 +451,7 @@ int virDomainLockLeaseDetach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p lease=%p", plugin, dom, lease); - if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, 0))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, false, 0))) return -1; if (virDomainLockManagerAddLease(lock, lease) < 0) @@ -380,3 +467,174 @@ int virDomainLockLeaseDetach(virLockManagerPluginPtr plugin, return ret; } + + +int +virDomainLockMetadataLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom) +{ + virLockManagerPtr lock; + const unsigned int flags = 0; + int ret = -1; + + VIR_DEBUG("plugin=%p dom=%p", plugin, dom); + + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, true, 0))) + return -1; + + if (virLockManagerAcquire(lock, NULL, flags, + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, NULL) < 0) + goto cleanup; + + ret = 0; + cleanup: + virLockManagerFree(lock); + return ret; +} + + +int +virDomainLockMetadataUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom) +{ + virLockManagerPtr lock; + const unsigned int flags = 0; + int ret = -1; + + VIR_DEBUG("plugin=%p dom=%p", plugin, dom); + + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, true, 0))) + return -1; + + if (virLockManagerRelease(lock, NULL, flags) < 0) + goto cleanup; + + ret = 0; + cleanup: + virLockManagerFree(lock); + return ret; +} + + +int +virDomainLockMetadataImageLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virStorageSourcePtr src) +{ + virLockManagerPtr lock; + int ret = -1; + + VIR_DEBUG("plugin=%p dom=%p src=%p", plugin, dom, src); + + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, false, 0))) + return -1; + + if (virDomainLockManagerAddImage(lock, src, true) < 0) + goto cleanup; + + if (virLockManagerAcquire(lock, NULL, 0, + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, NULL) < 0) + goto cleanup; + + ret = 0; + cleanup: + virLockManagerFree(lock); + return ret; +} + + +int +virDomainLockMetadataImageUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virStorageSourcePtr src) +{ + virLockManagerPtr lock; + int ret = -1; + + VIR_DEBUG("plugin=%p dom=%p src=%p", plugin, dom, src); + + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, false, 0))) + return -1; + + if (virDomainLockManagerAddImage(lock, src, true) < 0) + goto cleanup; + + if (virLockManagerRelease(lock, NULL, 0) < 0) + goto cleanup; + + ret = 0; + cleanup: + virLockManagerFree(lock); + return ret; +} + + +int +virDomainLockMetadataDiskLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainDiskDefPtr disk) +{ + return virDomainLockMetadataImageLock(plugin, dom, disk->src); +} + + +int +virDomainLockMetadataDiskUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainDiskDefPtr disk) +{ + return virDomainLockMetadataImageUnlock(plugin, dom, disk->src); +} + + +int +virDomainLockMetadataMemLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainMemoryDefPtr mem) +{ + virLockManagerPtr lock; + int ret = -1; + + VIR_DEBUG("plugin=%p dom=%p mem=%p", plugin, dom, mem); + + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, false, 0))) + return -1; + + if (virDomainLockManagerAddMemory(lock, mem) < 0) + goto cleanup; + + if (virLockManagerAcquire(lock, NULL, 0, + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, NULL) < 0) + goto cleanup; + + ret = 0; + cleanup: + virLockManagerFree(lock); + return ret; +} + + +int +virDomainLockMetadataMemUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainMemoryDefPtr mem) +{ + virLockManagerPtr lock; + int ret = -1; + + VIR_DEBUG("plugin=%p dom=%p mem=%p", plugin, dom, mem); + + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, false, 0))) + return -1; + + if (virDomainLockManagerAddMemory(lock, mem) < 0) + goto cleanup; + + if (virLockManagerRelease(lock, NULL, 0) < 0) + goto cleanup; + + ret = 0; + cleanup: + virLockManagerFree(lock); + return ret; +} diff --git a/src/locking/domain_lock.h b/src/locking/domain_lock.h index fb4910230c..fbd3ee1d4e 100644 --- a/src/locking/domain_lock.h +++ b/src/locking/domain_lock.h @@ -66,4 +66,32 @@ int virDomainLockLeaseDetach(virLockManagerPluginPtr plugin, virDomainObjPtr dom, virDomainLeaseDefPtr lease); +int virDomainLockMetadataLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom); + +int virDomainLockMetadataUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom); + +int virDomainLockMetadataDiskLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainDiskDefPtr disk); +int virDomainLockMetadataDiskUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainDiskDefPtr disk); + +int virDomainLockMetadataImageLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virStorageSourcePtr src); + +int virDomainLockMetadataImageUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virStorageSourcePtr src); + +int virDomainLockMetadataMemLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainMemoryDefPtr mem); +int virDomainLockMetadataMemUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainMemoryDefPtr mem); + #endif /* __VIR_DOMAIN_LOCK_H__ */ -- 2.16.4

On 08/17/2018 07:58 PM, John Ferlan wrote: D'oh! Okay, I'll split this. Well, I think it's just a matter of preference though. My preference was to have this future extensible. I mean, if we ever introduce new model that we need to lock, this switch() would just get its path and the rest is already handled. Whereas if I'd move AddResource() call right into the switch() the call would be duplicated then. Yes. It was discussed here: https://www.redhat.com/archives/libvir-list/2018-July/msg01880.html I prefer avoiding magical constants. For example: func(x, NULL, 0, 0); vs void *resources = NULL; size_t nresources = 0; uint flags = 0; func(x, resources, nresources, flags); Michal

On 08/14/2018 07:19 AM, Michal Privoznik wrote: Kind of sparse... Maybe a few words related to what benefit locking the metadata provides. There is a danger in all this too if the unlock does fail since metadata locks are wait type locks any subsequent lock will wait. Technically not true yet. Also , I think two such attempts with the second eliciting the VIR_WARN is perfectly reasonable (IOW: on failure of the following goto cleanup with locked == true, try again, and spew that message). This repeats a few times, but I'll note just once. Should we add the @stdin_path and @vm->pid and/or vm->name here? It may help some day. Similar comments for other new VIR_WARN's, but the parameters change... Just below here is HostdevLabel processing... For a SCSI host device that would seem to cover the type of resource described in the cover letter to patch6 - at least for iSCSI LUNs (and of course vHBA LUNs). Need to look at the hostdev->mode and hostdev->source.subsys.type and dig into the subsys.u.scsi to get to the src->path. Other than that nothing else sticks out to me. John

On Tue, Aug 14, 2018 at 01:19:43PM +0200, Michal Privoznik wrote: I'm wondering if this is really the right level to plug in the metadata locking ? What about if we just pass a virLockManagerPtr to the security drivers and let them lock each resource at the time they need to modify its metadata. This will trivially guarantee that we always lock the exact set of files that we'll be changing metadata on. eg in SELinux driver the virSecuritySELinuxSetFileconHelper method would directly call virLockManagerAcquire & virLockManagerRelease, avoiding the entire virDomainLock abstraction which was really focused around managing the content locks around lifecycle state changes. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|