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

["Pavel Dovgalyuk" <Pavel.Dovgaluk@ispras.ru>]

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> 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 <pavel.dovgaluk@ispras.ru>
> 
> 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.

Thank you for the response.
I forgot to rebase these patches, but there are minor problems with applying them.

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

This way seems to be better.

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

I cannot use block drivers for this. When driver functions are used, QEMU
is already used coroutines (and probably started bottom halves).
Coroutines make execution non-deterministic.
That's why we have to intercept blk_aio_ functions, that are called
deterministically.

Pavel Dovgalyuk