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

2015-05-06 Thread 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



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

2015-05-06 Thread 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 
> 
> 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

2015-05-06 Thread 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 

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

Paolo



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

2015-05-06 Thread 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 
---
 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