On 08/17/2018 07:58 PM, John Ferlan wrote:
>
>
> 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.
>
D'oh! Okay, I'll split this.
>
>> 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.
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.
>
>> +
>> + 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)
Yes. It was discussed here:
https://www.redhat.com/archives/libvir-list/2018-July/msg01880.html
>
>> 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.
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);
>
>> + 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
>
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
