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

[Qemu-devel] [PATCH v2 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 v2 1/2] block: Forbid bdrv_set_aio_context outside BQL

[Fam Zheng]

Even if the caller has the old #AioContext, there can be a deadlock, due
to the leading bdrv_drain_all:

Suppose there are three io threads (a, b, c) with each owning a BDS
(bds_a, bds_b, bds_c), and a and b want to move their own BDS to c at
the same time:

  iothread a                           iothread b
--------------------------------------------------------------------------
  bdrv_set_aio_context(bds_a, c)       bdrv_set_aio_context(bds_b, c)
  -> bdrv_drain_all()                  -> bdrv_drain_all()
     -> acquire a (OK, already has)       -> acquire a (blocked)
     -> acquire b (blocked)               -> acquire b
     -> acquire c                         -> acquire c

Current caller of bdrv_set_aio_context outside BQL is
virtio-scsi-dataplane, which will be fixed in the next patches.

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 v2 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 v2 1/2] block: Forbid bdrv_set_aio_context outside BQL

[Fam Zheng]

On Sun, 02/15 10:40, Fam Zheng wrote:
> Even if the caller has the old #AioContext, there can be a deadlock, due
> to the leading bdrv_drain_all:
> 
> Suppose there are three io threads (a, b, c) with each owning a BDS
> (bds_a, bds_b, bds_c), and a and b want to move their own BDS to c at
> the same time:
> 
>   iothread a                           iothread b
> --------------------------------------------------------------------------
>   bdrv_set_aio_context(bds_a, c)       bdrv_set_aio_context(bds_b, c)
>   -> bdrv_drain_all()                  -> bdrv_drain_all()
>      -> acquire a (OK, already has)       -> acquire a (blocked)
>      -> acquire b (blocked)               -> acquire b
>      -> acquire c                         -> acquire c

This doesn't recap the essence of the bug because one may argue that
aio_context_acquire(c) is needed before either thread calling
bdrv_set_aio_context. Actually it doesn't matter in this case, because iothread
b can as well do bdrv_set_aio_context(bds_b, d).

I'll update the commit log with v3.

Fam

> 
> Current caller of bdrv_set_aio_context outside BQL is
> virtio-scsi-dataplane, which will be fixed in the next patches.
> 
> 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
> 
>