Re: [Libguestfs] [PATCH v2v] convert: Allow preferred block driver to be specified on the command line
On 3/6/23 18:52, Richard W.M. Jones wrote: > This is just an outline patch, only compile tested. > > It doesn't make changes to virt-v2v-in-place, but those would be the > same as made in v2v/v2v.ml. > > It reuses the existing Types.guestcaps_block_type which is a bit ugly > but fairly practical. > > I've made the change to the documentation, but it needs a test. > > Rich. > Thanks for the provided outlines, I adapted them in my v2: https://listman.redhat.com/archives/libguestfs/2023-March/030987.html https://listman.redhat.com/archives/libguestfs/2023-March/030989.html I have tested the in-place mode, and the switch seems to be working as expected. The patch also adds the convert option to the virt-v2v-inspector tool as it's also using the conversion mechanism. I figured that this cmdline switch isn't much needed there, so the option is set just for avoiding compilation issues. ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v2v] convert: Allow preferred block driver to be specified on the command line
On Tue, Mar 07, 2023 at 11:26:37AM +0100, Laszlo Ersek wrote:
> On 3/7/23 09:35, Richard W.M. Jones wrote:
> > On Tue, Mar 07, 2023 at 08:48:56AM +0100, Laszlo Ersek wrote:
> >> On 3/6/23 17:52, Richard W.M. Jones wrote:
> >>
> >>> diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml
> >>> index 9d8d271d05..f36b486359 100644
> >>> --- a/convert/convert_windows.ml
> >>> +++ b/convert/convert_windows.ml
> >>> @@ -38,7 +38,7 @@ module G = Guestfs
> >>> * time the Windows VM is booted on KVM.
> >>> *)
> >>>
> >>> -let convert (g : G.guestfs) _ inspect i_firmware _ static_ips =
> >>> +let convert (g : G.guestfs) _ inspect i_firmware block_driver _
> >>> static_ips =
> >>>
> >>> (*--*)
> >>>(* Inspect the Windows guest. *)
> >>>
> >>> @@ -47,6 +47,16 @@ let convert (g : G.guestfs) _ inspect i_firmware _
> >>> static_ips =
> >>> *)
> >>>let virtio_win =
> >>> Inject_virtio_win.from_environment g inspect.i_root Config.datadir in
> >>> + (match block_driver with
> >>> + | Virtio_blk -> () (* the default, no need to do anything *)
> >>> +(*
> >>> + | Virtio_scsi ->
> >>> + let drivers = Inject_virtio_win.get_block_driver_priority
> >>> virtio_win in
> >>> + let drivers = "vioscsi" :: drivers in
> >>> + Inject_virtio_win.set_block_driver_priority virtio_win drivers
> >>> +*)
> >>> + | IDE -> assert false (* not possible - but maybe ...? *)
> >>> + );
> >>>
> >>>(* If the Windows guest appears to be using group policy.
> >>> *
> >>
> >> So ["virtio_blk"; "vrtioblk"; "viostor"] in
> >> "common/mlcustomize/inject_virtio_win.ml" would be replaced with a
> >> reference cell, and get_block_driver_priority /
> >> set_block_driver_priority would manipulate that.
> >
> > References are really just structures with mutable fields and some
> > syntactic sugar:
> >
> > https://github.com/ocaml/ocaml/blob/66db964f48869c6470b8ccd1ed672e244ba9d680/stdlib/stdlib.ml#L237
> >
> > The reason mutable fields of structures in general have to be
> > specially marked with the "mutable" keyword is because of the
> > "remembered set", an obscure corner of generational garbage collection:
> >
> > https://www.cs.cornell.edu/courses/cs3110/2012sp/lectures/lec26-gc/lec26.html
> >
> > If mutable fields didn't exist then you could never have a pointer
> > from the old generation into the new generation, because new
> > generation objects are always younger than old generation objects, and
> > as all fields have to be set at object creation time (no mutation),
> > there cannot exist pointers from old to new.
> >
> > Since mutable fields _do_ exist, it's possible for such pointers to
> > exist. When you do a minor GC of the younger generation, to avoid
> > having to scan over the whole of the old heap to find these pointers,
> > when a mutable field is mutated a record is added to the "remembered
> > set", which is treated as an additional set of roots for the minor GC.
> >
> > After each minor GC the remembered set can be cleared.
>
> Why is that?
>
> If the "remembered" objects remain in the young generation, still only
> referenced by objects of the old generation, then the extra roots
> pointing to these young objects should persist, shouldn't they? A young
> object can be removed from the remembered set when it transitions to the
> old generation at the same time -- but that need not happen at each GC
> run over the young generation. IIUC.
When you do a minor GC you're moving all the still referenced objects
from the minor heap ("young generation") into the major heap ("old
generation"). Therefore they just become objects in the old
generation where they can all reference each other, not distinguished
by age, and that is all handled by a different algorithm (mark-sweep-compact).
Meanwhile the minor heap, which is basically a single fixed-sized area
of memory that allocations come from is reset.
Thus the remembered set is no longer needed and can be cleared.
This is for the simple two generation case as used by OCaml. Golang
uses a non-generational collector. Java uses a multi-generation
system where they have remembered sets for each generation ("region"
in Java-GC-speak), and they also use a slightly different algorithm
for the remembered set called "card table" but it solves the same
problem.
BTW I found a page which describes the OCaml collector and benchmarks
the overhead of mutable structs and has the discussion of the
trade-offs involved (in short, mutable algorithms have less
allocation, but the allocations have more overhead because of the
remembered set):
https://dev.realworldocaml.org/garbage-collector.html#scrollNav-4-5
Rich.
> > But there's
> > still some overhead here, so marking fields in a struct as mutable is
> > not cost free. (Of course in this specific case the cost is
> > infinitesimally small, but in general it can be a concern.)
> >
> > This is a long way o
Re: [Libguestfs] [PATCH v2v] convert: Allow preferred block driver to be specified on the command line
On 3/7/23 09:35, Richard W.M. Jones wrote: > On Tue, Mar 07, 2023 at 08:48:56AM +0100, Laszlo Ersek wrote: >> On 3/6/23 17:52, Richard W.M. Jones wrote: >> >>> diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml >>> index 9d8d271d05..f36b486359 100644 >>> --- a/convert/convert_windows.ml >>> +++ b/convert/convert_windows.ml >>> @@ -38,7 +38,7 @@ module G = Guestfs >>> * time the Windows VM is booted on KVM. >>> *) >>> >>> -let convert (g : G.guestfs) _ inspect i_firmware _ static_ips = >>> +let convert (g : G.guestfs) _ inspect i_firmware block_driver _ static_ips >>> = >>> >>> (*--*) >>>(* Inspect the Windows guest. *) >>> >>> @@ -47,6 +47,16 @@ let convert (g : G.guestfs) _ inspect i_firmware _ >>> static_ips = >>> *) >>>let virtio_win = >>> Inject_virtio_win.from_environment g inspect.i_root Config.datadir in >>> + (match block_driver with >>> + | Virtio_blk -> () (* the default, no need to do anything *) >>> +(* >>> + | Virtio_scsi -> >>> + let drivers = Inject_virtio_win.get_block_driver_priority virtio_win >>> in >>> + let drivers = "vioscsi" :: drivers in >>> + Inject_virtio_win.set_block_driver_priority virtio_win drivers >>> +*) >>> + | IDE -> assert false (* not possible - but maybe ...? *) >>> + ); >>> >>>(* If the Windows guest appears to be using group policy. >>> * >> >> So ["virtio_blk"; "vrtioblk"; "viostor"] in >> "common/mlcustomize/inject_virtio_win.ml" would be replaced with a >> reference cell, and get_block_driver_priority / >> set_block_driver_priority would manipulate that. > > References are really just structures with mutable fields and some > syntactic sugar: > > https://github.com/ocaml/ocaml/blob/66db964f48869c6470b8ccd1ed672e244ba9d680/stdlib/stdlib.ml#L237 > > The reason mutable fields of structures in general have to be > specially marked with the "mutable" keyword is because of the > "remembered set", an obscure corner of generational garbage collection: > > https://www.cs.cornell.edu/courses/cs3110/2012sp/lectures/lec26-gc/lec26.html > > If mutable fields didn't exist then you could never have a pointer > from the old generation into the new generation, because new > generation objects are always younger than old generation objects, and > as all fields have to be set at object creation time (no mutation), > there cannot exist pointers from old to new. > > Since mutable fields _do_ exist, it's possible for such pointers to > exist. When you do a minor GC of the younger generation, to avoid > having to scan over the whole of the old heap to find these pointers, > when a mutable field is mutated a record is added to the "remembered > set", which is treated as an additional set of roots for the minor GC. > > After each minor GC the remembered set can be cleared. Why is that? If the "remembered" objects remain in the young generation, still only referenced by objects of the old generation, then the extra roots pointing to these young objects should persist, shouldn't they? A young object can be removed from the remembered set when it transitions to the old generation at the same time -- but that need not happen at each GC run over the young generation. IIUC. > But there's > still some overhead here, so marking fields in a struct as mutable is > not cost free. (Of course in this specific case the cost is > infinitesimally small, but in general it can be a concern.) > > This is a long way of saying that it's not a reference cell, but a > plain field in the 't' struct, with extra runtime overhead and code > generated for when it is mutated. Thanks! Laszlo ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v2v] convert: Allow preferred block driver to be specified on the command line
On Tue, Mar 07, 2023 at 08:48:56AM +0100, Laszlo Ersek wrote: > On 3/6/23 17:52, Richard W.M. Jones wrote: > > > diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml > > index 9d8d271d05..f36b486359 100644 > > --- a/convert/convert_windows.ml > > +++ b/convert/convert_windows.ml > > @@ -38,7 +38,7 @@ module G = Guestfs > > * time the Windows VM is booted on KVM. > > *) > > > > -let convert (g : G.guestfs) _ inspect i_firmware _ static_ips = > > +let convert (g : G.guestfs) _ inspect i_firmware block_driver _ static_ips > > = > > > > (*--*) > >(* Inspect the Windows guest. *) > > > > @@ -47,6 +47,16 @@ let convert (g : G.guestfs) _ inspect i_firmware _ > > static_ips = > > *) > >let virtio_win = > > Inject_virtio_win.from_environment g inspect.i_root Config.datadir in > > + (match block_driver with > > + | Virtio_blk -> () (* the default, no need to do anything *) > > +(* > > + | Virtio_scsi -> > > + let drivers = Inject_virtio_win.get_block_driver_priority virtio_win > > in > > + let drivers = "vioscsi" :: drivers in > > + Inject_virtio_win.set_block_driver_priority virtio_win drivers > > +*) > > + | IDE -> assert false (* not possible - but maybe ...? *) > > + ); > > > >(* If the Windows guest appears to be using group policy. > > * > > So ["virtio_blk"; "vrtioblk"; "viostor"] in > "common/mlcustomize/inject_virtio_win.ml" would be replaced with a > reference cell, and get_block_driver_priority / > set_block_driver_priority would manipulate that. References are really just structures with mutable fields and some syntactic sugar: https://github.com/ocaml/ocaml/blob/66db964f48869c6470b8ccd1ed672e244ba9d680/stdlib/stdlib.ml#L237 The reason mutable fields of structures in general have to be specially marked with the "mutable" keyword is because of the "remembered set", an obscure corner of generational garbage collection: https://www.cs.cornell.edu/courses/cs3110/2012sp/lectures/lec26-gc/lec26.html If mutable fields didn't exist then you could never have a pointer from the old generation into the new generation, because new generation objects are always younger than old generation objects, and as all fields have to be set at object creation time (no mutation), there cannot exist pointers from old to new. Since mutable fields _do_ exist, it's possible for such pointers to exist. When you do a minor GC of the younger generation, to avoid having to scan over the whole of the old heap to find these pointers, when a mutable field is mutated a record is added to the "remembered set", which is treated as an additional set of roots for the minor GC. After each minor GC the remembered set can be cleared. But there's still some overhead here, so marking fields in a struct as mutable is not cost free. (Of course in this specific case the cost is infinitesimally small, but in general it can be a concern.) This is a long way of saying that it's not a reference cell, but a plain field in the 't' struct, with extra runtime overhead and code generated for when it is mutated. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v2v] convert: Allow preferred block driver to be specified on the command line
On 3/6/23 17:52, Richard W.M. Jones wrote: > diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml > index 9d8d271d05..f36b486359 100644 > --- a/convert/convert_windows.ml > +++ b/convert/convert_windows.ml > @@ -38,7 +38,7 @@ module G = Guestfs > * time the Windows VM is booted on KVM. > *) > > -let convert (g : G.guestfs) _ inspect i_firmware _ static_ips = > +let convert (g : G.guestfs) _ inspect i_firmware block_driver _ static_ips = >(*--*) >(* Inspect the Windows guest. *) > > @@ -47,6 +47,16 @@ let convert (g : G.guestfs) _ inspect i_firmware _ > static_ips = > *) >let virtio_win = > Inject_virtio_win.from_environment g inspect.i_root Config.datadir in > + (match block_driver with > + | Virtio_blk -> () (* the default, no need to do anything *) > +(* > + | Virtio_scsi -> > + let drivers = Inject_virtio_win.get_block_driver_priority virtio_win in > + let drivers = "vioscsi" :: drivers in > + Inject_virtio_win.set_block_driver_priority virtio_win drivers > +*) > + | IDE -> assert false (* not possible - but maybe ...? *) > + ); > >(* If the Windows guest appears to be using group policy. > * So ["virtio_blk"; "vrtioblk"; "viostor"] in "common/mlcustomize/inject_virtio_win.ml" would be replaced with a reference cell, and get_block_driver_priority / set_block_driver_priority would manipulate that. This looks OK to me. That said, I'd prefer that this patch be split up a bit (by Andrey). Patch#1 could introduce the new field and make sure it is passed around (incl. setting the default virtio-blk value); that's purely boilerplate, so the patch would be well-focused. Patch #2 could glue the new field to the new Inject_virtio_win APIs, in "convert_windows.ml". Patch#3 could implement the command line changes and update the documentation. Laszlo ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH v2v] convert: Allow preferred block driver to be specified on the command line
---
docs/virt-v2v.pod | 10 ++
convert/convert.mli | 1 +
convert/convert_linux.mli | 3 ++-
convert/convert_windows.mli | 3 ++-
convert/convert.ml | 9 ++---
convert/convert_linux.ml| 2 +-
convert/convert_windows.ml | 12 +++-
in-place/in_place.ml| 3 ++-
inspector/inspector.ml | 3 ++-
v2v/v2v.ml | 12 +++-
10 files changed, 48 insertions(+), 10 deletions(-)
diff --git a/docs/virt-v2v.pod b/docs/virt-v2v.pod
index b458607d14..e096418b2c 100644
--- a/docs/virt-v2v.pod
+++ b/docs/virt-v2v.pod
@@ -207,6 +207,16 @@ The options are silently ignored for other input methods.
See I<--network> below.
+=item B<--block-driver virtio-blk>
+
+=item B<--block-driver virtio-scsi>
+
+When choosing a block driver for Windows guests, prefer C or
+C. The default is C.
+
+Note this has no effect for Linux guests at the moment. That may be
+added in future.
+
=item B<--colors>
=item B<--colours>
diff --git a/convert/convert.mli b/convert/convert.mli
index 3bd39e2da8..c63bf6f0da 100644
--- a/convert/convert.mli
+++ b/convert/convert.mli
@@ -17,6 +17,7 @@
*)
type options = {
+ block_driver : Types.guestcaps_block_type; (** [--block-driver] option *)
keep_serial_console : bool;
ks : Tools_utils.key_store; (** [--key] option *)
network_map : Networks.t;(** [-b] and [-n] options *)
diff --git a/convert/convert_linux.mli b/convert/convert_linux.mli
index 6eb272e9b4..dc6968fe51 100644
--- a/convert/convert_linux.mli
+++ b/convert/convert_linux.mli
@@ -23,5 +23,6 @@
Mint and Kali are supported by this module. *)
val convert : Guestfs.guestfs -> Types.source -> Types.inspect ->
- Firmware.i_firmware -> bool -> Types.static_ip list ->
+ Firmware.i_firmware -> Types.guestcaps_block_type ->
+ bool -> Types.static_ip list ->
Types.guestcaps
diff --git a/convert/convert_windows.mli b/convert/convert_windows.mli
index 42dac9f50c..33a14f6596 100644
--- a/convert/convert_windows.mli
+++ b/convert/convert_windows.mli
@@ -21,5 +21,6 @@
This module converts a Windows guest to run on KVM. *)
val convert : Guestfs.guestfs -> Types.source -> Types.inspect ->
- Firmware.i_firmware -> bool -> Types.static_ip list ->
+ Firmware.i_firmware -> Types.guestcaps_block_type ->
+ bool -> Types.static_ip list ->
Types.guestcaps
diff --git a/convert/convert.ml b/convert/convert.ml
index 0aa0e5cd3c..fa34d2ed7f 100644
--- a/convert/convert.ml
+++ b/convert/convert.ml
@@ -31,6 +31,7 @@ open Utils
module G = Guestfs
type options = {
+ block_driver : guestcaps_block_type;
keep_serial_console : bool;
ks : key_store;
network_map : Networks.t;
@@ -88,7 +89,7 @@ let rec convert dir options source =
(* Conversion. *)
let guestcaps =
do_convert g source inspect i_firmware
- options.keep_serial_console options.static_ips in
+ options.block_driver options.keep_serial_console options.static_ips in
g#umount_all ();
@@ -221,7 +222,8 @@ and do_fstrim g inspect =
) fses
(* Conversion. *)
-and do_convert g source inspect i_firmware keep_serial_console interfaces =
+and do_convert g source inspect i_firmware
+ block_driver keep_serial_console interfaces =
(match inspect.i_product_name with
| "unknown" ->
message (f_"Converting the guest to run on KVM")
@@ -246,7 +248,8 @@ and do_convert g source inspect i_firmware
keep_serial_console interfaces =
inspect.i_type inspect.i_distro in
debug "picked conversion module %s" conversion_name;
let guestcaps =
-convert g source inspect i_firmware keep_serial_console interfaces in
+convert g source inspect i_firmware
+block_driver keep_serial_console interfaces in
debug "%s" (string_of_guestcaps guestcaps);
(* Did we manage to install virtio drivers? *)
diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
index d5c0f24dbb..0f25af0eab 100644
--- a/convert/convert_linux.ml
+++ b/convert/convert_linux.ml
@@ -34,7 +34,7 @@ open Linux_kernels
module G = Guestfs
(* The conversion function. *)
-let convert (g : G.guestfs) source inspect i_firmware keep_serial_console _ =
+let convert (g : G.guestfs) source inspect i_firmware _ keep_serial_console _ =
(*--*)
(* Inspect the guest first. We already did some basic inspection in
* the common v2v.ml code, but that has to deal with generic guests
diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml
index 9d8d271d05..f36b486359 100644
--- a/convert/convert_windows.ml
+++ b/convert/convert_windows.ml
@@ -38,7 +38,7 @@ module G = Guestfs
* time the Windows VM is booted on KVM.
*)
-let convert (g : G.guestfs) _ inspect i_firmware _ static_ips =
+let convert (g : G.guestfs) _ inspect i_firmware block_driver _ static_ip
[Libguestfs] [PATCH v2v] convert: Allow preferred block driver to be specified on the command line
This is just an outline patch, only compile tested. It doesn't make changes to virt-v2v-in-place, but those would be the same as made in v2v/v2v.ml. It reuses the existing Types.guestcaps_block_type which is a bit ugly but fairly practical. I've made the change to the documentation, but it needs a test. Rich. ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
