replay additions

From: Pavel Dovgalyuk <[email protected]>

Asynchronous events saved at checkpoints may invoke callbacks when processed. These callbacks may also generate/read new events (e.g. clock reads). Therefore event processing flag must be reset before callback invocation.

Signed-off-by Pavel Dovgalyuk <[email protected]>

---
 replay/replay-events.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

replay/replay-events.c +0 -0

index 94a6dcccfc..768b505f3d 100644
@@ -295,13 +295,13 @@ void replay_read_events(int checkpoint)
         if (!event) {
             break;

replay/replay-events.c +0 -0

     }
 }
 

From: Paolo Bonzini [mailto:[email protected]]

Quoted 26 lines · Pavel Dovgalyuk

From: Pavel Dovgalyuk <[email protected]>

Asynchronous events saved at checkpoints may invoke callbacks when processed. These callbacks may also generate/read new events (e.g. clock reads). Therefore event processing flag must be reset before callback invocation.

Signed-off-by Pavel Dovgalyuk <[email protected]>

---
 replay/replay-events.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

replay/replay-events.c +2 -0

index 94a6dcccfc..768b505f3d 100644
@@ -295,13 +295,13 @@ void replay_read_events(int checkpoint)
         if (!event) {
             break;
         }
+        replay_finish_event();
+        read_event_kind = -1;
         replay_mutex_unlock();
         replay_run_event(event);
         replay_mutex_lock();
 

replay/replay-events.c void replay_read_events(int checkpoint) · line 304

Quoted 3 lines

replay/replay-events.c +0 -2

         g_free(event);
-        replay_finish_event();
-        read_event_kind = -1;

While at it, g_free can be moved outside the lock.

Also, replay_flush_events and replay_save_events are leaving the event in events_list while releasing the lock, and only doing QTAILQ_REMOVE after taking it back. This can cause events to be processed twice.

From: Pavel Dovgalyuk <[email protected]>

This patch does not allows saving/loading vmstate when replay events queue is not empty. There is no reliable way to save events queue, because it describes internal coroutine state. Therefore saving and loading operations should be deferred to another record/replay step.

Signed-off-by Pavel Dovgalyuk <[email protected]>

From: Juan Quintela [mailto:[email protected]]

Quoted 9 lines · Pavel Dovgalyuk

From: Pavel Dovgalyuk <[email protected]>

This patch does not allows saving/loading vmstate when replay events queue is not empty. There is no reliable way to save events queue, because it describes internal coroutine state. Therefore saving and loading operations should be deferred to another record/replay step.

Signed-off-by Pavel Dovgalyuk <[email protected]>

This functions have changed, see last series (but change is trivial from monitor_printf to error_<something>)

migration/savevm.c int save_vmstate(Monitor *mon, const char *name) · line 2092

Quoted 10 lines

migration/savevm.c +6 -0

@@ -2083,6 +2084,12 @@ int save_vmstate(Monitor *mon, const char *name)
     Error *local_err = NULL;
     AioContext *aio_context;
 
+    if (!replay_can_snapshot()) {
+        monitor_printf(mon, "Record/replay does not allow making snapshot right now. "
+                        "Try stopping at another step.\n");
+        return ret;
+    }
+

To issue a savevm/loadvm the user don't have to stop qemu, so I think we can improve the message to something les in both places?

"Try saving/loading later"?

From: Pavel Dovgalyuk <[email protected]>

This patch does not allows saving/loading vmstate when replay events queue is not empty. There is no reliable way to save events queue, because it describes internal coroutine state. Therefore saving and loading operations should be deferred to another record/replay step.

Signed-off-by Pavel Dovgalyuk <[email protected]>

---
 include/sysemu/replay.h  |    3 +++
 migration/savevm.c       |   13 +++++++++++++
 replay/replay-snapshot.c |    6 ++++++
 3 files changed, 22 insertions(+)

include/sysemu/replay.h +3 -0

index f1c0712795..5c9db2a2ef 100644
@@ -164,5 +164,8 @@ void replay_audio_in(int *recorded, void *samples, int *wpos, int size);
 /*! Called at the start of execution.
     Loads or saves initial vmstate depending on execution mode. */
 void replay_vmstate_init(void);
+/*! Called to ensure that replay state is consistent and VM snapshot
+    can be created */
+bool replay_can_snapshot(void);
 
 #endif

migration/savevm.c +13 -0

index 03ae1bdeb4..358d22170d 100644
@@ -54,6 +54,7 @@
 #include "qemu/cutils.h"
 #include "io/channel-buffer.h"
 #include "io/channel-file.h"
