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

On Wed, Dec 7, 2016 at 9:38 PM, Peter Krempa <pkrempa(a)redhat.com&gt; wrote: > 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.

> 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. As part of this patch the initialization of the external snapshot file happens as part of qemuStorageVolumeRegister in qemuDomainSnapshotCreateXML and servers the initialized data to 1. qemuDomainSnapshotPrepareDiskExternal called by qemuDomainSnapshotPrepare 2. qemuDomainSnapshotCreateSingleDiskActive 3. qemuSecurityChownCallback and 4. virStorageFileGetMetadataRecurse saving the calls to virStorageFileInitAs at each points literally 4 calls May be I can change the hunk to use virStorageFileIsInitialized.

> >> >> 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. same here.

> >> + if (qemuDomainStorageFileInit(driver, vm, newDiskSrc) < 0) >> + goto cleanup; >> + initFlag = true; >> + } >> >> if (qemuGetDriveSourceString(newDiskSrc, NULL, &source) < 0) >> goto cleanup;

>> @@ -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. Let me know if I read it wrong. In this function variable 'snap->def' will be assigned to 'def' in virDomainSnapshotAssignDef so we should initialize def->disks[ ] right ?

> > This code also is not necessary for internal snapshots and thus should > not be executed in such cases. Yeah revised one should look like for (i = 0; i < def->ndisks; i++) { if(def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { if (qemuStorageVolumeRegister(cfg, vm, def->disks[i].src) < 0) goto cleanup; }

}

>> + >> +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. Right! knowing the fact, thought good to have a Unregister for counter function Register ?

>> 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

>> @@ -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. I doubt If I got you right here. An object as part of ?