| Message ID | 20170504084210.7488.37561.stgit@PASHA-ISP |
|---|---|
| State | New |
| Headers | show |
Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> wrote: > From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> > > 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 <pavel.dovgaluk@ispras.ru> This functions have changed, see last series (but change is trivial from monitor_printf to error_<something>) > @@ -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"? Thanks, Juan.
> From: Juan Quintela [mailto:quintela@redhat.com] > Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> wrote: > > From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> > > > > 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 <pavel.dovgaluk@ispras.ru> > > This functions have changed, see last series (but change is trivial from > monitor_printf to error_<something>) > > > @@ -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"? Right, thanks. Pavel Dovgalyuk
On 04/05/2017 10:42, Pavel Dovgalyuk wrote: > From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> > > 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? Paolo > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> > --- > include/sysemu/replay.h | 3 +++ > migration/savevm.c | 13 +++++++++++++ > replay/replay-snapshot.c | 6 ++++++ > 3 files changed, 22 insertions(+) > > diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h > index f1c0712795..5c9db2a2ef 100644 > --- a/include/sysemu/replay.h > +++ b/include/sysemu/replay.h > @@ -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 > diff --git a/migration/savevm.c b/migration/savevm.c > index 03ae1bdeb4..358d22170d 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -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)); > diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c > index 65e2d375c2..438d82ec33 100644 > --- a/replay/replay-snapshot.c > +++ b/replay/replay-snapshot.c > @@ -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:pbonzini@redhat.com] > On 04/05/2017 10:42, Pavel Dovgalyuk wrote: > > From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> > > > > 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. > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> > > --- > > include/sysemu/replay.h | 3 +++ > > migration/savevm.c | 13 +++++++++++++ > > replay/replay-snapshot.c | 6 ++++++ > > 3 files changed, 22 insertions(+) > > > > diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h > > index f1c0712795..5c9db2a2ef 100644 > > --- a/include/sysemu/replay.h > > +++ b/include/sysemu/replay.h > > @@ -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 > > diff --git a/migration/savevm.c b/migration/savevm.c > > index 03ae1bdeb4..358d22170d 100644 > > --- a/migration/savevm.c > > +++ b/migration/savevm.c > > @@ -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)); > > diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c > > index 65e2d375c2..438d82ec33 100644 > > --- a/replay/replay-snapshot.c > > +++ b/replay/replay-snapshot.c > > @@ -76,3 +76,9 @@ void replay_vmstate_init(void) > > } > > } > > } > > + > > +bool replay_can_snapshot(void) > > +{ > > + return replay_mode == REPLAY_MODE_NONE > > + || !replay_has_events(); > > +} > > Pavel Dovgalyuk
On 04/05/2017 13:13, Pavel Dovgalyuk wrote: >>> 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. But that would apply to loading only. Saving should still be always possible. Paolo
> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > On 04/05/2017 13:13, Pavel Dovgalyuk wrote: > >>> 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. > > 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. Pavel Dovgalyuk
On 04/05/2017 13:54, Pavel Dovgalyuk wrote: >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] >> On 04/05/2017 13:13, Pavel Dovgalyuk wrote: >>>>> 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. >> >> 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. Paolo
> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > On 04/05/2017 13:54, Pavel Dovgalyuk wrote: > >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > >> On 04/05/2017 13:13, Pavel Dovgalyuk wrote: > >>>>> 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. > >> > >> 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. Pavel Dovgalyuk
On 04/05/2017 14:10, Pavel Dovgalyuk wrote: >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] >> On 04/05/2017 13:54, Pavel Dovgalyuk wrote: >>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com] >>>> On 04/05/2017 13:13, Pavel Dovgalyuk wrote: >>>>>>> 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. >>>> >>>> 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. What checkpoint can hang the drain during a save? Paolo
> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > On 04/05/2017 14:10, Pavel Dovgalyuk wrote: > >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > >> On 04/05/2017 13:54, Pavel Dovgalyuk wrote: > >>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > >>>> On 04/05/2017 13:13, Pavel Dovgalyuk wrote: > >>>>>>> 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. > >>>> > >>>> 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. > > 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. Pavel Dovgalyuk
On 04/05/2017 14:34, Pavel Dovgalyuk wrote: >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] >> On 04/05/2017 14:10, Pavel Dovgalyuk wrote: >>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com] >>>> On 04/05/2017 13:54, Pavel Dovgalyuk wrote: >>>>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com] >>>>>> On 04/05/2017 13:13, Pavel Dovgalyuk wrote: >>>>>>>>> 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. >>>>>> >>>>>> 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. >> >> 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? Paolo
> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > On 04/05/2017 14:34, Pavel Dovgalyuk wrote: > >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > >> On 04/05/2017 14:10, Pavel Dovgalyuk wrote: > >>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > >>>> On 04/05/2017 13:54, Pavel Dovgalyuk wrote: > >>>>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > >>>>>> On 04/05/2017 13:13, Pavel Dovgalyuk wrote: > >>>>>>>>> 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. > >>>>>> > >>>>>> 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. > >> > >> 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? 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. Pavel Dovgalyuk
On 04/05/2017 15:02, Pavel Dovgalyuk wrote: >> 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. 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. Paolo
> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > On 04/05/2017 15:02, Pavel Dovgalyuk wrote: > >> 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. > > 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). Then we'll have to add something like "stop" event which flushes the queue. And there is still a problem with stopping in replay mode, because there is no way to flush the queue, because coroutines are executing somewhere. > 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. I think it could be an acceptable limitation (like disabling savevm/loadvm when queue is not empty). I assume that RR is mostly used for debugging and analysis and it could have some limitations of this kind. Making our own block queue (instead of coroutines) is much worse, therefore we have to replay the original internal behavior. Pavel Dovgalyuk
diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h index f1c0712795..5c9db2a2ef 100644 --- a/include/sysemu/replay.h +++ b/include/sysemu/replay.h @@ -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 diff --git a/migration/savevm.c b/migration/savevm.c index 03ae1bdeb4..358d22170d 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -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)); diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c index 65e2d375c2..438d82ec33 100644 --- a/replay/replay-snapshot.c +++ b/replay/replay-snapshot.c @@ -76,3 +76,9 @@ void replay_vmstate_init(void) } } } + +bool replay_can_snapshot(void) +{ + return replay_mode == REPLAY_MODE_NONE + || !replay_has_events(); +}