[Qemu-devel] [PATCH v2 0/2] block: thread-safety patches for virtio-blk dataplane rerror/werror

[Qemu-devel] [PATCH v2 0/2] block: thread-safety patches for virtio-blk dataplane rerror/werror

[Paolo Bonzini]

With virtio-blk dataplane, I/O errors might occur while QEMU is
not in the main I/O thread.  This makes the block layer's
bdrv_error_action() function thread-safe (modulo changes in
the monitor to make QMP events thread-safe too, already posted).

Thanks to Kevin for discussing v1, the ideas are based on his
input too.

v1->v2: redone, v1 should really have been an RFC

Paolo Bonzini (2):
  vl: allow other threads to do qemu_system_vmstop_request
  block: asynchronously stop the VM on I/O errors

 block.c                 | 20 ++++++++++--
 cpus.c                  |  1 +
 docs/qmp/qmp-events.txt |  2 +-
 include/sysemu/sysemu.h |  1 +
 stubs/vm-stop.c         |  7 +++-
 target-lm32/op_helper.c |  2 +-
 vl.c                    | 85 +++++++++++++++++++++++++++++++------------------
 7 files changed, 82 insertions(+), 36 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/2] vl: allow other threads to do qemu_system_vmstop_request

[Paolo Bonzini]

There patch protects vmstop_requested with a lock and introduces
qemu_system_vmstop_request_prepare.

Together with the new call to qemu_vmstop_requested in vm_start,
qemu_system_vmstop_request_prepare avoids a race where the VM could remain
stopped even though the iostatus of a block device has already been set
(for example).

qemu_system_vmstop_request_prepare however also lets the caller thread
delay observation of the state change until it has itself communicated
that change to the user.  This delay avoids any possibility of a wrong
reordering of the BLOCK_IO_ERROR event and the subsequent STOP event.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c                  |  1 +
 include/sysemu/sysemu.h |  1 +
 target-lm32/op_helper.c |  2 +-
 vl.c                    | 85 +++++++++++++++++++++++++++++++------------------
 4 files changed, 57 insertions(+), 32 deletions(-)

