Re: [Qemu-devel] [PATCH v3 5/5] replay: introduce block devices record/replay

[Kevin Wolf <kwolf@redhat.com>]

Am 01.03.2016 um 12:08 hat Pavel Dovgalyuk geschrieben:
> This patch introduces block driver that implement recording
> and replaying of block devices' operations.
> All block completion operations are added to the queue.
> Queue is flushed at checkpoints and information about processed requests
> is recorded to the log. In replay phase the queue is matched with
> events read from the log. Therefore block devices requests are processed
> deterministically.
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>

I like this new version much better than the old one. :-)

> +static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags,
> +                          Error **errp)
> +{
> +    Error *local_err = NULL;
> +    int ret;
> +
> +    /* Open the image file */
> +    bs->file = bdrv_open_child(NULL, options, "image",
> +                               bs, &child_file, false, &local_err);
> +    if (local_err) {
> +        ret = -EINVAL;
> +        error_propagate(errp, local_err);
> +        goto fail;
> +    }
> +
> +    ret = 0;
> +fail:
> +    if (ret < 0) {
> +        bdrv_unref_child(bs, bs->file);

This is unnecessary because in error cases, bdrv_open_child() returns
NULL. On the other hand, bdrv_unref_child() accepts a NULL child and
just doesn't do anything then, so it's harmless.

> +    }
> +    return ret;
> +}
> +
> +static void blkreplay_close(BlockDriverState *bs)
> +{
> +}
> +
> +static int64_t blkreplay_getlength(BlockDriverState *bs)
> +{
> +    return bdrv_getlength(bs->file->bs);
> +}
> +
> +static void blkreplay_bh_cb(void *opaque)
> +{
> +    Request *req = opaque;
> +    qemu_coroutine_enter(req->co, NULL);
> +    qemu_bh_delete(req->bh);
> +    g_free(req);
> +}

Maybe add a comment here that explains why the BH exists (forcing the
coroutine into its I/O thread, if I understand correctly).

> +static void block_request_create(uint64_t reqid, BlockDriverState *bs,
> +                                 Coroutine *co)
> +{
> +    Request *req = g_malloc0(sizeof(Request));
> +    req->co = co;
> +    req->bh = aio_bh_new(bdrv_get_aio_context(bs), blkreplay_bh_cb, req);

g_malloc0() zeroes all fields and then you immediately overwrite them,
so this is useless work. If you just want to be protected when adding
new fields, you could consider this instead:

Request *req = g_new(Request, 1);
*req = (Request) {
    .co = co,
    .bh = aio_bh_new(bdrv_get_aio_context(bs), blkreplay_bh_cb, req),
};

It's just a suggestion, though, feel free to change it or leave it.

> +    replay_block_event(req->bh, reqid);
> +}
> +
> +static int coroutine_fn blkreplay_co_readv(BlockDriverState *bs,
> +    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
> +{
> +    uint64_t reqid = request_id++;
> +
> +    bdrv_co_readv(bs->file->bs, sector_num, nb_sectors, qiov);
> +    block_request_create(reqid, bs, qemu_coroutine_self());
> +    qemu_coroutine_yield();
> +
> +    return 0;
> +}

Error handling is broken here. You need to forward the return value of
bdrv_co_readv() to the caller.

> +
> +static int coroutine_fn blkreplay_co_writev(BlockDriverState *bs,
> +    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
> +{
> +    uint64_t reqid = request_id++;
> +
> +    bdrv_co_writev(bs->file->bs, sector_num, nb_sectors, qiov);
> +    block_request_create(reqid, bs, qemu_coroutine_self());
> +    qemu_coroutine_yield();
> +
> +    return 0;
> +}

Same here.

> +static int coroutine_fn blkreplay_co_write_zeroes(BlockDriverState *bs,
> +    int64_t sector_num, int nb_sectors, BdrvRequestFlags flags)
> +{
> +    uint64_t reqid = request_id++;
> +
> +    bdrv_co_write_zeroes(bs->file->bs, sector_num, nb_sectors, flags);
> +    block_request_create(reqid, bs, qemu_coroutine_self());
> +    qemu_coroutine_yield();
> +
> +    return 0;
> +}

And here.

> +static int coroutine_fn blkreplay_co_discard(BlockDriverState *bs,
> +    int64_t sector_num, int nb_sectors)
> +{
> +    uint64_t reqid = request_id++;
> +
> +    bdrv_co_discard(bs->file->bs, sector_num, nb_sectors);
> +    block_request_create(reqid, bs, qemu_coroutine_self());
> +    qemu_coroutine_yield();
> +
> +    return 0;
> +}

Here, too.

> +static int coroutine_fn blkreplay_co_flush(BlockDriverState *bs)
> +{
> +    uint64_t reqid = request_id++;
> +
> +    bdrv_co_flush(bs->file->bs);
> +    block_request_create(reqid, bs, qemu_coroutine_self());
> +    qemu_coroutine_yield();
> +
> +    return 0;
> +}

And finally here as well.

Kevin