Re: [Qemu-devel] [PATCH] block: asynchronously stop the VM on I/O errors

[Paolo Bonzini <pbonzini@redhat.com>]

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