diff --git a/cpus.c b/cpus.c
index dd7ac13..f566a88 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1206,6 +1206,7 @@ void cpu_stop_current(void)
 int vm_stop(RunState state)
 {
     if (qemu_in_vcpu_thread()) {
+        qemu_system_vmstop_request_prepare();
         qemu_system_vmstop_request(state);
         /*
          * FIXME: should not return to device code in case
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index ba5c7f8..0a2f2bb 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -60,6 +60,7 @@ void qemu_system_powerdown_request(void);
 void qemu_register_powerdown_notifier(Notifier *notifier);
 void qemu_system_debug_request(void);
 void qemu_system_vmstop_request(RunState reason);
+void qemu_system_vmstop_request_prepare(void);
 int qemu_shutdown_requested_get(void);
 int qemu_reset_requested_get(void);
 void qemu_system_killed(int signal, pid_t pid);
diff --git a/target-lm32/op_helper.c b/target-lm32/op_helper.c
index 40fbed6..25ad09a 100644
--- a/target-lm32/op_helper.c
+++ b/target-lm32/op_helper.c
@@ -52,7 +52,7 @@ void HELPER(ill)(CPULM32State *env)
     fprintf(stderr, "VM paused due to illegal instruction. "
             "Connect a debugger or switch to the monitor console "
             "to find out more.\n");
-    qemu_system_vmstop_request(RUN_STATE_PAUSED);
+    vm_stop(RUN_STATE_PAUSED);
     cs->halted = 1;
     raise_exception(env, EXCP_HALTED);
 #endif
diff --git a/vl.c b/vl.c
index 0c15608..229a8d1 100644
--- a/vl.c
+++ b/vl.c
@@ -567,6 +567,10 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
 
 static RunState current_run_state = RUN_STATE_PRELAUNCH;
 
+/* We use RUN_STATE_MAX but any invalid value will do */
+static RunState vmstop_requested = RUN_STATE_MAX;
+static QemuMutex vmstop_lock;
+
 typedef struct {
     RunState from;
     RunState to;
@@ -643,10 +647,11 @@ static void runstate_init(void)
     const RunStateTransition *p;
 
     memset(&runstate_valid_transitions, 0, sizeof(runstate_valid_transitions));
-
     for (p = &runstate_transitions_def[0]; p->from != RUN_STATE_MAX; p++) {
         runstate_valid_transitions[p->from][p->to] = true;
     }
+
+    qemu_mutex_init(&vmstop_lock);
 }
 
 /* This function will abort() on invalid state transitions */
@@ -686,6 +691,54 @@ StatusInfo *qmp_query_status(Error **errp)
     return info;
 }
 
+static bool qemu_vmstop_requested(RunState *r)
+{
+    qemu_mutex_lock(&vmstop_lock);
+    *r = vmstop_requested;
+    vmstop_requested = RUN_STATE_MAX;
+    qemu_mutex_unlock(&vmstop_lock);
+    return *r < RUN_STATE_MAX;
+}
+
+void qemu_system_vmstop_request_prepare(void)
+{
+    qemu_mutex_lock(&vmstop_lock);
+}
+
+void qemu_system_vmstop_request(RunState state)
+{
+    vmstop_requested = state;
+    qemu_mutex_unlock(&vmstop_lock);
+    qemu_notify_event();
+}
+
+void vm_start(void)
+{
+    RunState requested;
+
+    qemu_vmstop_requested(&requested);
+    if (runstate_is_running() && requested == RUN_STATE_MAX) {
+        return;
+    }
+
+    /* Ensure that a STOP/RESUME pair of events is emitted if a
+     * vmstop request was pending.  The BLOCK_IO_ERROR event, for
+     * example, according to documentation is always followed by
+     * the STOP event.
+     */
+    if (runstate_is_running()) {
+        monitor_protocol_event(QEVENT_STOP, NULL);
+    } else {
+        cpu_enable_ticks();
+        runstate_set(RUN_STATE_RUNNING);
+        vm_state_notify(1, RUN_STATE_RUNNING);
+        resume_all_vcpus();
+    }
+
+    monitor_protocol_event(QEVENT_RESUME, NULL);
+}
+
+
 /***********************************************************/
 /* real time host monotonic timer */
 
@@ -1747,17 +1800,6 @@ void vm_state_notify(int running, RunState state)
     }
 }
 
-void vm_start(void)
-{
-    if (!runstate_is_running()) {
-        cpu_enable_ticks();
-        runstate_set(RUN_STATE_RUNNING);
-        vm_state_notify(1, RUN_STATE_RUNNING);
-        resume_all_vcpus();
-        monitor_protocol_event(QEVENT_RESUME, NULL);
-    }
-}
-
 /* reset/shutdown handler */
 
 typedef struct QEMUResetEntry {
@@ -1782,7 +1824,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)
 {
@@ -1850,18 +1891,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));
@@ -2011,12 +2040,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;
-- 
1.8.3.1

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

[Paolo Bonzini]

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, we can 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.

However, we need to ensure that any action taken by management will
result in correct detection of the error _and_ a running VM.  In particular:

- the event must be raised after the iostatus has been set, so that
"info block" will return an iostatus that matches the event.

- the VM must be stopped after the iostatus has been set, so that
"info block" will return an iostatus that matches the runstate.

The ordering between the STOP and BLOCK_IO_ERROR events is preserved;
BLOCK_IO_ERROR is documented to come first.

This makes bdrv_error_action() thread safe (assuming QMP events are,
which is attacked by a separate series).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c                 | 20 ++++++++++++++++++--
 docs/qmp/qmp-events.txt |  2 +-
 stubs/vm-stop.c         |  7 ++++++-
 3 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 17f763d..32082f6 100644
