Libguestfs - Oct 2015 - Re: [PATCH v3 0/3] v2v: simplify driver copying from virtio-win iso

On Thu, Oct 15, 2015 at 10:07:45AM +0100, Richard W.M. Jones
wrote:
> On Wed, Oct 14, 2015 at 07:55:41PM +0300, Roman Kagan wrote:
> > Libguestfs supports passing an ISO image as a source of virtio windows
> > drivers to v2v.
> > 
> > This series attempts to make it simpler and better scoped.
> 
> Looks good (apart from patch 3 - the Gc.compact is necessary!)
Why?  As a matter of fact that was the primary reason I even started
with this patchset: that Gc.compact was sitting in the middle of the
main v2v function and was yet another obstacle in the refactoring I was
after.

Now that the extra guestfs handle doesn't survive beyond
copy_virtio_drivers() what is the need for that explicit GC (which was
introduced at the time exactly to clear up that handle in commit
47b5f245bec908f803f0a89c3b1e3166cfe33aad)?

Roman.

On Thu, Oct 15, 2015 at 12:27:58PM +0300, Roman Kagan
wrote:
> On Thu, Oct 15, 2015 at 10:07:45AM +0100, Richard W.M. Jones wrote:
> > On Wed, Oct 14, 2015 at 07:55:41PM +0300, Roman Kagan wrote:
> > > Libguestfs supports passing an ISO image as a source of virtio
windows
> > > drivers to v2v.
> > > 
> > > This series attempts to make it simpler and better scoped.
> > 
> > Looks good (apart from patch 3 - the Gc.compact is necessary!)
> 
> Why?  As a matter of fact that was the primary reason I even started
> with this patchset: that Gc.compact was sitting in the middle of the
> main v2v function and was yet another obstacle in the refactoring I was
> after.
> 
> Now that the extra guestfs handle doesn't survive beyond
> copy_virtio_drivers() what is the need for that explicit GC (which was
> introduced at the time exactly to clear up that handle in commit
> 47b5f245bec908f803f0a89c3b1e3166cfe33aad)?
Just because the handle becomes unreachable doesn't mean the GC will
immediately collect it.  Objects are only required to be finalized
when you invoke Gc.full_major (Gc.compact calls Gc.full_major).

Previous to these patches, the handle was still unreachable, but
Gc.compact didn't free it because of a different problem.  That
different issue has been fixed by:

https://github.com/libguestfs/libguestfs/commit/8bbc5e73cb5b56b5cfbe979ac0e1c14d1701a0d8

In summary, we still need to call Gc.compact before doing the
long-running copy operation, to make sure that the resources used by
virt-v2v are minimized during the copy.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Thu, Oct 15, 2015 at 10:40:15AM +0100, Richard W.M. Jones
wrote:
> On Thu, Oct 15, 2015 at 12:27:58PM +0300, Roman Kagan wrote:
> > On Thu, Oct 15, 2015 at 10:07:45AM +0100, Richard W.M. Jones wrote:
> > > On Wed, Oct 14, 2015 at 07:55:41PM +0300, Roman Kagan wrote:
> > > > Libguestfs supports passing an ISO image as a source of
virtio windows
> > > > drivers to v2v.
> > > > 
> > > > This series attempts to make it simpler and better scoped.
> > > 
> > > Looks good (apart from patch 3 - the Gc.compact is necessary!)
> > 
> > Why?  As a matter of fact that was the primary reason I even started
> > with this patchset: that Gc.compact was sitting in the middle of the
> > main v2v function and was yet another obstacle in the refactoring I
was
> > after.
> > 
> > Now that the extra guestfs handle doesn't survive beyond
> > copy_virtio_drivers() what is the need for that explicit GC (which was
> > introduced at the time exactly to clear up that handle in commit
> > 47b5f245bec908f803f0a89c3b1e3166cfe33aad)?
> 
> Just because the handle becomes unreachable doesn't mean the GC will
> immediately collect it.  Objects are only required to be finalized
> when you invoke Gc.full_major (Gc.compact calls Gc.full_major).
> 
> Previous to these patches, the handle was still unreachable, but
> Gc.compact didn't free it because of a different problem.  That
> different issue has been fixed by:
> 
>
https://github.com/libguestfs/libguestfs/commit/8bbc5e73cb5b56b5cfbe979ac0e1c14d1701a0d8
> 
> In summary, we still need to call Gc.compact before doing the
> long-running copy operation, to make sure that the resources used by
> virt-v2v are minimized during the copy.
What resources are you talking about?  The only costly resource eater is
the auxiliary hypervisor backend instance; it used to be terminated
indirectly via this forced GC here (after your fix), but now it's shut
down explictly before leaving copy_virtio_drivers().  Anything else that
can get collected here is negligible compared to what remains.

Roman.