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.dovga...@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