+#include "sysemu/replay.h"
 
 #ifndef ETH_P_RARP
 #define ETH_P_RARP 0x8035

@@ -2083,6 +2084,12 @@ int save_vmstate(Monitor *mon, const char *name)
     Error *local_err = NULL;
     AioContext *aio_context;
 
+    if (!replay_can_snapshot()) {
+        monitor_printf(mon, "Record/replay does not allow making snapshot right now. "
+                        "Try stopping at another step.\n");
+        return ret;
+    }
+
     if (!bdrv_all_can_snapshot(&bs)) {
         monitor_printf(mon, "Device '%s' is writable but does not "
                        "support snapshots.\n", bdrv_get_device_name(bs));

@@ -2244,6 +2251,12 @@ int load_vmstate(const char *name)
     AioContext *aio_context;
     MigrationIncomingState *mis = migration_incoming_get_current();
 
+    if (!replay_can_snapshot()) {
+        error_report("Record/replay does not allow loading snapshot right now. "
+                     "Try stopping at another step.\n");
+        return -EINVAL;
+    }
+
     if (!bdrv_all_can_snapshot(&bs)) {
         error_report("Device '%s' is writable but does not support snapshots.",
                      bdrv_get_device_name(bs));

replay/replay-snapshot.c +6 -0

index 65e2d375c2..438d82ec33 100644
@@ -76,3 +76,9 @@ void replay_vmstate_init(void)
         }
     }
 }
+
+bool replay_can_snapshot(void)
+{
+    return replay_mode == REPLAY_MODE_NONE
+        || !replay_has_events();
+}

From: Paolo Bonzini [mailto:[email protected]]

Quoted 7 lines · Pavel Dovgalyuk

From: Pavel Dovgalyuk <[email protected]>

This patch does not allows saving/loading vmstate when replay events queue is not empty. There is no reliable way to save events queue, because it describes internal coroutine state. Therefore saving and loading operations should be deferred to another record/replay step.

Can it actually be non-empty after bdrv_drain_all?

Quoted 65 lines

Signed-off-by Pavel Dovgalyuk <[email protected]>

---
 include/sysemu/replay.h  |    3 +++
 migration/savevm.c       |   13 +++++++++++++
 replay/replay-snapshot.c |    6 ++++++
 3 files changed, 22 insertions(+)

include/sysemu/replay.h +3 -0

index f1c0712795..5c9db2a2ef 100644
@@ -164,5 +164,8 @@ void replay_audio_in(int *recorded, void *samples, int *wpos, int size);
 /*! Called at the start of execution.
     Loads or saves initial vmstate depending on execution mode. */
 void replay_vmstate_init(void);
+/*! Called to ensure that replay state is consistent and VM snapshot
+    can be created */
+bool replay_can_snapshot(void);
 
 #endif

migration/savevm.c +13 -0

index 03ae1bdeb4..358d22170d 100644
@@ -54,6 +54,7 @@
 #include "qemu/cutils.h"
 #include "io/channel-buffer.h"
 #include "io/channel-file.h"
+#include "sysemu/replay.h"
 
 #ifndef ETH_P_RARP
 #define ETH_P_RARP 0x8035

@@ -2083,6 +2084,12 @@ int save_vmstate(Monitor *mon, const char *name)
     Error *local_err = NULL;
     AioContext *aio_context;
 
+    if (!replay_can_snapshot()) {
+        monitor_printf(mon, "Record/replay does not allow making snapshot right now. "
+                        "Try stopping at another step.\n");
+        return ret;
+    }
+
     if (!bdrv_all_can_snapshot(&bs)) {
         monitor_printf(mon, "Device '%s' is writable but does not "
                        "support snapshots.\n", bdrv_get_device_name(bs));

@@ -2244,6 +2251,12 @@ int load_vmstate(const char *name)
     AioContext *aio_context;
     MigrationIncomingState *mis = migration_incoming_get_current();
 
+    if (!replay_can_snapshot()) {
+        error_report("Record/replay does not allow loading snapshot right now. "
+                     "Try stopping at another step.\n");
+        return -EINVAL;
+    }
+
     if (!bdrv_all_can_snapshot(&bs)) {
         error_report("Device '%s' is writable but does not support snapshots.",
                      bdrv_get_device_name(bs));

replay/replay-snapshot.c +0 -0

index 65e2d375c2..438d82ec33 100644
@@ -76,3 +76,9 @@ void replay_vmstate_init(void)
         }
     }

replay/replay-snapshot.c void replay_vmstate_init(void) · line 84

