On Thu, May 07, 2015 at 02:43:43PM +0100, Stefan Hajnoczi wrote: > On Wed, May 06, 2015 at 07:23:32PM +0800, Fam Zheng wrote: > > Reported by Paolo. > > > > Unlike the iohandler in main loop, iothreads currently process the event > > notifier used as virtio-blk ioeventfd in all nested aio_poll. This is > > dangerous > > without proper protection, because guest requests could sneak to block layer > > where they mustn't. > > > > For example, a QMP transaction may involve multiple bdrv_drain_all() in > > handling the list of AioContext it works on. If an aio_poll in one of the > > bdrv_drain_all() happens to process a guest VQ kick by dispatching the > > ioeventfd event, a new guest write is then submitted, and voila, the > > transaction semantics is violated. > > > > This series avoids this problem by disabling virtio-blk handlers during > > bdrv_drain_all() and transactions. > > > > Notes: > > > > If the general approach is right, other transaction types could get the > > blockers similarly, in next revision. And some related bdrv_drain_all() > > could > > also be changed to bdrv_drain(). > > > > virtio-scsi-dataplane will be a bit more complicated, but still doable. It > > would probably need one more interface abstraction between scsi-disk, > > scsi-bus > > and virtio-scsi. > > > > Although other devices don't have a pause/resume callback yet, the > > blk_check_request, which returns -EBUSY if "device io" op blocker is set, > > could > > hopefully cover most cases already. > > > > Timers and block jobs also generate IO, but it should be fine as long as > > they > > don't change guest visible data, which is true AFAICT. > > > > > > Fam Zheng (7): > > block: Add op blocker type "device IO" > > block: Block "device IO" during bdrv_drain and bdrv_drain_all > > block: Add op blocker notifier list > > block-backend: Add blk_op_blocker_add_notifier > > virtio-blk: Move complete_request to 'ops' structure > > virtio-blk: Don't handle output when there is "device IO" op blocker > > blockdev: Add "device IO" op blocker during snapshot transaction > > > > block.c | 20 ++++++++++++ > > block/block-backend.c | 10 ++++++ > > block/io.c | 12 +++++++ > > blockdev.c | 7 +++++ > > hw/block/dataplane/virtio-blk.c | 36 ++++++++++++++++++--- > > hw/block/virtio-blk.c | 69 > > +++++++++++++++++++++++++++++++++++++++-- > > include/block/block.h | 9 ++++++ > > include/block/block_int.h | 3 ++ > > include/hw/virtio/virtio-blk.h | 17 ++++++++-- > > include/sysemu/block-backend.h | 2 ++ > > 10 files changed, 174 insertions(+), 11 deletions(-) > > Please also add a listener to nbd.c so NBD exports cannot submit I/O > during bdrv_drain_all().
qmp_transaction probably also needs this. Stefan
pgpAQPw3DSGnC.pgp
Description: PGP signature