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(-)
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
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
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
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
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?
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
© 2016 - 2024 Red Hat, Inc.