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.
>
> >
> >> + 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.
> }
[...]
> >> +
> >> +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.
[...]
> >> 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.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
