| Message ID | 1431669868-26804-6-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 15.05.2015 08:04, 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> > --- > 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 e287e09..a5e8e35 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); Indentation is off if you intended to indent based on the opening parenthesis. Different question: Since event->type == BLOCK_OP_TYPE_DEVICE_IO, when will event->blocking ever be not equal to blk_op_is_blocked()? > + 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); This is indeed a way to make sure the notifier is not leaked. *cough* I don't know why I never think of that... Apart from the event->blocking question above, looks good to me (although that's a pretty weak statement, considering I don't know the virtio code that well...). Max > } > > 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; >
On Mon, 05/18 20:19, Max Reitz wrote: > On 15.05.2015 08:04, Fam Zheng wrote: > > Indentation is off if you intended to indent based on the opening > parenthesis. Will fix. > > Different question: Since event->type == BLOCK_OP_TYPE_DEVICE_IO, when will > event->blocking ever be not equal to blk_op_is_blocked()? It's unnecessary. Fam
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index e287e09..a5e8e35 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;
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(-)