On Wed, Dec 7, 2016 at 9:38 PM, Peter Krempa <pkrempa@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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'.
I like it.
>
>> 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.
Okay!
>
>> + 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.
correct, just over looked at it :)
>
> 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;
>> @@ -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.
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;
}
}
>
>> +
>> 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.
Right! knowing the fact, thought good to have a Unregister for counter
function Register ?
>
>> +
>> +
>> /**
>> * 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.
I doubt If I got you right here.
An object as part of ?
Thanks,
--
Prasanna
>
> Peter
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
