[Qemu-devel] [RFC PATCH 0/7] Fix transactional snapshot with virtio-blk dataplane

[Qemu-devel] [RFC PATCH 0/7] Fix transactional snapshot with virtio-blk dataplane

[Fam Zheng]

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(-)

-- 
1.9.3

[Qemu-devel] [RFC PATCH 1/7] block: Add op blocker type "device IO"

[Fam Zheng]

Preventing device from submitting IO is useful around various nested
poll. Op blocker is a good place to put this flag.

Devices would submit IO requests through blk_* block backend interface,
which calls blk_check_request to check the validity. Return -EBUSY if
the operation is blocked, in which case device IO is temporarily
disabled by another BDS user.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/block-backend.c | 4 ++++
 include/block/block.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 93e46f3..71fc695 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -478,6 +478,10 @@ static int blk_check_request(BlockBackend *blk, int64_t sector_num,
         return -EIO;
     }
 
+    if (bdrv_op_is_blocked(blk->bs, BLOCK_OP_TYPE_DEVICE_IO, NULL)) {
+        return -EBUSY;
+    }
+
     return blk_check_byte_request(blk, sector_num * BDRV_SECTOR_SIZE,
                                   nb_sectors * BDRV_SECTOR_SIZE);
 }
diff --git a/include/block/block.h b/include/block/block.h
index 7d1a717..906fb31 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -159,6 +159,7 @@ typedef enum BlockOpType {
     BLOCK_OP_TYPE_RESIZE,
     BLOCK_OP_TYPE_STREAM,
     BLOCK_OP_TYPE_REPLACE,
+    BLOCK_OP_TYPE_DEVICE_IO,
     BLOCK_OP_TYPE_MAX,
 } BlockOpType;
 
-- 
1.9.3

[Qemu-devel] [RFC PATCH 2/7] block: Block "device IO" during bdrv_drain and bdrv_drain_all

[Fam Zheng]

We don't want new requests from guest, so block the operation around the
nested poll.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/block/io.c b/block/io.c
index 1ce62c4..d369de3 100644
--- a/block/io.c
+++ b/block/io.c
@@ -289,9 +289,15 @@ static bool bdrv_drain_one(BlockDriverState *bs)
  */
 void bdrv_drain(BlockDriverState *bs)
 {
+    Error *blocker = NULL;
+
+    error_setg(&blocker, "bdrv_drain in progress");
+    bdrv_op_block(bs, BLOCK_OP_TYPE_DEVICE_IO, blocker);
     while (bdrv_drain_one(bs)) {
         /* Keep iterating */
     }
+    bdrv_op_unblock(bs, BLOCK_OP_TYPE_DEVICE_IO, blocker);
+    error_free(blocker);
 }
 
 /*
@@ -311,6 +317,9 @@ void bdrv_drain_all(void)
     /* Always run first iteration so any pending completion BHs run */
     bool busy = true;
     BlockDriverState *bs = NULL;
+    Error *blocker = NULL;
+
+    error_setg(&blocker, "bdrv_drain_all in progress");
 
     while ((bs = bdrv_next(bs))) {
         AioContext *aio_context = bdrv_get_aio_context(bs);
@@ -319,6 +328,7 @@ void bdrv_drain_all(void)
         if (bs->job) {
             block_job_pause(bs->job);
         }
+        bdrv_op_block(bs, BLOCK_OP_TYPE_DEVICE_IO, blocker);
         aio_context_release(aio_context);
     }
 
@@ -343,8 +353,10 @@ void bdrv_drain_all(void)
         if (bs->job) {
             block_job_resume(bs->job);
         }
+        bdrv_op_unblock(bs, BLOCK_OP_TYPE_DEVICE_IO, blocker);
         aio_context_release(aio_context);
     }
