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

[Prasanna Kalever]

On Mon, Dec 12, 2016 at 9:53 PM, Peter Krempa <pkrempa at redhat.com> 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 at redhat.com> 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 at redhat.com>
>> >> ---
>> >>  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