[Qemu-devel] [PATCH v3 0/2] block, virtio-scsi: Fix blk_set_aio_context

[Qemu-devel] [PATCH v3 0/2] block, virtio-scsi: Fix blk_set_aio_context

[Fam Zheng]

This is the simplified fix of:

[PATCH 0/3] virtio-scsi: Fix unsafe bdrv_set_aio_context calls

I included the original patch 1 - the function header comment update for
bdrv_set_aio_context and added Paolo's rev-by.

Fam Zheng (2):
  block: Forbid bdrv_set_aio_context outside BQL
  virtio-scsi-dataplane: Call blk_set_aio_context within BQL

 hw/scsi/virtio-scsi.c | 15 +++++++--------
 include/block/block.h |  3 +--
 2 files changed, 8 insertions(+), 10 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v3 1/2] block: Forbid bdrv_set_aio_context outside BQL

[Fam Zheng]

Even if the caller has both the old and the new AioContext's, there can
be a deadlock, due to the leading bdrv_drain_all.

Suppose there are four io threads (A, B, A0, B0) with A and B owning a
BDS for each (bs_a, bs_b); Now A wants to move bs_a to iothread A0, and
B wants to move bs_b to B0, at the same time:

  iothread A                           iothread B
--------------------------------------------------------------------------
  aio_context_acquire(A0) /* OK */     aio_context_acquire(B0) /* OK */
  bdrv_set_aio_context(bs_a, A0)       bdrv_set_aio_context(bs_b, B0)
  -> bdrv_drain_all()                  -> bdrv_drain_all()
     -> acquire A /* OK */               -> acquire A /* blocked */
     -> acquire B /* blocked */          -> acquire B
     ...                                 ...

Deadlock happens because A is waiting for B, and B is waiting for A.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/block/block.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 321295e..4fce25d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -546,8 +546,7 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
  * Changes the #AioContext used for fd handlers, timers, and BHs by this
  * BlockDriverState and all its children.
  *
- * This function must be called from the old #AioContext or with a lock held so
- * the old #AioContext is not executing.
+ * This function must be called with iothread lock held.
  */
 void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context);
 
-- 
2.1.0

[Qemu-devel] [PATCH v3 2/2] virtio-scsi-dataplane: Call blk_set_aio_context within BQL

[Fam Zheng]

It's not safe to call blk_set_aio_context from outside BQL because of
the bdrv_drain_all there. Let's put it in the hotplug callback which
will be called by qdev device realization for each scsi device attached
to the bus.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/virtio-scsi.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 9e2c718..8c437dd 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -254,10 +254,8 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
     int target;
     int ret = 0;
 
-    if (s->dataplane_started && blk_get_aio_context(d->conf.blk) != s->ctx) {
-        aio_context_acquire(s->ctx);
-        blk_set_aio_context(d->conf.blk, s->ctx);
-        aio_context_release(s->ctx);
+    if (s->dataplane_started) {
+        assert(blk_get_aio_context(d->conf.blk) == s->ctx);
     }
     /* Here VIRTIO_SCSI_S_OK means "FUNCTION COMPLETE".  */
     req->resp.tmf.response = VIRTIO_SCSI_S_OK;
@@ -540,10 +538,8 @@ bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
         virtio_scsi_complete_cmd_req(req);
         return false;
     }
-    if (s->dataplane_started && blk_get_aio_context(d->conf.blk) != s->ctx) {
-        aio_context_acquire(s->ctx);
-        blk_set_aio_context(d->conf.blk, s->ctx);
-        aio_context_release(s->ctx);
+    if (s->dataplane_started) {
+        assert(blk_get_aio_context(d->conf.blk) == s->ctx);
     }
     req->sreq = scsi_req_new(d, req->req.cmd.tag,
                              virtio_scsi_get_lun(req->req.cmd.lun),
@@ -767,6 +763,9 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
             return;
         }
         blk_op_block_all(sd->conf.blk, s->blocker);
+        aio_context_acquire(s->ctx);
+        blk_set_aio_context(sd->conf.blk, s->ctx);
+        aio_context_release(s->ctx);
     }
 
     if ((vdev->guest_features >> VIRTIO_SCSI_F_HOTPLUG) & 1) {
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 0/2] block, virtio-scsi: Fix blk_set_aio_context

[Paolo Bonzini]

On 15/02/2015 04:06, Fam Zheng wrote:
> This is the simplified fix of:
> 
> [PATCH 0/3] virtio-scsi: Fix unsafe bdrv_set_aio_context calls
> 
> I included the original patch 1 - the function header comment update for
> bdrv_set_aio_context and added Paolo's rev-by.
> 
> Fam Zheng (2):
>   block: Forbid bdrv_set_aio_context outside BQL
>   virtio-scsi-dataplane: Call blk_set_aio_context within BQL
> 
>  hw/scsi/virtio-scsi.c | 15 +++++++--------
>  include/block/block.h |  3 +--
>  2 files changed, 8 insertions(+), 10 deletions(-)
> 

Applied, thanks.

Paolo