On Mon, Dec 12, 2016 at 9:53 PM, Peter Krempa <pkrempa@xxxxxxxxxx> wrote:
> On Mon, Dec 12, 2016 at 20:01:35 +0530, Prasanna Kalever wrote:
>> 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.
>
> [...]
>
>> > 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.
>
> Yes I thought you were not initializing it originally. That's fine then.
>
> ...
>
>>
>> >
>> >>
>> >> 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.
>
> ...
>
> That's true. But if you make sure that the caller always initializes it,
> the condition will become always false.
Oh yeah!
>
>>
>> >
>> >> + 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 ?
>
> Yes you are right. I was probably looking ad a different function, since
> libvirt almost everywhere uses 'def' for the domain definition.
>
>
>> > 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;
>> }
>
> Yes that's right.
>
> Also you should execute this code after the call to
> virDomainSnapshotAlignDisks and qemuDomainSnapshotPrepare. They are
> checking various stuff and determining the default location for the
> disks, so the initialization would fail in some cases otherwise.
In that case the *VolumeRegister* needs to happen as part of
'qemuDomainSnapshotPrepare' to avoid 'virStorageFileInit' call in
'qemuDomainSnapshotPrepareDiskExternal'.
and then the UnRegister happens as part of 'qemuDomainSnapshotCreateXML'
Hope that should be fine ?
>
>> }
>
> [...]
>
>> >> +
>> >> +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 ?
>
> Okay. It may make sense in the future to have all the exit points marked
> using a special function.
Yeah, sure.
>
> [...]
>
>> >> 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 ?
>
> I meant to make it a virObject, so that you can refcount it. The need
> for a flag to track whether it was initialized is rather fragile.
Make sense.
Thanks,
--
Prasanna
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
