Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[David Hildenbrand <david@redhat.com>]

On 14.02.20 12:08, David Hildenbrand wrote:
> On 14.02.20 12:02, Dr. David Alan Gilbert wrote:
>> * David Hildenbrand (david@redhat.com) wrote:
>>> On 14.02.20 11:42, Dr. David Alan Gilbert wrote:
>>>> * David Hildenbrand (david@redhat.com) wrote:
>>>>> On 14.02.20 11:25, Dr. David Alan Gilbert wrote:
>>>>>> * David Hildenbrand (david@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 <david@redhat.com>
Date: Fri, 14 Feb 2020 13:01:03 +0100
Subject: [PATCH v1] tmp

Signed-off-by: David Hildenbrand <david@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