| Message ID | 20190313084330.16015-1-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vl: Fix to create migration object before block backends again | expand |
Am 13.03.2019 um 09:43 hat Markus Armbruster geschrieben: > Recent commit cda4aa9a5a0 moved block backend creation before machine > property evaluation. This broke qemu-iotests 055. Turns out we need > to create the migration object before block backends, so block > backends can add migration blockers. Fix by calling > migration_object_init() earlier, right before configure_blockdev(). > > Fixes: cda4aa9a5a08777cf13e164c0543bd4888b8adce > Reported-by: Kevin Wolf <kwolf@redhat.com> > Signed-off-by: Markus Armbruster <armbru@redhat.com> Thanks, applied to the block branch. Kevin
On Wed, Mar 13, 2019 at 09:43:30AM +0100, Markus Armbruster wrote: > Recent commit cda4aa9a5a0 moved block backend creation before machine > property evaluation. This broke qemu-iotests 055. Turns out we need > to create the migration object before block backends, so block > backends can add migration blockers. Fix by calling > migration_object_init() earlier, right before configure_blockdev(). > > Fixes: cda4aa9a5a08777cf13e164c0543bd4888b8adce > Reported-by: Kevin Wolf <kwolf@redhat.com> > Signed-off-by: Markus Armbruster <armbru@redhat.com> Hi, After this patch is applied, migration with Xen doesn't work anymore. When QEMU on the receiving end is started, it prints: qemu-system-i386: load of migration failed: Invalid argument > + /* > + * Migration object can only be created after global properties > + * are applied correctly. > + */ > + migration_object_init(); I think it's because migration_object_init() is now called before configure_accelerator(). xen_accel_class_init() have some compat_props for migration. Any idea how to fix this? Thanks,
On Tue, Mar 26, 2019 at 03:58:52PM +0000, Anthony PERARD wrote: > On Wed, Mar 13, 2019 at 09:43:30AM +0100, Markus Armbruster wrote: > > Recent commit cda4aa9a5a0 moved block backend creation before machine > > property evaluation. This broke qemu-iotests 055. Turns out we need > > to create the migration object before block backends, so block > > backends can add migration blockers. Fix by calling > > migration_object_init() earlier, right before configure_blockdev(). > > > > Fixes: cda4aa9a5a08777cf13e164c0543bd4888b8adce > > Reported-by: Kevin Wolf <kwolf@redhat.com> > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > > Hi, > > After this patch is applied, migration with Xen doesn't work anymore. > When QEMU on the receiving end is started, it prints: > qemu-system-i386: load of migration failed: Invalid argument I should have quote this from QEMU stderr instead of the single line abrove: qemu-system-i386: Configuration section missing qemu-system-i386: load of migration failed: Invalid argument > > + /* > > + * Migration object can only be created after global properties > > + * are applied correctly. > > + */ > > + migration_object_init(); > > I think it's because migration_object_init() is now called before > configure_accelerator(). xen_accel_class_init() have some compat_props > for migration. > > Any idea how to fix this?
Anthony PERARD <anthony.perard@citrix.com> writes: > On Tue, Mar 26, 2019 at 03:58:52PM +0000, Anthony PERARD wrote: >> On Wed, Mar 13, 2019 at 09:43:30AM +0100, Markus Armbruster wrote: >> > Recent commit cda4aa9a5a0 moved block backend creation before machine >> > property evaluation. This broke qemu-iotests 055. Turns out we need >> > to create the migration object before block backends, so block >> > backends can add migration blockers. Fix by calling >> > migration_object_init() earlier, right before configure_blockdev(). >> > >> > Fixes: cda4aa9a5a08777cf13e164c0543bd4888b8adce >> > Reported-by: Kevin Wolf <kwolf@redhat.com> >> > Signed-off-by: Markus Armbruster <armbru@redhat.com> >> >> Hi, >> >> After this patch is applied, migration with Xen doesn't work anymore. >> When QEMU on the receiving end is started, it prints: >> qemu-system-i386: load of migration failed: Invalid argument > > I should have quote this from QEMU stderr instead of the single line > abrove: > qemu-system-i386: Configuration section missing > qemu-system-i386: load of migration failed: Invalid argument > >> > + /* >> > + * Migration object can only be created after global properties >> > + * are applied correctly. >> > + */ >> > + migration_object_init(); >> >> I think it's because migration_object_init() is now called before >> configure_accelerator(). xen_accel_class_init() have some compat_props >> for migration. I see. Anything that gets created before a certain compat property becomes known silently ignores the compat property[*]. Accelerator compat properties become known when we select the accelerator. This is awfully late, because we want to pick the first accelerator that initializes, which requires a machine. >> Any idea how to fix this? Known dependencies: * migration object_init() must run before configure_blockdev(), so block backends can register migration blockers * configure_blockdev() must run before machine_set_property(), so machine properties can refer to block backends. * configure_accelerator() must run after machine creation, because it needs a machine * configure_accelerator() must run before migration_object_init(), so migration_object_init() can see accelerator compat properties. We're currently violating the last one, because weren't aware of it. To fix that, rejigger some more: move configure_accelerator() before migration_object_init() (with a suitable comment), then see what else breaks. Could you try the appended patch and report whether it fixes your problem? [*] Silent ignore is nasty. I got experimental code to turn it into a hard failure. diff --git a/vl.c b/vl.c index d61d5604e5..28b9e9a170 100644 --- a/vl.c +++ b/vl.c @@ -4276,6 +4276,12 @@ int main(int argc, char **argv, char **envp) exit(0); } + /* + * Must run before migration_object_init() to make Xen's + * accelerator compat properties stick + */ + configure_accelerator(current_machine, argv[0]); + /* * Migration object can only be created after global properties * are applied correctly. @@ -4297,8 +4303,6 @@ int main(int argc, char **argv, char **envp) current_machine->maxram_size = maxram_size; current_machine->ram_slots = ram_slots; - configure_accelerator(current_machine, argv[0]); - if (!qtest_enabled() && machine_class->deprecation_reason) { error_report("Machine type '%s' is deprecated: %s", machine_class->name, machine_class->deprecation_reason);
On Tue, Mar 26, 2019 at 06:48:54PM +0100, Markus Armbruster wrote: > Known dependencies: > > * migration object_init() must run before configure_blockdev(), so block > backends can register migration blockers > > * configure_blockdev() must run before machine_set_property(), so > machine properties can refer to block backends. > > * configure_accelerator() must run after machine creation, because it > needs a machine > > * configure_accelerator() must run before migration_object_init(), so > migration_object_init() can see accelerator compat properties. > > We're currently violating the last one, because weren't aware of it. To > fix that, rejigger some more: move configure_accelerator() before > migration_object_init() (with a suitable comment), then see what else > breaks. Could you try the appended patch and report whether it fixes > your problem? Yes, that patch works and doesn't break anything else with Xen. > diff --git a/vl.c b/vl.c > index d61d5604e5..28b9e9a170 100644 > --- a/vl.c > +++ b/vl.c > @@ -4276,6 +4276,12 @@ int main(int argc, char **argv, char **envp) > exit(0); > } > > + /* > + * Must run before migration_object_init() to make Xen's > + * accelerator compat properties stick > + */ > + configure_accelerator(current_machine, argv[0]); > + > /* > * Migration object can only be created after global properties > * are applied correctly. > @@ -4297,8 +4303,6 @@ int main(int argc, char **argv, char **envp) > current_machine->maxram_size = maxram_size; > current_machine->ram_slots = ram_slots; > > - configure_accelerator(current_machine, argv[0]); > - > if (!qtest_enabled() && machine_class->deprecation_reason) { > error_report("Machine type '%s' is deprecated: %s", > machine_class->name, machine_class->deprecation_reason);
Anthony PERARD <anthony.perard@citrix.com> writes: > On Tue, Mar 26, 2019 at 06:48:54PM +0100, Markus Armbruster wrote: >> Known dependencies: >> >> * migration object_init() must run before configure_blockdev(), so block >> backends can register migration blockers >> >> * configure_blockdev() must run before machine_set_property(), so >> machine properties can refer to block backends. >> >> * configure_accelerator() must run after machine creation, because it >> needs a machine >> >> * configure_accelerator() must run before migration_object_init(), so >> migration_object_init() can see accelerator compat properties. >> >> We're currently violating the last one, because weren't aware of it. To >> fix that, rejigger some more: move configure_accelerator() before >> migration_object_init() (with a suitable comment), then see what else >> breaks. Could you try the appended patch and report whether it fixes >> your problem? > > Yes, that patch works and doesn't break anything else with Xen. Thanks! I posted this fix together with another one as "[PATCH 0/2] Compat props bug fixes". I fogot to add your Tested-by, sorry.
diff --git a/vl.c b/vl.c index 027b853d92..b3e48eff27 100644 --- a/vl.c +++ b/vl.c @@ -4276,10 +4276,17 @@ int main(int argc, char **argv, char **envp) exit(0); } + /* + * Migration object can only be created after global properties + * are applied correctly. + */ + migration_object_init(); + /* * Note: we need to create block backends before * machine_set_property(), so machine properties can refer to - * them. + * them, and after migration_object_init(), so we can create + * migration blockers. */ configure_blockdev(&bdo_queue, machine_class, snapshot); @@ -4297,12 +4304,6 @@ int main(int argc, char **argv, char **envp) machine_class->name, machine_class->deprecation_reason); } - /* - * Migration object can only be created after global properties - * are applied correctly. - */ - migration_object_init(); - if (qtest_chrdev) { qtest_init(qtest_chrdev, qtest_log, &error_fatal); }
Recent commit cda4aa9a5a0 moved block backend creation before machine property evaluation. This broke qemu-iotests 055. Turns out we need to create the migration object before block backends, so block backends can add migration blockers. Fix by calling migration_object_init() earlier, right before configure_blockdev(). Fixes: cda4aa9a5a08777cf13e164c0543bd4888b8adce Reported-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com> --- vl.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)