--- a/block.c
+++ b/block.c
@@ -3629,10 +3629,27 @@ void bdrv_error_action(BlockDriverState *bs, BlockErrorAction action,
                        bool is_read, int error)
 {
     assert(error >= 0);
-    bdrv_emit_qmp_error_event(bs, QEVENT_BLOCK_IO_ERROR, action, is_read);
+
     if (action == BDRV_ACTION_STOP) {
-        vm_stop(RUN_STATE_IO_ERROR);
+        /* First set the iostatus, so that "info block" returns an iostatus
+         * that matches the events raised so far (an additional error iostatus
+         * is fine, but not a lost one).
+         */
         bdrv_iostatus_set_err(bs, error);
+
+        /* Then raise the request to stop the VM and the event.
+         * qemu_system_vmstop_request_prepare has two effects.  First,
+         * it ensures that the STOP event always comes after the
+         * BLOCK_IO_ERROR event.  Second, it ensures that even if management
+         * can observe the STOP event and do a "cont" before the STOP
+         * event is issued, the VM will not stop.  In this case, vm_start()
+         * also ensures that the STOP/RESUME pair of events is emitted.
+         */
+        qemu_system_vmstop_request_prepare();
+        bdrv_emit_qmp_error_event(bs, QEVENT_BLOCK_IO_ERROR, action, is_read);
+        qemu_system_vmstop_request(RUN_STATE_IO_ERROR);
+    } else {
+        bdrv_emit_qmp_error_event(bs, QEVENT_BLOCK_IO_ERROR, action, is_read);
     }
 }
 
diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
index 145402e..849ec9d 100644
--- a/docs/qmp/qmp-events.txt
+++ b/docs/qmp/qmp-events.txt
@@ -52,7 +52,7 @@ Data:
 - "action": action that has been taken, it's one of the following (json-string):
     "ignore": error has been ignored
     "report": error has been reported to the device
-    "stop": error caused VM to be stopped
+    "stop": the VM is going to stop because of the error
 
 Example:
 
diff --git a/stubs/vm-stop.c b/stubs/vm-stop.c
index f82c897..69fd86b 100644
--- a/stubs/vm-stop.c
+++ b/stubs/vm-stop.c
@@ -1,7 +1,12 @@
 #include "qemu-common.h"
 #include "sysemu/sysemu.h"
 
-int vm_stop(RunState state)
+void qemu_system_vmstop_request_prepare(void)
+{
+    abort();
+}
+
+void qemu_system_vmstop_request(RunState state)
 {
     abort();
 }
-- 
1.8.3.1

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1965 bytes --]

On 06/05/2014 06:53 AM, Paolo Bonzini wrote:
> 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, we can 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.
> 
> However, we need to ensure that any action taken by management will
> result in correct detection of the error _and_ a running VM.  In particular:
> 
> - the event must be raised after the iostatus has been set, so that
> "info block" will return an iostatus that matches the event.
> 
> - the VM must be stopped after the iostatus has been set, so that
> "info block" will return an iostatus that matches the runstate.
> 
> The ordering between the STOP and BLOCK_IO_ERROR events is preserved;
> BLOCK_IO_ERROR is documented to come first.
> 
> This makes bdrv_error_action() thread safe (assuming QMP events are,
> which is attacked by a separate series).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block.c                 | 20 ++++++++++++++++++--
>  docs/qmp/qmp-events.txt |  2 +-
>  stubs/vm-stop.c         |  7 ++++++-
>  3 files changed, 25 insertions(+), 4 deletions(-)

This may need to be rebased, since events are now QAPI.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 1/2] vl: allow other threads to do qemu_system_vmstop_request

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

On 06/05/2014 06:53 AM, Paolo Bonzini wrote:
> There patch protects vmstop_requested with a lock and introduces

s/There/This/ ?