Quoted 8 lines

replay/replay-snapshot.c +6 -0

 }
+
+bool replay_can_snapshot(void)
+{
+    return replay_mode == REPLAY_MODE_NONE
+        || !replay_has_events();
+}

Quoted 7 lines

Quoted 5 lines

This patch does not allows saving/loading vmstate when replay events queue is not empty. There is no reliable way to save events queue, because it describes internal coroutine state. Therefore saving and loading operations should be deferred to another record/replay step.

Can it actually be non-empty after bdrv_drain_all?

drain/flush cannot succeed, because started requests are prisoned in the replay events queue.

From: Paolo Bonzini [mailto:[email protected]]

Quoted 6 lines · Pavel Dovgalyuk

Quoted 6 lines

Quoted 5 lines

This patch does not allows saving/loading vmstate when replay events queue is not empty. There is no reliable way to save events queue, because it describes internal coroutine state. Therefore saving and loading operations should be deferred to another record/replay step.

Quoted 4 lines

Quoted 1 line

Can it actually be non-empty after bdrv_drain_all?

drain/flush cannot succeed, because started requests are prisoned in the replay events queue.

But that would apply to loading only. Saving should still be always possible.

Quoted 10 lines

From: Paolo Bonzini [mailto:[email protected]]

Quoted 8 lines · Pavel Dovgalyuk

Quoted 7 lines

Quoted 5 lines

This patch does not allows saving/loading vmstate when replay events queue is not empty. There is no reliable way to save events queue, because it describes internal coroutine state. Therefore saving and loading operations should be deferred to another record/replay step.

Can it actually be non-empty after bdrv_drain_all?

Quoted 5 lines

Quoted 2 lines

drain/flush cannot succeed, because started requests are prisoned in the replay events queue.

But that would apply to loading only. Saving should still be always possible.

We can save it. But it wouldn't load correctly - replay queue will be empty after loading.

From: Paolo Bonzini [mailto:[email protected]]

Quoted 13 lines · Pavel Dovgalyuk

Quoted 13 lines

From: Paolo Bonzini [mailto:[email protected]]

Quoted 10 lines · Pavel Dovgalyuk

Quoted 7 lines

Quoted 5 lines

This patch does not allows saving/loading vmstate when replay events queue is not empty. There is no reliable way to save events queue, because it describes internal coroutine state. Therefore saving and loading operations should be deferred to another record/replay step.

Can it actually be non-empty after bdrv_drain_all?

drain/flush cannot succeed, because started requests are prisoned in the replay events queue.

Quoted 4 lines

Quoted 2 lines

But that would apply to loading only. Saving should still be always possible.

We can save it. But it wouldn't load correctly - replay queue will be empty after loading.

When saving you can drain, and then the events queue should be empty. Or I am misunderstanding how it works, which is possible too.

Quoted 20 lines

From: Paolo Bonzini [mailto:[email protected]]

Quoted 17 lines · Pavel Dovgalyuk

Quoted 8 lines

From: Paolo Bonzini [mailto:[email protected]]

Quoted 6 lines · Pavel Dovgalyuk

Quoted 6 lines

Quoted 5 lines

This patch does not allows saving/loading vmstate when replay events queue is not empty. There is no reliable way to save events queue, because it describes internal coroutine state. Therefore saving and loading operations should be deferred to another record/replay step.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Can it actually be non-empty after bdrv_drain_all?

drain/flush cannot succeed, because started requests are prisoned in the replay events queue.

But that would apply to loading only. Saving should still be always possible.

We can save it. But it wouldn't load correctly - replay queue will be empty after loading.

Quoted 2 lines

When saving you can drain, and then the events queue should be empty. Or I am misunderstanding how it works, which is possible too.

Drain will wait until the queue becomes empty.
Queue is processed only at checkpoints.
Checkpoints are met in iothread (at timers processing and so on). But iothread is waiting for finishing the drain.

From: Paolo Bonzini [mailto:[email protected]]

Quoted 23 lines · Pavel Dovgalyuk

Quoted 22 lines

From: Paolo Bonzini [mailto:[email protected]]

Quoted 10 lines · Pavel Dovgalyuk

Quoted 10 lines

From: Paolo Bonzini [mailto:[email protected]]

Quoted 8 lines · Pavel Dovgalyuk

Quoted 7 lines

Quoted 5 lines

This patch does not allows saving/loading vmstate when replay events queue is not empty. There is no reliable way to save events queue, because it describes internal coroutine state. Therefore saving and loading operations should be deferred to another record/replay step.

Can it actually be non-empty after bdrv_drain_all?

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

