Il 03/06/2014 16:37, Kevin Wolf ha scritto:
> Am 03.06.2014 um 16:16 hat Paolo Bonzini geschrieben:
>> With virtio-blk dataplane, I/O errors might occur while QEMU is
>> not in the main I/O thread. However, it's invalid to call vm_stop
>> when we're neither in a VCPU thread nor in the main I/O thread,
>> even if we were to take the iothread mutex around it.
>>
>> To avoid this problem, simply raise a request to the main I/O thread,
>> similar to what QEMU does when vm_stop is called from a CPU thread.
>> We know that bdrv_error_action is called from an AIO callback, and
>> the moment at which the callback will fire is not well-defined; it
>> depends on the moment at which the disk or OS finishes the operation,
>> which can happen at any time.
>>
>> Note that QEMU is certainly not in a CPU thread and we do not need to
>> call cpu_stop_current() like vm_stop() does.
>
> Do I understand correctly that this is not a fundamental truth of qemu's
> operation, but holds true only because the drivers that do support
> rerror/werror all use bdrv_aio_readv/writev(), which guarantees that a
> BH is used in error cases? Otherwise I think an I/O handler in a vcpu
> thread could directly call into the block layer and fail immediately
> (might happen for example if we added rerror/werror support to ATAPI).
>
> By delaying the actual state change, does this break the invariant that
> bs->iostatus is BLOCK_DEVICE_IO_STATUS_OK while the VM is running?
These two comments are actually related, in that the invariant was
already not respected if an I/O handler in a VCPU thread could fail
immediately.
Breaking this invariant means that you have a very small window where
{'execute':'cont'} would actually not restart the VM. I think this
should be fixed by dropping the request in vm_start, like this:
diff --git a/vl.c b/vl.c
index db9ea90..09af28a 100644
--- a/vl.c
+++ b/vl.c
@@ -1721,8 +1721,31 @@ void vm_state_notify(int running, RunState state)
}
}
+static RunState vmstop_requested = RUN_STATE_MAX;
+
+/* We use RUN_STATE_MAX but any invalid value will do */
+static bool qemu_vmstop_requested(RunState *r)
+{
+ if (vmstop_requested < RUN_STATE_MAX) {
+ *r = vmstop_requested;
+ vmstop_requested = RUN_STATE_MAX;
+ return true;
+ }
+
+ return false;
+}
+
+void qemu_system_vmstop_request(RunState state)
+{
+ vmstop_requested = state;
+ qemu_notify_event();
+}
+
void vm_start(void)
{
+ RunState dummy;
+
+ qemu_vmstop_requested(&dummy);
if (!runstate_is_running()) {
cpu_enable_ticks();
runstate_set(RUN_STATE_RUNNING);
@@ -1756,7 +1779,6 @@ static NotifierList suspend_notifiers =
static NotifierList wakeup_notifiers =
NOTIFIER_LIST_INITIALIZER(wakeup_notifiers);
static uint32_t wakeup_reason_mask = ~(1 << QEMU_WAKEUP_REASON_NONE);
-static RunState vmstop_requested = RUN_STATE_MAX;
int qemu_shutdown_requested_get(void)
{
@@ -1824,18 +1846,6 @@ static int qemu_debug_requested(void)
return r;
}
-/* We use RUN_STATE_MAX but any invalid value will do */
-static bool qemu_vmstop_requested(RunState *r)
-{
- if (vmstop_requested < RUN_STATE_MAX) {
- *r = vmstop_requested;
- vmstop_requested = RUN_STATE_MAX;
- return true;
- }
-
- return false;
-}
-
void qemu_register_reset(QEMUResetHandler *func, void *opaque)
{
QEMUResetEntry *re = g_malloc0(sizeof(QEMUResetEntry));
@@ -1985,12 +1995,6 @@ void qemu_system_debug_request(void)
qemu_notify_event();
}
-void qemu_system_vmstop_request(RunState state)
-{
- vmstop_requested = state;
- qemu_notify_event();
-}
-
static bool main_loop_should_exit(void)
{
RunState r;
Also, I think that bdrv_emit_qmp_error_event is placed wrong.
It should be called only after setting the iostatus, otherwise
there is a small window where the iostatus is "no error" but
the event has been generated already.
Paolo