+    error_free(blocker);
 }
 
 /**
-- 
1.9.3

[Qemu-devel] [RFC PATCH 3/7] block: Add op blocker notifier list

[Fam Zheng]

BDS users can register a notifier and get notified about op blocker
changes.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c                   | 20 ++++++++++++++++++++
 include/block/block.h     |  8 ++++++++
 include/block/block_int.h |  3 +++
 3 files changed, 31 insertions(+)

diff --git a/block.c b/block.c
index 7904098..054ddb4 100644
--- a/block.c
+++ b/block.c
@@ -3375,6 +3375,12 @@ struct BdrvOpBlocker {
     QLIST_ENTRY(BdrvOpBlocker) list;
 };
 
+void bdrv_op_blocker_add_notifier(BlockDriverState *bs,
+                                  Notifier *notifier)
+{
+    notifier_list_add(&bs->op_blocker_notifiers, notifier);
+}
+
 bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
 {
     BdrvOpBlocker *blocker;
@@ -3391,11 +3397,24 @@ bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
     return false;
 }
 
+static void bdrv_op_blocker_notify(BlockDriverState *bs, BlockOpType op,
+                                   Error *reason, bool blocking)
+{
+    BlockOpEvent event = (BlockOpEvent) {
+        op = op,
+        reason = reason,
+        blocking = true,
+    };
+
+    notifier_list_notify(&bs->op_blocker_notifiers, &event);
+}
+
 void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason)
 {
     BdrvOpBlocker *blocker;
     assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
 
+    bdrv_op_blocker_notify(bs, op, reason, true);
     blocker = g_new0(BdrvOpBlocker, 1);
     blocker->reason = reason;
     QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list);
@@ -3405,6 +3424,7 @@ void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason)
 {
     BdrvOpBlocker *blocker, *next;
     assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
+    bdrv_op_blocker_notify(bs, op, reason, false);
     QLIST_FOREACH_SAFE(blocker, &bs->op_blockers[op], list, next) {
         if (blocker->reason == reason) {
             QLIST_REMOVE(blocker, list);
diff --git a/include/block/block.h b/include/block/block.h
index 906fb31..3420b2c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -163,6 +163,12 @@ typedef enum BlockOpType {
     BLOCK_OP_TYPE_MAX,
 } BlockOpType;
 
+typedef struct {
+    BlockOpType type;
+    Error *reason;
+    bool blocking;
+} BlockOpEvent;
+
 void bdrv_iostatus_enable(BlockDriverState *bs);
 void bdrv_iostatus_reset(BlockDriverState *bs);
 void bdrv_iostatus_disable(BlockDriverState *bs);
@@ -491,6 +497,8 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
 void bdrv_ref(BlockDriverState *bs);
 void bdrv_unref(BlockDriverState *bs);
 
+void bdrv_op_blocker_add_notifier(BlockDriverState *bs,
+                                  Notifier *notifier);
 bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp);
 void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason);
 void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index db29b74..195ae30 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -418,6 +418,9 @@ struct BlockDriverState {
     /* operation blockers */
     QLIST_HEAD(, BdrvOpBlocker) op_blockers[BLOCK_OP_TYPE_MAX];
 
+    /* Callback before any op blocker change */
+    NotifierList op_blocker_notifiers;
+
     /* long-running background operation */
     BlockJob *job;
 
-- 
1.9.3

Re: [Qemu-devel] [RFC PATCH 3/7] block: Add op blocker notifier list

[Paolo Bonzini]

