Libguestfs - Oct 2015 - Re: [PATCH v2 4/4] v2v: reuse main guestfs for virtio win drivers iso

If we're given an ISO image as the source of virtio windows drivers, and
the backend supports hot-adding drives, reuse the same guestfs handle
that is open for accessing the guest being inspected, rather than
creating a new one (which would run another virtual machine instance and
is fairly resource-intensive).

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
changes since v1:
 - updated usage of string functions
 - add missing and remove excessive parentheses
 - preserve support for backends with no hot-add capability

 v2v/convert_windows.ml | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml
index cfa5474..5fd8bb6 100644
--- a/v2v/convert_windows.ml
+++ b/v2v/convert_windows.ml
@@ -294,13 +294,25 @@ echo uninstalling Xen PV driver
     )
     else if is_regular_file virtio_win then (
       try
-        let g2 = new Guestfs.guestfs () in
-        if trace () then g2#set_trace true;
-        if verbose () then g2#set_verbose true;
-        g2#add_drive_opts virtio_win ~readonly:true;
-        g2#launch ();
-        let vio_root = "/" in
-        g2#mount_ro "/dev/sda" vio_root;
+        let label = "viowin" in
+        let vio_root = "/virtio-win-iso" in
+        let g2 +          (* not all backends support hot-adding drives; create
an extra
+           * guestfs handle in that case *)
+          try
+            g#add_drive_opts virtio_win ~readonly:true ~label:label;
+            g
+          with Guestfs.Error msg -> (
+            let g2 = new Guestfs.guestfs () in
+            if trace () then g2#set_trace true;
+            if verbose () then g2#set_verbose true;
+            g2#add_drive_opts virtio_win ~readonly:true ~label:label;
+            g2#launch ();
+            g2
+          ) in
+
+        g2#mkmountpoint vio_root;
+        g2#mount_ro ("/dev/disk/guestfs" // label) vio_root;
         let paths = g2#find vio_root in
         Array.iter (
           fun path ->
@@ -315,10 +327,18 @@ echo uninstalling Xen PV driver
                 printf "Copying virtio driver bits: '%s:%s' ->
'%s'\n"
                   virtio_win path target;
 
-              g#write target (g2#read_file source)
+              if (g2 == g) then
+                g#cp source target
+              else
+                g#write target (g2#read_file source)
             )
         ) paths;
-        g2#close()
+        g2#umount vio_root;
+        g2#rmmountpoint vio_root;
+        if (g2 == g) then
+          g2#remove_drive label
+        else
+          g2#close()
       with Guestfs.Error msg ->
         error (f_"%s: cannot open virtio-win ISO file: %s")
virtio_win msg
     )
-- 
2.4.3

On Tue, Oct 13, 2015 at 03:50:54PM +0300, Roman Kagan
wrote:
> If we're given an ISO image as the source of virtio windows drivers,
and
> the backend supports hot-adding drives, reuse the same guestfs handle
> that is open for accessing the guest being inspected, rather than
> creating a new one (which would run another virtual machine instance and
> is fairly resource-intensive).
I really don't like this specific patch.  Hotplugging is not very
reliable in my experience.  The patch tries to work out if hotplugging
is going to work by calling guestfs_add_drive and taking an alternate
path, but I'm not so convinced that would work on other architectures
(eg. hotplugging on aarch64 is in various states between not working
at all and flaky).

Are you observing any benefit with this patch?  v2v conversion takes a
long time - I can't believe the extra overhead of creating a handle
could make much difference.

I'll look at the other patches soon.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Tue, Oct 13, 2015 at 02:36:10PM +0100, Richard W.M. Jones
wrote:
> On Tue, Oct 13, 2015 at 03:50:54PM +0300, Roman Kagan wrote:
> > If we're given an ISO image as the source of virtio windows
drivers, and
> > the backend supports hot-adding drives, reuse the same guestfs handle
> > that is open for accessing the guest being inspected, rather than
> > creating a new one (which would run another virtual machine instance
and
> > is fairly resource-intensive).
> 
> I really don't like this specific patch.  Hotplugging is not very
> reliable in my experience.  The patch tries to work out if hotplugging
> is going to work by calling guestfs_add_drive and taking an alternate
> path, but I'm not so convinced that would work on other architectures
> (eg. hotplugging on aarch64 is in various states between not working
> at all and flaky).
> 
> Are you observing any benefit with this patch?  v2v conversion takes a
> long time - I can't believe the extra overhead of creating a handle
> could make much difference.
Frankly, I don't have convincing numbers to justify this patch.  In my
measurements with test conversions on an overwise idle workstation gives
on the order of 5% gain where statistical variance is just a little
less; OTOH LIBGUESTFS_BACKEND=direct gives statistically significant
30-40% improvement :)

We've been thinking of doing mass VM conversion where an extra qemu
instance for every v2v operation would create significant memory and io
overhead but I don't have a proof of that.

In view of hot-add being unstable as you say, I'm fine with dropping
this patch altogether.

Thanks,
Roman.