Re: [Qemu-block] [PATCH v3 05/12] virtio-blk: Don't handle output when there is "device IO" op blocker

[Max Reitz]

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 <address@hidden>
> ---
>   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;
>