On 08/14/2018 07:19 AM, Michal Privoznik wrote:
> 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@xxxxxxxxxx>
> ---
> src/libvirt_private.syms | 8 ++
> src/locking/domain_lock.c | 304 ++++++++++++++++++++++++++++++++++++++++++----
> src/locking/domain_lock.h | 28 +++++
> 3 files changed, 317 insertions(+), 23 deletions(-)
>
There seems to be more than just what's described going on in this patch
which could be split out....
1. Use @def in virDomainLockManagerNew
2. Add @metadataonly to virDomainLockManagerNew and
virDomainLockManagerAddImage
3. Introduce virDomainLockManagerAddMemory and
virDomainLockManagerAddFile along with changes to
virDomainLockManagerNew along with perhaps a few words why those files
were chosen and what decisions led to their selection. If someone adds
something in the future how would they know to add them to the list?
4. Various virDomainLockMetadata* API's.
> 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;
Why not flip flop the order of new helpers and just call/return
virDomainLockManagerAddFile directly with 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;
And then just return 0 here. or just call it here since it returns 0 if
@!file anyway.
> +
> + 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) {
So this would seemingly answer my question long ago - one could get one
or both locks (@0 w/ length==1 and @1 w/ length==1)
> 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;
We never change this from 0 to something else (not even the next
patch)... Not that it matters, just noting it considering that other new
API's just pass 0 and not flags which is only ever set to 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)
Strange to see a longer second line, although I understand seeing NULL
along leads to why not just put it on the previous line type review comment.
FWIW: Seeing how @action is set here led me down the path of seeing how
it's used.
I wonder if perhaps that could be used in the lockd/sanlock code in
order to determine how/whether to fail rather than just returning 0 in
sanlock when METADATA isn't supported. If it really matters that is.
It's not 100% clear to me what/how the expected action usage should be.
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + virLockManagerFree(lock);
> + return ret;
> +}
> +
> +
> +int
> +virDomainLockMetadataUnlock(virLockManagerPluginPtr plugin,
> + virDomainObjPtr dom)
> +{
> + virLockManagerPtr lock;
> + const unsigned int flags = 0;
Similar @flags commentary.
> + int ret = -1;
> +
> + VIR_DEBUG("plugin=%p dom=%p", plugin, dom);
> +
> + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, true, 0)))
Since MetadataLock/Unlock are the only ones passing true for the
@metadataOnly bool in virDomainLockManagerNew, I'm "conflicted" over
whether there should a helper for both in lieu of the bool, but then the
bool does the trick and it's following @withResources which was there
since inception, so I guess it's fine albeit odd w/r/t a *New API doing
more than just allocating memory or setting defaults.
Not an issue, but an observation, in case you were wondering. Trying to
learn a bit about this code while doing this review...
John
> + 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__ */
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
