From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39210) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aT6qw-00077A-Fv for qemu-devel@nongnu.org; Tue, 09 Feb 2016 06:53:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aT6qr-0006pu-8t for qemu-devel@nongnu.org; Tue, 09 Feb 2016 06:53:06 -0500 Received: from mail.ispras.ru ([83.149.199.45]:58977) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aT6qq-0006po-MN for qemu-devel@nongnu.org; Tue, 09 Feb 2016 06:53:01 -0500 From: "Pavel Dovgalyuk" References: <20160209055506.8208.67.stgit@PASHA-ISP> <20160209055524.8208.16023.stgit@PASHA-ISP> <20160209102739.GB8554@noname.redhat.com> In-Reply-To: <20160209102739.GB8554@noname.redhat.com> Date: Tue, 9 Feb 2016 14:52:59 +0300 Message-ID: <001201d16330$6ce67e40$46b37ac0$@Dovgaluk@ispras.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Language: ru Subject: Re: [Qemu-devel] [PATCH 3/3] replay: introduce block devices record/replay List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: 'Kevin Wolf' Cc: edgar.iglesias@xilinx.com, peter.maydell@linaro.org, igor.rubinov@gmail.com, mark.burton@greensocs.com, real@ispras.ru, hines@cert.org, qemu-devel@nongnu.org, maria.klimushenkova@ispras.ru, stefanha@redhat.com, pbonzini@redhat.com, batuzovk@ispras.ru, alex.bennee@linaro.org, fred.konrad@greensocs.com > 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 > > 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