On 06/05/2015 13:23, Fam Zheng wrote:
>  void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason)
>  {
>      BdrvOpBlocker *blocker;
>      assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
>  
> +    bdrv_op_blocker_notify(bs, op, reason, true);
>      blocker = g_new0(BdrvOpBlocker, 1);
>      blocker->reason = reason;
>      QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list);
> @@ -3405,6 +3424,7 @@ void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason)
>  {
>      BdrvOpBlocker *blocker, *next;
>      assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
> +    bdrv_op_blocker_notify(bs, op, reason, false);
>      QLIST_FOREACH_SAFE(blocker, &bs->op_blockers[op], list, next) {
>          if (blocker->reason == reason) {
>              QLIST_REMOVE(blocker, list);

Changed in the following patch.

Also, should we only invoke the notifier if the list becomes
empty/non-empty?  Is there any reason why the notifier would like to
know the particular Error that was added?

Paolo

Re: [Qemu-devel] [RFC PATCH 3/7] block: Add op blocker notifier list

[Fam Zheng]

On Wed, 05/06 16:22, Paolo Bonzini wrote:
> 
> 
> On 06/05/2015 13:23, Fam Zheng wrote:
> >  void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason)
> >  {
> >      BdrvOpBlocker *blocker;
> >      assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
> >  
> > +    bdrv_op_blocker_notify(bs, op, reason, true);
> >      blocker = g_new0(BdrvOpBlocker, 1);
> >      blocker->reason = reason;
> >      QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list);
> > @@ -3405,6 +3424,7 @@ void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason)
> >  {
> >      BdrvOpBlocker *blocker, *next;
> >      assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
> > +    bdrv_op_blocker_notify(bs, op, reason, false);
> >      QLIST_FOREACH_SAFE(blocker, &bs->op_blockers[op], list, next) {
> >          if (blocker->reason == reason) {
> >              QLIST_REMOVE(blocker, list);
> 
> Changed in the following patch.

Bad git rebase -i, will fix.

> 
> Also, should we only invoke the notifier if the list becomes
> empty/non-empty?  Is there any reason why the notifier would like to
> know the particular Error that was added?
> 

Good point, this can be simplified.

Fam

[Qemu-devel] [RFC PATCH 4/7] block-backend: Add blk_op_blocker_add_notifier

[Fam Zheng]

Forward the call to bdrv_op_blocker_add_notifier.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c                        | 4 ++--
 block/block-backend.c          | 6 ++++++
 include/sysemu/block-backend.h | 2 ++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 054ddb4..d98a304 100644
--- a/block.c
+++ b/block.c
@@ -3414,23 +3414,23 @@ void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason)
     BdrvOpBlocker *blocker;
     assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
 
-    bdrv_op_blocker_notify(bs, op, reason, true);
     blocker = g_new0(BdrvOpBlocker, 1);
     blocker->reason = reason;
     QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list);
+    bdrv_op_blocker_notify(bs, op, reason, true);
 }
 
 void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason)
 {
     BdrvOpBlocker *blocker, *next;
     assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
-    bdrv_op_blocker_notify(bs, op, reason, false);
     QLIST_FOREACH_SAFE(blocker, &bs->op_blockers[op], list, next) {
         if (blocker->reason == reason) {
             QLIST_REMOVE(blocker, list);
             g_free(blocker);
         }
     }
+    bdrv_op_blocker_notify(bs, op, reason, false);
 }
 
 void bdrv_op_block_all(BlockDriverState *bs, Error *reason)
diff --git a/block/block-backend.c b/block/block-backend.c
index 71fc695..90d7476 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -806,6 +806,12 @@ void blk_op_unblock_all(BlockBackend *blk, Error *reason)
     bdrv_op_unblock_all(blk->bs, reason);
 }
 
+void blk_op_blocker_add_notifier(BlockBackend *blk,
+                                 Notifier *notifier)
+{
+    bdrv_op_blocker_add_notifier(blk->bs, notifier);
+}
+
 AioContext *blk_get_aio_context(BlockBackend *blk)
 {
     return bdrv_get_aio_context(blk->bs);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index b4a4d5e..cde9651 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -136,6 +136,8 @@ int blk_get_flags(BlockBackend *blk);
 int blk_get_max_transfer_length(BlockBackend *blk);
 void blk_set_guest_block_size(BlockBackend *blk, int align);
 void *blk_blockalign(BlockBackend *blk, size_t size);
+void blk_op_blocker_add_notifier(BlockBackend *blk,
+                                 Notifier *notifier);
 bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp);
 void blk_op_unblock(BlockBackend *blk, BlockOpType op, Error *reason);
 void blk_op_block_all(BlockBackend *blk, Error *reason);
-- 
1.9.3