drain/flush cannot succeed, because started requests are prisoned in the replay events queue.

But that would apply to loading only. Saving should still be always possible.

We can save it. But it wouldn't load correctly - replay queue will be empty after loading.

When saving you can drain, and then the events queue should be empty. Or I am misunderstanding how it works, which is possible too.

Quoted 4 lines

Drain will wait until the queue becomes empty.
Queue is processed only at checkpoints.
Checkpoints are met in iothread (at timers processing and so on). But iothread is waiting for finishing the drain.

What checkpoint can hang the drain during a save?

Quoted 30 lines

From: Paolo Bonzini [mailto:[email protected]]

Quoted 27 lines · Pavel Dovgalyuk

Quoted 15 lines

From: Paolo Bonzini [mailto:[email protected]]

Quoted 13 lines · Pavel Dovgalyuk

Quoted 13 lines

From: Paolo Bonzini [mailto:[email protected]]

Quoted 10 lines · Pavel Dovgalyuk

Quoted 7 lines

Quoted 5 lines

This patch does not allows saving/loading vmstate when replay events queue is not empty. There is no reliable way to save events queue, because it describes internal coroutine state. Therefore saving and loading operations should be deferred to another record/replay step.

Can it actually be non-empty after bdrv_drain_all?

drain/flush cannot succeed, because started requests are prisoned in the replay events queue.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

But that would apply to loading only. Saving should still be always possible.

We can save it. But it wouldn't load correctly - replay queue will be empty after loading.

When saving you can drain, and then the events queue should be empty. Or I am misunderstanding how it works, which is possible too.

Drain will wait until the queue becomes empty.
Queue is processed only at checkpoints.
Checkpoints are met in iothread (at timers processing and so on). But iothread is waiting for finishing the drain.

Quoted 1 line

What checkpoint can hang the drain during a save?

No one. There are no checkpoins during vmsave and during drain. Therefore the queue will never become empty.

From: Paolo Bonzini [mailto:[email protected]]

Quoted 14 lines · Pavel Dovgalyuk

Quoted 14 lines

From: Paolo Bonzini [mailto:[email protected]]

Quoted 12 lines · Pavel Dovgalyuk

Quoted 12 lines

From: Paolo Bonzini [mailto:[email protected]]

Quoted 10 lines · Pavel Dovgalyuk

Quoted 10 lines

From: Paolo Bonzini [mailto:[email protected]]

Quoted 8 lines · Pavel Dovgalyuk

Quoted 7 lines

Quoted 5 lines

This patch does not allows saving/loading vmstate when replay events queue is not empty. There is no reliable way to save events queue, because it describes internal coroutine state. Therefore saving and loading operations should be deferred to another record/replay step.

Can it actually be non-empty after bdrv_drain_all?

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

drain/flush cannot succeed, because started requests are prisoned in the replay events queue.

But that would apply to loading only. Saving should still be always possible.

We can save it. But it wouldn't load correctly - replay queue will be empty after

loading.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

Quoted 3 lines

When saving you can drain, and then the events queue should be empty. Or I am misunderstanding how it works, which is possible too.

Drain will wait until the queue becomes empty.
Queue is processed only at checkpoints.
Checkpoints are met in iothread (at timers processing and so on). But iothread is waiting for finishing the drain.

Quoted 4 lines

Quoted 1 line

What checkpoint can hang the drain during a save?

No one. There are no checkpoins during vmsave and during drain. Therefore the queue will never become empty.

Understood. And what checkpoint will you be waiting for during drain, causing a deadlock?

Quoted 2 lines

Understood. And what checkpoint will you be waiting for during drain, causing a deadlock?

Every checkpoint processes the queue, but none of them are invoked,
because iothread (which invokes all the checkpoints) is waiting for the end of the drain.

And we cannot add a checkpoint into the drain, because then vmstop will affect the behavior of the guest.

From: Paolo Bonzini [mailto:[email protected]]

Quoted 5 lines · Pavel Dovgalyuk

Quoted 2 lines

Understood. And what checkpoint will you be waiting for during drain, causing a deadlock?

Every checkpoint processes the queue, but none of them are invoked,
because iothread (which invokes all the checkpoints) is waiting for the end of the drain.

Quoted 2 lines

And we cannot add a checkpoint into the drain, because then vmstop will affect the behavior of the guest.

But it does affect the behavior already when RR is off. And it shouldn't be a problem if it does in record mode (as long as the effect can be replayed).

If RR cannot do drain reliably, not even in record mode, that is a huge problem because a lot of block layer operations assume they can.