Re: [libvirt] [PATCH v4 2/4] storage: optimize calls to virStorageFileInit and friends

On Tue, Dec 06, 2016 at 22:51:59 +0530, Prasanna Kumar Kalever wrote: > Currently, each among virStorageFileGetMetadataRecurse, > qemuSecurityChownCallback, qemuDomainSnapshotPrepareDiskExternal and > qemuDomainSnapshotCreateSingleDiskActive makes calls to virStorageFileInit > and friends for simple operations like stat, read headers, chown and etc. > > This patch optimize/unify calls to virStorageFileInit and virStorageFileDeinit. A bit clearer explanation would help. This is mostly relevat for VM startup where the multiple calls are done. VM shutdown currently calls just one operation, but is not optimized with this patch. > > Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever(a)redhat.com&gt; > --- > src/qemu/qemu_domain.c | 2 +- > src/qemu/qemu_domain.h | 5 +++++ > src/qemu/qemu_driver.c | 40 +++++++++++++++++++++++++++---------- > src/qemu/qemu_process.c | 47 ++++++++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_process.h | 4 ++++ > src/storage/storage_driver.c | 11 ++++++++--- > 6 files changed, 95 insertions(+), 14 deletions(-) Read below for a suggestion of a farbetter approach than having the 'initFlag' variable in all places that call the code. [...] > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 3517aa2..d0e05f8 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -328,6 +328,7 @@ qemuSecurityChownCallback(virStorageSourcePtr src, > { > struct stat sb; > int save_errno = 0; > + bool initFlag = false; I'd call this 'deinitsrc'.

> int ret = -1; > > if (!virStorageFileSupportsSecurityDriver(src)) > @@ -351,8 +352,11 @@ qemuSecurityChownCallback(virStorageSourcePtr src, > } > > /* storage file init reports errors, return -2 on failure */ > - if (virStorageFileInit(src) < 0) > - return -2; > + if (!src->drv) { You should export and reuse virStorageFileIsInitialized rather than reimplementing the detection.

> + if (virStorageFileInit(src) < 0) > + return -2; > + initFlag = true; > + } > > if (virStorageFileChown(src, uid, gid) < 0) { > save_errno = errno; > @@ -362,7 +366,8 @@ qemuSecurityChownCallback(virStorageSourcePtr src, > ret = 0; > > cleanup: > - virStorageFileDeinit(src); > + if (initFlag) > + virStorageFileDeinit(src); > errno = save_errno; > > return ret; > @@ -13865,9 +13870,6 @@ qemuDomainSnapshotPrepareDiskExternal(virConnectPtr conn, > return -1; > } > > - if (virStorageFileInit(snapdisk->src) < 0) > - return -1; > - > if (virStorageFileStat(snapdisk->src, &st) < 0) { > if (errno != ENOENT) { > virReportSystemError(errno, > @@ -13891,7 +13893,6 @@ qemuDomainSnapshotPrepareDiskExternal(virConnectPtr conn, > ret = 0; > > cleanup: > - virStorageFileDeinit(snapdisk->src); > return ret; > } > > @@ -14126,6 +14127,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, > const char *formatStr = NULL; > int ret = -1, rc; > bool need_unlink = false; > + bool initFlag = false; > > if (snap->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { > virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > @@ -14138,12 +14140,18 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, > > if (!(newDiskSrc = virStorageSourceCopy(snap->src, false))) > goto cleanup; > + else > + if (snap->src->drv) > + newDiskSrc->drv = snap->src->drv; The above code is confusing and does not conform to the coding style. Drop the 'else' statement completely, since the first part jumps to cleanup anyways.

Also snap->src->drv won't ever be initialized. the 'snap' object is freshly created from the XML provided by the user so it never was part of the definition and thus couldn't ever be initialized.

> > if (virStorageSourceInitChainElement(newDiskSrc, disk->src, false) < 0) > goto cleanup; > > - if (qemuDomainStorageFileInit(driver, vm, newDiskSrc) < 0) > - goto cleanup; > + if (!newDiskSrc->drv) { This condition is always true due to the fact above.

> + if (qemuDomainStorageFileInit(driver, vm, newDiskSrc) < 0) > + goto cleanup; > + initFlag = true; > + } > > if (qemuGetDriveSourceString(newDiskSrc, NULL, &source) < 0) > goto cleanup; > @@ -14211,7 +14219,8 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, > cleanup: > if (need_unlink && virStorageFileUnlink(newDiskSrc)) > VIR_WARN("unable to unlink just-created %s", source); > - virStorageFileDeinit(newDiskSrc); > + if (initFlag) > + virStorageFileDeinit(newDiskSrc); > virStorageSourceFree(newDiskSrc); > virStorageSourceFree(persistDiskSrc); > VIR_FREE(device); > @@ -14566,6 +14575,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, > virDomainSnapshotObjPtr snap = NULL; > virDomainSnapshotPtr snapshot = NULL; > virDomainSnapshotDefPtr def = NULL; > + virDomainSnapshotDefPtr refDef = NULL; > bool update_current = true; > bool redefine = flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE; > unsigned int parse_flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; > @@ -14574,6 +14584,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, > bool align_match = true; > virQEMUDriverConfigPtr cfg = NULL; > virCapsPtr caps = NULL; > + size_t i; > > virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | > VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT | > @@ -14690,6 +14701,10 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, > > qemuDomainObjSetAsyncJobMask(vm, QEMU_JOB_NONE); > > + for (i = 0; i < def->ndisks; i++) > + if (qemuStorageVolumeRegister(cfg, vm, def->disks[i].src) < 0) > + goto cleanup; Initializing the domain disks is useless at this point. The new snapshot volumes will be accessed, not the existing disks.

This code also is not necessary for internal snapshots and thus should not be executed in such cases.

> + > if (redefine) { > if (virDomainSnapshotRedefinePrep(domain, vm, &def, &snap, > &update_current, flags) < 0) > @@ -14800,6 +14815,11 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, > snapshot = virGetDomainSnapshot(domain, snap->def->name); > > endjob: > + refDef = (!snap) ? def : snap->def; > + > + for (i = 0; i < refDef->ndisks; i++) > + qemuStorageVolumeUnRegister(refDef->disks[i].src); > + > if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) { > if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->caps, > cfg->snapshotDir) < 0) { > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 7f19c69..c58e622 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -4590,6 +4590,35 @@ qemuProcessStartValidate(virQEMUDriverPtr driver, > } > > > +int > +qemuStorageVolumeRegister(virQEMUDriverConfigPtr cfg, > + virDomainObjPtr vm, virStorageSourcePtr src) > +{ > + uid_t uid; > + gid_t gid; > + > + if (virStorageSourceIsEmpty(src)) > + return 0; > + > + qemuDomainGetImageIds(cfg, vm, src, &uid, &gid); > + > + if (virStorageFileInitAs(src, uid, gid) < 0) > + return -1; > + > + return 0; > +} > + > + > +void > +qemuStorageVolumeUnRegister(virStorageSourcePtr src) > +{ > + if (virStorageSourceIsEmpty(src)) > + return; > + > + virStorageFileDeinit(src); > +} This wrapper is rather useless. You can call virStorageFileDeinit on volume files that were not initialized.

> + > + > /** > * qemuProcessInit: > * > @@ -4614,6 +4643,7 @@ qemuProcessInit(virQEMUDriverPtr driver, > qemuDomainObjPrivatePtr priv = vm->privateData; > int stopFlags; > int ret = -1; > + size_t i; > > VIR_DEBUG("vm=%p name=%s id=%d migration=%d", > vm, vm->def->name, vm->def->id, migration); > @@ -4666,6 +4696,12 @@ qemuProcessInit(virQEMUDriverPtr driver, > if (qemuDomainSetPrivatePaths(driver, vm) < 0) > goto cleanup; > > + if (!(flags & VIR_QEMU_PROCESS_START_PRETEND)) { > + for (i = 0; i < vm->def->ndisks; i++) > + if (qemuStorageVolumeRegister(cfg, vm, vm->def->disks[i]->src) < 0) > + goto cleanup; This does not conform to the coding style. Multi line bodies need to be included in a block. > + } > + > ret = 0; > > cleanup: > @@ -6047,6 +6087,10 @@ void qemuProcessStop(virQEMUDriverPtr driver, > VIR_FREE(xml); > } > > + for (i = 0; i < vm->def->ndisks; i++) > + if (qemuStorageVolumeRegister(cfg, vm, vm->def->disks[i]->src) < 0) > + goto cleanup; > + > /* Reset Security Labels unless caller don't want us to */ > if (!(flags & VIR_QEMU_PROCESS_STOP_NO_RELABEL)) > virSecurityManagerRestoreAllLabel(driver->securityManager, If it's requested not to relabel the file as seed just below the code you added, you don't need to initialize the volume at all. > @@ -6210,6 +6254,9 @@ void qemuProcessStop(virQEMUDriverPtr driver, > VIR_FREE(xml); > } > > + for (i = 0; i < vm->def->ndisks; i++) > + qemuStorageVolumeUnRegister(vm->def->disks[i]->src); In the above case, this will need to be skipped as well. > + > virDomainObjRemoveTransientDef(vm); > > endjob: [...] > diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c > index 7c7fddd..24e2f35 100644 > --- a/src/storage/storage_driver.c > +++ b/src/storage/storage_driver.c > @@ -3195,6 +3195,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, > ssize_t headerLen; > virStorageSourcePtr backingStore = NULL; > int backingFormat; > + bool initFlag = false; > > VIR_DEBUG("path=%s format=%d uid=%u gid=%u probe=%d", > src->path, src->format, > @@ -3204,8 +3205,11 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, > if (!virStorageFileSupportsBackingChainTraversal(src)) > return 0; > > - if (virStorageFileInitAs(src, uid, gid) < 0) > - return -1; > + if (!src->drv) { > + if (virStorageFileInitAs(src, uid, gid) < 0) > + return -1; > + initFlag = true; > + } > > if (virStorageFileAccess(src, F_OK) < 0) { > if (src == parent) { > @@ -3281,7 +3285,8 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, > > cleanup: > VIR_FREE(buf); > - virStorageFileDeinit(src); > + if (initFlag) > + virStorageFileDeinit(src); > virStorageSourceFree(backingStore); > return ret; > } Alternatively and preferably you could turn src->drv into a object. This would add the option to do reference counting and would get rid of the need to remember whether a given piece of code needs to deinitialize a storage volume. Most of the changes above would not be necessary then.

Peter