[Qemu-devel] [RFC PATCH 5/7] virtio-blk: Move complete_request to 'ops' structure

[Fam Zheng]

Should more ops be added to differentiate code between dataplane and
non-dataplane, the new saved_ops approach will be cleaner than messing
with N pointers.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 13 ++++++++-----
 hw/block/virtio-blk.c           |  8 ++++++--
 include/hw/virtio/virtio-blk.h  |  9 +++++++--
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 3db139b..ec0c8f4 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -51,8 +51,7 @@ struct VirtIOBlockDataPlane {
 
     /* Operation blocker on BDS */
     Error *blocker;
-    void (*saved_complete_request)(struct VirtIOBlockReq *req,
-                                   unsigned char status);
+    const VirtIOBlockOps *saved_ops;
 };
 
 /* Raise an interrupt to signal guest, if necessary */
@@ -88,6 +87,10 @@ static void complete_request_vring(VirtIOBlockReq *req, unsigned char status)
     qemu_bh_schedule(s->bh);
 }
 
+static const VirtIOBlockOps virtio_blk_data_plane_ops = {
+    .complete_request = complete_request_vring,
+};
+
 static void handle_notify(EventNotifier *e)
 {
     VirtIOBlockDataPlane *s = container_of(e, VirtIOBlockDataPlane,
@@ -269,8 +272,8 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     }
     s->host_notifier = *virtio_queue_get_host_notifier(vq);
 
-    s->saved_complete_request = vblk->complete_request;
-    vblk->complete_request = complete_request_vring;
+    s->saved_ops = vblk->ops;
+    vblk->ops = &virtio_blk_data_plane_ops;
 
     s->starting = false;
     s->started = true;
@@ -313,7 +316,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
         return;
     }
     s->stopping = true;
-    vblk->complete_request = s->saved_complete_request;
+    vblk->ops = s->saved_ops;
     trace_virtio_blk_data_plane_stop(s);
 
     aio_context_acquire(s->ctx);
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e6afe97..f4a9d19 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -59,9 +59,13 @@ static void virtio_blk_complete_request(VirtIOBlockReq *req,
     virtio_notify(vdev, s->vq);
 }
 
+static const VirtIOBlockOps virtio_blk_ops = (VirtIOBlockOps) {
+    .complete_request = virtio_blk_complete_request,
+};
+
 static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
 {
-    req->dev->complete_request(req, status);
+    req->dev->ops->complete_request(req, status);
 }
 
 static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
@@ -905,7 +909,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     s->sector_mask = (s->conf.conf.logical_block_size / BDRV_SECTOR_SIZE) - 1;
 
     s->vq = virtio_add_queue(vdev, 128, virtio_blk_handle_output);
-    s->complete_request = virtio_blk_complete_request;
+    s->ops = &virtio_blk_ops;
     virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err);
     if (err != NULL) {
         error_propagate(errp, err);
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 6bf5905..28b3436 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -44,6 +44,12 @@ struct VirtIOBlkConf
 struct VirtIOBlockDataPlane;
 
 struct VirtIOBlockReq;
+
+typedef struct {
+    /* Function to push to vq and notify guest */
+    void (*complete_request)(struct VirtIOBlockReq *req, unsigned char status);
+} VirtIOBlockOps;
+
 typedef struct VirtIOBlock {
     VirtIODevice parent_obj;
     BlockBackend *blk;
@@ -54,8 +60,7 @@ typedef struct VirtIOBlock {
     unsigned short sector_mask;
     bool original_wce;
     VMChangeStateEntry *change;
-    /* Function to push to vq and notify guest */
-    void (*complete_request)(struct VirtIOBlockReq *req, unsigned char status);
+    const VirtIOBlockOps *ops;
     Notifier migration_state_notifier;
     struct VirtIOBlockDataPlane *dataplane;
 } VirtIOBlock;
-- 
1.9.3

[Qemu-devel] [RFC PATCH 6/7] virtio-blk: Don't handle output when there is "device IO" op blocker

[Fam Zheng]

virtio-blk now listens to op blocker change of the associated block
backend.

Up on setting op blocker on BLOCK_OP_TYPE_DEVICE_IO:

  non-dataplane:
   1) Set VirtIOBlock.paused
   2) In virtio_blk_handle_output, do nothing if VirtIOBlock.paused

  dataplane:
   1) Clear the host event notifier
   2) In handle_notify, do nothing if VirtIOBlock.paused

