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

[Kevin Wolf]

Am 03.06.2014 um 17:51 hat Paolo Bonzini geschrieben:
> 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.

Oh, right, I somehow expected that vm_stop() waits for the CPU to be
stopped before it returns, but that's not what it does.

> 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:
> [...]

Sounds like an option. Do we need to send a QEVENT_STOP/QEVENT_RESUME
pair? If we don't, the client will still notice a difference to a real
stop and resume.

> 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.

Yes, I agree.

The documentation for this event actually answers my above question:

    Note: If action is "stop", a STOP event will eventually follow the
    BLOCK_IO_ERROR event.

Perhaps we should also change the documentation of the "stop" value to
clarify that the VM may not actually be stopped yet. It currently reads
like this:

    "stop": error caused VM to be stopped

Kevin