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

[Kevin Wolf]

Am 09.02.2016 um 06:55 hat Pavel Dovgalyuk geschrieben:
> This patch introduces a set of functions that implement recording
> and replaying of block devices' operations. These functions form a thin
> layer between blk_aio_ functions and replay subsystem.
> All asynchronous block requests are added to the queue to be processed
> at deterministically invoked record/replay checkpoints.
> 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 <address@hidden>

This series doesn't seem to apply to current master, so it's somewhat
hard to look at the end result. I can see just from patches, though,
that this will need some more discussion.

Just picking one example of how you convert blk_* functions:

> -BlockAIOCB *blk_aio_write_zeroes(BlockBackend *blk, int64_t sector_num,
> -                                 int nb_sectors, BdrvRequestFlags flags,
> -                                 BlockCompletionFunc *cb, void *opaque)
> +BlockAIOCB *blk_aio_write_zeroes_impl(BlockBackend *blk, int64_t sector_num,
> +                                      int nb_sectors, BdrvRequestFlags flags,
> +                                      BlockCompletionFunc *cb, void *opaque)
>  {
>      int ret = blk_check_request(blk, sector_num, nb_sectors);
>      if (ret < 0) {
> @@ -673,6 +674,13 @@ BlockAIOCB *blk_aio_write_zeroes(BlockBackend *blk, 
> int64_t sector_num,
>                                   cb, opaque);
>  }
>  
> +BlockAIOCB *blk_aio_write_zeroes(BlockBackend *blk, int64_t sector_num,
> +                                 int nb_sectors, BdrvRequestFlags flags,
> +                                 BlockCompletionFunc *cb, void *opaque)
> +{
> +    return replay_aio_write_zeroes(blk, sector_num, nb_sectors, flags, cb, 
> opaque);
> +}
> +

> +BlockAIOCB *replay_aio_write_zeroes(BlockBackend *blk, int64_t sector_num,
> +                                    int nb_sectors, BdrvRequestFlags flags,
> +                                    BlockCompletionFunc *cb, void *opaque)
> +{
> +    if (replay_mode == REPLAY_MODE_NONE) {
> +        return blk_aio_write_zeroes_impl(blk, sector_num, nb_sectors, flags, 
> cb, opaque);
> +    } else {
> +        ReplayAIOCB *acb = 
> replay_aio_create(REPLAY_ASYNC_EVENT_BLOCK_WRITE_ZEROES,
> +                                             blk, cb, opaque);
> +        acb->req.sector = sector_num;
> +        acb->req.nb_sectors = nb_sectors;
> +        acb->req.flags = flags;
> +        replay_add_event(REPLAY_ASYNC_EVENT_BLOCK_WRITE_ZEROES, acb, NULL, 
> 0);
> +
> +        return &acb->common;
> +    }
> +}

I think it's obvious that adding two functions to the call chain which
do nothing in the common case is a bit ugly. If we did this for every
feature that could possibly be enabled, we'd end up with two-kilometer
stack traces.

So definitely don't call into replay.c, which just calls back in 99.9%
of the cases, but if anything, do the check in block-backends.c.


But even this doesn't feel completely right, because block drivers are
already layered and there is no need to hardcode something optional (and
rarely used) in the hot code path that could just be another layer.

I assume that you know beforehand if you want to replay something, so
requiring you to configure your block devices with a replay driver on
top of the stack seems reasonable enough.

Kevin