Up on removing the op blocker:

  non-dataplane:
   1) Clear VirtIOBlock.paused
   2) Schedule a BH on the AioContext of the backend, which calls
   virtio_blk_handle_output, so that the previous unhandled kicks can
   make progress

  dataplane:
   1) Set the host event notifier
   2) Notify the host event notifier so that unhandled events are
   processed

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 25 +++++++++++++++-
 hw/block/virtio-blk.c           | 63 +++++++++++++++++++++++++++++++++++++++--
 include/hw/virtio/virtio-blk.h  |  8 +++++-
 3 files changed, 92 insertions(+), 4 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index ec0c8f4..d6c943c 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -87,8 +87,28 @@ static void complete_request_vring(VirtIOBlockReq *req, unsigned char status)
     qemu_bh_schedule(s->bh);
 }
 
+static void virtio_blk_data_plane_pause(VirtIOBlock *vblk)
+{
+    VirtIOBlockDataPlane *s = vblk->dataplane;
+
+    event_notifier_test_and_clear(&s->host_notifier);
+    aio_set_event_notifier(s->ctx, &s->host_notifier, NULL);
+}
+
+static void handle_notify(EventNotifier *e);
+static void virtio_blk_data_plane_resume(VirtIOBlock *vblk)
+{
+    VirtIOBlockDataPlane *s = vblk->dataplane;
+
+    aio_set_event_notifier(s->ctx, &s->host_notifier, handle_notify);
+
+    event_notifier_set(&s->host_notifier);
+}
+
 static const VirtIOBlockOps virtio_blk_data_plane_ops = {
-    .complete_request = complete_request_vring,
+    .complete_request           = complete_request_vring,
+    .pause                      = virtio_blk_data_plane_pause,
+    .resume                     = virtio_blk_data_plane_resume,
 };
 
 static void handle_notify(EventNotifier *e)
@@ -98,6 +118,9 @@ static void handle_notify(EventNotifier *e)
     VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
 
     event_notifier_test_and_clear(&s->host_notifier);
+    if (vblk->paused) {
+        return;
+    }
     blk_io_plug(s->conf->conf.blk);
     for (;;) {
         MultiReqBuffer mrb = {};
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f4a9d19..4bc1b2a 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -59,8 +59,38 @@ static void virtio_blk_complete_request(VirtIOBlockReq *req,
     virtio_notify(vdev, s->vq);
 }
 
+typedef struct {
+    QEMUBH *bh;
+    VirtIOBlock *s;
+} VirtIOBlockResumeData;
+
+static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq);
+static void virtio_blk_resume_bh_cb(void *opaque)
+{
+    VirtIOBlockResumeData *data = opaque;
+    qemu_bh_delete(data->bh);
+    virtio_blk_handle_output(VIRTIO_DEVICE(data->s), data->s->vq);
+}
+
+static void virtio_blk_pause(VirtIOBlock *vblk)
+{
+    /* TODO: stop ioeventfd */
+}
+
+static void virtio_blk_resume(VirtIOBlock *vblk)
+{
+    VirtIOBlockResumeData *data = g_new(VirtIOBlockResumeData, 1);
+    data->bh = aio_bh_new(blk_get_aio_context(vblk->blk),
+            virtio_blk_resume_bh_cb, data);
+    data->s = vblk;
+    data->s->paused = false;
+    qemu_bh_schedule(data->bh);
+}
+
 static const VirtIOBlockOps virtio_blk_ops = (VirtIOBlockOps) {
-    .complete_request = virtio_blk_complete_request,
+    .complete_request           = virtio_blk_complete_request,
+    .pause                      = virtio_blk_pause,
+    .resume                     = virtio_blk_resume,
 };
 
 static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
