[Qemu-devel] [RFC v3 0/3] scsi: restart dma after vm change state handlers

Stefan Hajnoczi posted 3 patches 4 years, 10 months ago
Test FreeBSD passed
Test docker-clang@ubuntu passed
Test s390x passed
Test docker-mingw@fedora passed
Test checkpatch passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190524183638.20745-1-stefanha@redhat.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Fam Zheng <fam@euphon.net>, Paolo Bonzini <pbonzini@redhat.com>
include/hw/scsi/scsi.h     |  5 +++++
include/hw/virtio/virtio.h |  7 +++++++
hw/scsi/scsi-bus.c         | 37 ++++++++++++++++++++++++++++++-------
hw/scsi/virtio-scsi.c      | 17 +++++++++++++++++
hw/virtio/virtio.c         |  9 +++++++++
5 files changed, 68 insertions(+), 7 deletions(-)
[Qemu-devel] [RFC v3 0/3] scsi: restart dma after vm change state handlers
Posted by Stefan Hajnoczi 4 years, 10 months ago
v3:
 * Fix s/k->vmstate_change/vdc->vmstate_change/
 * Still RFC, waiting for customer to confirm this fixes the issue
v2:
 * Do it properly with a clean API instead of deferring to a BH!
   Thanks for encouraging me to do this, Kevin.

These patches solve a deadlock when the 'cont' command is used and there are
failed requests on a virtio-scsi device with iothreads.  The deadlock itself is
actually not the thing we need to fix because we should never reach that case
anyway.  Instead we need to make sure DMA restart is only performed after the
virtio-scsi iothread is re-initialized.

Stefan Hajnoczi (3):
  virtio: add vdc->vmchange_state() callback
  scsi: add scsi_bus_dma_restart()
  virtio-scsi: fix iothread deadlock on 'cont'

 include/hw/scsi/scsi.h     |  5 +++++
 include/hw/virtio/virtio.h |  7 +++++++
 hw/scsi/scsi-bus.c         | 37 ++++++++++++++++++++++++++++++-------
 hw/scsi/virtio-scsi.c      | 17 +++++++++++++++++
 hw/virtio/virtio.c         |  9 +++++++++
 5 files changed, 68 insertions(+), 7 deletions(-)

-- 
2.21.0


Re: [Qemu-devel] [RFC v3 0/3] scsi: restart dma after vm change state handlers
Posted by Paolo Bonzini 4 years, 10 months ago
On 24/05/19 20:36, Stefan Hajnoczi wrote:
> v3:
>  * Fix s/k->vmstate_change/vdc->vmstate_change/
>  * Still RFC, waiting for customer to confirm this fixes the issue
> v2:
>  * Do it properly with a clean API instead of deferring to a BH!
>    Thanks for encouraging me to do this, Kevin.
> 
> These patches solve a deadlock when the 'cont' command is used and there are
> failed requests on a virtio-scsi device with iothreads.  The deadlock itself is
> actually not the thing we need to fix because we should never reach that case
> anyway.  Instead we need to make sure DMA restart is only performed after the
> virtio-scsi iothread is re-initialized.

custom_dma_restart is a bit ugly...  Do you think it would make sense to
order the VMStateChange handlers using some kind of enum (with the order
unspecified within the category)?

We could start with

	VMSTATECHANGE_PRIO_UNKNOWN  = 0  (if needed?)
	VMSTATECHANGE_PRIO_IOTHREAD = 100
        VMSTATECHANGE_PRIO_DEVICE   = 200

where higher priorities run first on stop and last on resume.

Paolo

Re: [Qemu-devel] [RFC v3 0/3] scsi: restart dma after vm change state handlers
Posted by Michael S. Tsirkin 4 years, 10 months ago
On Fri, May 24, 2019 at 08:47:18PM +0200, Paolo Bonzini wrote:
> On 24/05/19 20:36, Stefan Hajnoczi wrote:
> > v3:
> >  * Fix s/k->vmstate_change/vdc->vmstate_change/
> >  * Still RFC, waiting for customer to confirm this fixes the issue
> > v2:
> >  * Do it properly with a clean API instead of deferring to a BH!
> >    Thanks for encouraging me to do this, Kevin.
> > 
> > These patches solve a deadlock when the 'cont' command is used and there are
> > failed requests on a virtio-scsi device with iothreads.  The deadlock itself is
> > actually not the thing we need to fix because we should never reach that case
> > anyway.  Instead we need to make sure DMA restart is only performed after the
> > virtio-scsi iothread is re-initialized.
> 
> custom_dma_restart is a bit ugly...  Do you think it would make sense to
> order the VMStateChange handlers using some kind of enum (with the order
> unspecified within the category)?
> 
> We could start with
> 
> 	VMSTATECHANGE_PRIO_UNKNOWN  = 0  (if needed?)