> qemu_system_vmstop_request_prepare.
> 
> Together with the new call to qemu_vmstop_requested in vm_start,
> qemu_system_vmstop_request_prepare avoids a race where the VM could remain
> stopped even though the iostatus of a block device has already been set
> (for example).
> 
> qemu_system_vmstop_request_prepare however also lets the caller thread
> delay observation of the state change until it has itself communicated
> that change to the user.  This delay avoids any possibility of a wrong
> reordering of the BLOCK_IO_ERROR event and the subsequent STOP event.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  cpus.c                  |  1 +
>  include/sysemu/sysemu.h |  1 +
>  target-lm32/op_helper.c |  2 +-
>  vl.c                    | 85 +++++++++++++++++++++++++++++++------------------
>  4 files changed, 57 insertions(+), 32 deletions(-)
> 

Not my area of code expertise, so take this with a grain of salt:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 0/2] block: thread-safety patches for virtio-blk dataplane rerror/werror

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1067 bytes --]

On Thu, Jun 05, 2014 at 02:53:57PM +0200, Paolo Bonzini wrote:
> With virtio-blk dataplane, I/O errors might occur while QEMU is
> not in the main I/O thread.  This makes the block layer's
> bdrv_error_action() function thread-safe (modulo changes in
> the monitor to make QMP events thread-safe too, already posted).
> 
> Thanks to Kevin for discussing v1, the ideas are based on his
> input too.
> 
> v1->v2: redone, v1 should really have been an RFC
> 
> Paolo Bonzini (2):
>   vl: allow other threads to do qemu_system_vmstop_request
>   block: asynchronously stop the VM on I/O errors
> 
>  block.c                 | 20 ++++++++++--
>  cpus.c                  |  1 +
>  docs/qmp/qmp-events.txt |  2 +-
>  include/sysemu/sysemu.h |  1 +
>  stubs/vm-stop.c         |  7 +++-
>  target-lm32/op_helper.c |  2 +-
>  vl.c                    | 85 +++++++++++++++++++++++++++++++------------------
>  7 files changed, 82 insertions(+), 36 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 0/2] block: thread-safety patches for virtio-blk dataplane rerror/werror

[Paolo Bonzini]

----- Messaggio originale -----
> Da: "Stefan Hajnoczi" <stefanha@redhat.com>
> A: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org, famz@redhat.com, kwolf@redhat.com
> Inviato: Venerdì, 20 giugno 2014 5:58:59
> Oggetto: Re: [PATCH v2 0/2] block: thread-safety patches for virtio-blk dataplane rerror/werror
> 
> On Thu, Jun 05, 2014 at 02:53:57PM +0200, Paolo Bonzini wrote:
> > With virtio-blk dataplane, I/O errors might occur while QEMU is
> > not in the main I/O thread.  This makes the block layer's
> > bdrv_error_action() function thread-safe (modulo changes in
> > the monitor to make QMP events thread-safe too, already posted).
> > 
> > Thanks to Kevin for discussing v1, the ideas are based on his
> > input too.
> > 
> > v1->v2: redone, v1 should really have been an RFC
> > 
> > Paolo Bonzini (2):
> >   vl: allow other threads to do qemu_system_vmstop_request
> >   block: asynchronously stop the VM on I/O errors
> > 
> >  block.c                 | 20 ++++++++++--
> >  cpus.c                  |  1 +
> >  docs/qmp/qmp-events.txt |  2 +-
> >  include/sysemu/sysemu.h |  1 +
> >  stubs/vm-stop.c         |  7 +++-
> >  target-lm32/op_helper.c |  2 +-
> >  vl.c                    | 85
> >  +++++++++++++++++++++++++++++++------------------
> >  7 files changed, 82 insertions(+), 36 deletions(-)
> 
> Thanks, applied to my block tree:
> https://github.com/stefanha/qemu/commits/block

Hi Stefan, both patches actually will need rebasing on top of the
QAPI event series from Wenchao.  I can resend it next week.

Paolo