@@ -597,6 +627,9 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
     VirtIOBlockReq *req;
     MultiReqBuffer mrb = {};
 
+    if (s->paused) {
+        return;
+    }
     /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
      * dataplane here instead of waiting for .set_status().
      */
@@ -787,7 +820,7 @@ static void virtio_blk_save(QEMUFile *f, void *opaque)
 
     virtio_save(vdev, f);
 }
-    
+
 static void virtio_blk_save_device(VirtIODevice *vdev, QEMUFile *f)
 {
     VirtIOBlock *s = VIRTIO_BLK(vdev);
@@ -875,6 +908,29 @@ static void virtio_blk_migration_state_changed(Notifier *notifier, void *data)
     }
 }
 
+static void virtio_blk_op_blocker_changed(Notifier *notifier, void *opaque)
+{
+    BlockOpEvent *event = opaque;
+    VirtIOBlock *s = container_of(notifier, VirtIOBlock,
+                                  op_blocker_notifier);
+    bool pause;
+
+    if (event->type != BLOCK_OP_TYPE_DEVICE_IO) {
+        return;
+    }
+    pause = event->blocking || blk_op_is_blocked(s->blk,
+                                                    BLOCK_OP_TYPE_DEVICE_IO,
+                                                    NULL);
+    if (pause == s->paused) {
+        return;
+    }
+    if (pause) {
+        s->ops->pause(s);
+    } else {
+        s->ops->resume(s);
+    }
+}
+
 static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -926,6 +982,9 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     blk_set_guest_block_size(s->blk, s->conf.conf.logical_block_size);
 
     blk_iostatus_enable(s->blk);
+
+    s->op_blocker_notifier.notify = virtio_blk_op_blocker_changed;
+    blk_op_blocker_add_notifier(s->blk, &s->op_blocker_notifier);
 }
 
 static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp)
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 28b3436..aa15fea 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -42,12 +42,16 @@ struct VirtIOBlkConf
 };
 
 struct VirtIOBlockDataPlane;
-
+struct VirtIOBlock;
 struct VirtIOBlockReq;
 
 typedef struct {
     /* Function to push to vq and notify guest */
     void (*complete_request)(struct VirtIOBlockReq *req, unsigned char status);
+
+    /* Functions to pause/resume request handling */
+    void (*pause)(struct VirtIOBlock *vblk);
+    void (*resume)(struct VirtIOBlock *vblk);
 } VirtIOBlockOps;
 
 typedef struct VirtIOBlock {
@@ -62,6 +66,8 @@ typedef struct VirtIOBlock {
     VMChangeStateEntry *change;
     const VirtIOBlockOps *ops;
     Notifier migration_state_notifier;
+    Notifier op_blocker_notifier;
+    bool paused;
     struct VirtIOBlockDataPlane *dataplane;
 } VirtIOBlock;
 
-- 
1.9.3

Re: [Qemu-devel] [RFC PATCH 6/7] virtio-blk: Don't handle output when there is "device IO" op blocker

[Paolo Bonzini]

On 06/05/2015 13:23, Fam Zheng wrote:
> virtio-blk now listens to op blocker change of the associated block
> backend.
> 
> Up on setting op blocker on BLOCK_OP_TYPE_DEVICE_IO:
> 
>   non-dataplane:
>    1) Set VirtIOBlock.paused
>    2) In virtio_blk_handle_output, do nothing if VirtIOBlock.paused
> 
>   dataplane:
>    1) Clear the host event notifier
>    2) In handle_notify, do nothing if VirtIOBlock.paused
> 
> Up on removing the op blocker:
> 
>   non-dataplane:
>    1) Clear VirtIOBlock.paused
>    2) Schedule a BH on the AioContext of the backend, which calls
>    virtio_blk_handle_output, so that the previous unhandled kicks can
>    make progress
> 
>   dataplane:
>    1) Set the host event notifier
>    2) Notify the host event notifier so that unhandled events are
>    processed
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

