On 14.02.20 12:08, David Hildenbrand wrote:
> On 14.02.20 12:02, Dr. David Alan Gilbert wrote:
>> * David Hildenbrand (da...@redhat.com) wrote:
>>> On 14.02.20 11:42, Dr. David Alan Gilbert wrote:
>>>> * David Hildenbrand (da...@redhat.com) wrote:
>>>>> On 14.02.20 11:25, Dr. David Alan Gilbert wrote:
>>>>>> * David Hildenbrand (da...@redhat.com) wrote:
>>>>>>> Resizing while migrating is dangerous and does not work as expected.
>>>>>>> The whole migration code works on the usable_length of ram blocks and
>>>>>>> does
>>>>>>> not expect this to change at random points in time.
>>>>>>>
>>>>>>> Precopy: The ram block size must not change on the source, after
>>>>>>> ram_save_setup(), so as long as the guest is still running on the
>>>>>>> source.
>>>>>>>
>>>>>>> Postcopy: The ram block size must not change on the target, after
>>>>>>> synchronizing the RAM block list (ram_load_precopy()).
>>>>>>>
>>>>>>> AFAIKS, resizing can be trigger *after* (but not during) a reset in
>>>>>>> ACPI code by the guest
>>>>>>> - hw/arm/virt-acpi-build.c:acpi_ram_update()
>>>>>>> - hw/i386/acpi-build.c:acpi_ram_update()
>>>>>>>
>>>>>>> I see no easy way to work around this. Fail hard instead of failing
>>>>>>> somewhere in migration code due to strange other reasons. AFAIKs, the
>>>>>>> rebuilts will be triggered during reboot, so this should not affect
>>>>>>> running guests, but only guests that reboot at a very bad time and
>>>>>>> actually require size changes.
>>>>>>>
>>>>>>> Let's further limit the impact by checking if an actual resize of the
>>>>>>> RAM (in number of pages) is required.
>>>>>>>
>>>>>>> Don't perform the checks in qemu_ram_resize(), as that's called during
>>>>>>> migration when syncing the used_length. Update documentation.
>>>>>>
>>>>>> Interesting; we need to do something about this - but banning resets
>>>>>> during migration is a bit harsh; and aborting the source VM is really
>>>>>> nasty - for a precopy especially we shouldn't kill the source VM,
>>>>>> we should just abort the migration.
>>>>>
>>>>> Any alternative, easy solutions to handle this? I do wonder how often
>>>>> this will actually trigger in real life.
>>>>
>>>> Well it's not that hard to abort a migration (I'm not sure we've got a
>>>> convenient wrapper to do it - but it shouldn't be hard to add).
>>>>
>>>
>>> We do have qmp_migrate_cancel(). I hope that can be called under BQL.
>>
>> Well it's a monitor command so I think so; although it's not really
>> designed for an error - it's a user action. Doing a
>> migrate_set_error(..) followed by a qemu_file_shutdown is probably a
>> good bet.
>
> I'll base on "[PATCH v2 fixed 00/16] Ram blocks with resizable anonymous
> allocations under POSIX", where I extend the ram block notifier with a
> resize notification.
>
> migrate/ram.c can register the notifier and react accordingly. E.g., for
> precopy, abort migration. Not sure about postcopy (below).
>
>>
>>> Can that be called in both, precopy and postcopy case? I assume in the
>>> precopy, it's easy.
>>
>> The problem is during postcopy you're toast when that happens because
>> you can't restart; however, can this happen once we're actually in
>> postcopy? It's a little different - if it happens before the transition
>> to postcopy then it's the same as precopy; if it happens afterwards..
>> well it's going to happen ont he destination side and that's quite
>> different.
>
> If it happens after, we are in trouble at least with received bitmaps.
> Not sure about other issues (it's a lot of code :) ). Especially
> shrinking while trying to place pages will be bad and fail. It's code
> that assumes used_length won't change.
>
> ramblock_recv_bitmap_send() on the target and ram_dirty_bitmap_reload()
> on the source. ram_dirty_bitmap_reload() will bail out if the sizes
> don't match.
>
So, with (modified) ram block notifiers it could look something like this:
>From c0049ac2e95d6756037db918852c507fb88297d9 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <da...@redhat.com>
Date: Fri, 14 Feb 2020 13:01:03 +0100
Subject: [PATCH v1] tmp
Signed-off-by: David Hildenbrand <da...@redhat.com>
---
migration/migration.c | 9 +++++++--
migration/migration.h | 1 +
migration/ram.c | 42 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 50 insertions(+), 2 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 3a21a4686c..0e7efe2920 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -175,13 +175,18 @@ void migration_object_init(void)
}
}
+void migration_cancel(void)
+{
+ migrate_fd_cancel(current_migration);
+}
+
void migration_shutdown(void)
{
/*
* Cancel the current migration - that will (eventually)
* stop the migration using this structure
*/
- migrate_fd_cancel(current_migration);
+ migration_cancel();
object_unref(OBJECT(current_migration));
}
@@ -2019,7 +2024,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
void qmp_migrate_cancel(Error **errp)
{
- migrate_fd_cancel(migrate_get_current());
+ migration_cancel();
}
void qmp_migrate_continue(MigrationStatus state, Error **errp)
diff --git a/migration/migration.h b/migration/migration.h
index 8473ddfc88..79fd74afa5 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -343,5 +343,6 @@ int foreach_not_ignored_block(RAMBlockIterFunc func, void
*opaque);
void migration_make_urgent_request(void);
void migration_consume_urgent_request(void);
bool migration_rate_limit(void);
+void migration_cancel(void);
#endif
diff --git a/migration/ram.c b/migration/ram.c
index ed23ed1c7c..f86f32b453 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -52,6 +52,7 @@
#include "migration/colo.h"
#include "block.h"
#include "sysemu/sysemu.h"
+#include "sysemu/runstate.h"
#include "savevm.h"
#include "qemu/iov.h"
#include "multifd.h"
@@ -3710,8 +3711,49 @@ static SaveVMHandlers savevm_ram_handlers = {
.resume_prepare = ram_resume_prepare,
};
+static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
+ size_t old_size, size_t new_size)
+{
+ /*
+ * We don't care about resizes triggered on incoming migration (when
+ * syncing ram blocks), or of course, when no migration is going on.
+ */
+ if (migration_is_idle() || !runstate_is_running()) {
+ return;
+ }
+
+ if (!postcopy_is_running()) {
+ Error *err = NULL;
+
+ /*
+ * Precopy code cannot deal with the size of ram blocks changing at
+ * random points in time. We're still running on the source, abort
+ * the migration and continue running here. Make sure to wait until
+ * migration was canceled.
+ */
+ error_setg(&err, "RAM resized during precopy.");
+ migrate_set_error(migrate_get_current(), err);
+ error_free(err);
+ migration_cancel();
+ } else {
+ /*
+ * Postcopy code cannot deal with the size of ram blocks changing at
+ * random points in time. We're running on the target. Fail hard.
+ *
+ * TODO: How to handle this in a better way?
+ */
+ error_report("RAM resized during postcopy.");
+ exit(-1);
+ }
+}
+
+static RAMBlockNotifier ram_mig_ram_notifier = {
+ .ram_block_resized = ram_mig_ram_block_resized,
+};
+
void ram_mig_init(void)
{
qemu_mutex_init(&XBZRLE.lock);
register_savevm_live("ram", 0, 4, &savevm_ram_handlers, &ram_state);
+ ram_block_notifier_add(&ram_mig_ram_notifier);
}
--
2.24.1
Thoughts?
--
Thanks,
David / dhildenb