Yes I think it's a good idea to explicitly say I don't care
about the order like this.

> 	VMSTATECHANGE_PRIO_IOTHREAD = 100
>         VMSTATECHANGE_PRIO_DEVICE   = 200
> 
> where higher priorities run first on stop and last on resume.
> 
> Paolo

Re: [Qemu-devel] [RFC v3 0/3] scsi: restart dma after vm change state handlers
Posted by Kevin Wolf 4 years, 10 months ago
Am 24.05.2019 um 20:47 hat Paolo Bonzini geschrieben:
> On 24/05/19 20:36, Stefan Hajnoczi wrote:
> > v3:
> >  * Fix s/k->vmstate_change/vdc->vmstate_change/
> >  * Still RFC, waiting for customer to confirm this fixes the issue
> > v2:
> >  * Do it properly with a clean API instead of deferring to a BH!
> >    Thanks for encouraging me to do this, Kevin.
> > 
> > These patches solve a deadlock when the 'cont' command is used and there are
> > failed requests on a virtio-scsi device with iothreads.  The deadlock itself is
> > actually not the thing we need to fix because we should never reach that case
> > anyway.  Instead we need to make sure DMA restart is only performed after the
> > virtio-scsi iothread is re-initialized.
> 
> custom_dma_restart is a bit ugly...  Do you think it would make sense to
> order the VMStateChange handlers using some kind of enum (with the order
> unspecified within the category)?
> 
> We could start with
> 
> 	VMSTATECHANGE_PRIO_UNKNOWN  = 0  (if needed?)
> 	VMSTATECHANGE_PRIO_IOTHREAD = 100
>         VMSTATECHANGE_PRIO_DEVICE   = 200
> 
> where higher priorities run first on stop and last on resume.

I don't think this is as nice because stopping or resuming a device
could involve some async operation (e.g. be delegated to a BH). In this
case, a device on a child bus must still wait for the BH (or other async
part) to be completed before it can resume its own operation.

Basically, a single flat list of global VM state handlers wasn't a good
design, because what we actually need in such cases is something
hierarchical.

Kevin

Re: [Qemu-devel] [RFC v3 0/3] scsi: restart dma after vm change state handlers
Posted by Paolo Bonzini 4 years, 10 months ago
On 30/05/19 00:10, Kevin Wolf wrote:
> I don't think this is as nice because stopping or resuming a device
> could involve some async operation (e.g. be delegated to a BH). In this
> case, a device on a child bus must still wait for the BH (or other async
> part) to be completed before it can resume its own operation.
> 
> Basically, a single flat list of global VM state handlers wasn't a good
> design, because what we actually need in such cases is something
> hierarchical.

So add an AioContext state change handler?

Re: [Qemu-devel] [RFC v3 0/3] scsi: restart dma after vm change state handlers
Posted by Kevin Wolf 4 years, 10 months ago
Am 30.05.2019 um 10:27 hat Paolo Bonzini geschrieben:
> On 30/05/19 00:10, Kevin Wolf wrote:
> > I don't think this is as nice because stopping or resuming a device
> > could involve some async operation (e.g. be delegated to a BH). In this
> > case, a device on a child bus must still wait for the BH (or other async
> > part) to be completed before it can resume its own operation.
> > 
> > Basically, a single flat list of global VM state handlers wasn't a good
> > design, because what we actually need in such cases is something
> > hierarchical.
> 
> So add an AioContext state change handler?

Does anything really change for the AioContext that could cause a
callback? As I read the code, only the virtio-scsi device state really
changes and it doesn't do more with the AioContext than just taking the
lock for a while.

But in any case, inferring whether the HBA is ready from some AioContext
state change, even if it were technically possible, is rather indirect
and more a hack than a proper solution in my book. So a callback from
the HBA to its bus feels like the correct approach.

Kevin