Does non-dataplane need to do anything, since it uses iohandlers rather
than aio_set_event_notifier_handler?

Paolo

Re: [Qemu-devel] [RFC PATCH 6/7] virtio-blk: Don't handle output when there is "device IO" op blocker

[Fam Zheng]

On Wed, 05/06 14:07, Paolo Bonzini wrote:
> 
> 
> On 06/05/2015 13:23, Fam Zheng wrote:
> > virtio-blk now listens to op blocker change of the associated block
> > backend.
> > 
> > Up on setting op blocker on BLOCK_OP_TYPE_DEVICE_IO:
> > 
> >   non-dataplane:
> >    1) Set VirtIOBlock.paused
> >    2) In virtio_blk_handle_output, do nothing if VirtIOBlock.paused
> > 
> >   dataplane:
> >    1) Clear the host event notifier
> >    2) In handle_notify, do nothing if VirtIOBlock.paused
> > 
> > Up on removing the op blocker:
> > 
> >   non-dataplane:
> >    1) Clear VirtIOBlock.paused
> >    2) Schedule a BH on the AioContext of the backend, which calls
> >    virtio_blk_handle_output, so that the previous unhandled kicks can
> >    make progress
> > 
> >   dataplane:
> >    1) Set the host event notifier
> >    2) Notify the host event notifier so that unhandled events are
> >    processed
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> Does non-dataplane need to do anything, since it uses iohandlers rather
> than aio_set_event_notifier_handler?

I guess it's not for this specific bug. See this as an attempt on a general
purpose "pause" mechanism to the device in investment for the future, for
example, bdrv_aio_drain. ;-)

I can drop it in v2 if you think the idea is not very helpful.

Fam

Re: [Qemu-devel] [RFC PATCH 6/7] virtio-blk: Don't handle output when there is "device IO" op blocker

[Paolo Bonzini]

On 06/05/2015 14:20, Fam Zheng wrote:
>> > 
>> > Does non-dataplane need to do anything, since it uses iohandlers rather
>> > than aio_set_event_notifier_handler?
> I guess it's not for this specific bug. See this as an attempt on a general
> purpose "pause" mechanism to the device in investment for the future, for
> example, bdrv_aio_drain. ;-)
> 
> I can drop it in v2 if you think the idea is not very helpful.

It's slightly more complicated but I think it's worth the extra coverage
testing that this provides.

Paolo

[Qemu-devel] [RFC PATCH 7/7] blockdev: Add "device IO" op blocker during snapshot transaction

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 5eaf77e..859fa2e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1398,6 +1398,7 @@ typedef struct ExternalSnapshotState {
     BlockDriverState *old_bs;
     BlockDriverState *new_bs;
     AioContext *aio_context;
+    Error *blocker;
 } ExternalSnapshotState;
 
 static void external_snapshot_prepare(BlkTransactionState *common,
@@ -1519,6 +1520,9 @@ static void external_snapshot_prepare(BlkTransactionState *common,
     if (ret != 0) {
         error_propagate(errp, local_err);
     }
+
+    error_setg(&state->blocker, "snapshot in progress");
+    bdrv_op_block(state->old_bs, BLOCK_OP_TYPE_DEVICE_IO, state->blocker);
 }
 
 static void external_snapshot_commit(BlkTransactionState *common)
@@ -1536,6 +1540,9 @@ static void external_snapshot_commit(BlkTransactionState *common)
     bdrv_reopen(state->new_bs, state->new_bs->open_flags & ~BDRV_O_RDWR,
                 NULL);
 
+    bdrv_op_unblock(state->old_bs, BLOCK_OP_TYPE_DEVICE_IO, state->blocker);
+    error_free(state->blocker);
+
     aio_context_release(state->aio_context);
 }
 
-- 
1.9.3

Re: [Qemu-devel] [Qemu-block] [RFC PATCH 0/7] Fix transactional snapshot with virtio-blk dataplane

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2662 bytes --]

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().

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [RFC PATCH 0/7] Fix transactional snapshot with virtio-blk dataplane

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2899 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]