From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42163) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aT5WO-0000u1-Ru for qemu-devel@nongnu.org; Tue, 09 Feb 2016 05:27:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aT5WL-0001ky-FR for qemu-devel@nongnu.org; Tue, 09 Feb 2016 05:27:48 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39061) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aT5WL-0001kp-7H for qemu-devel@nongnu.org; Tue, 09 Feb 2016 05:27:45 -0500 Date: Tue, 9 Feb 2016 11:27:39 +0100 From: Kevin Wolf Message-ID: <20160209102739.GB8554@noname.redhat.com> References: <20160209055506.8208.67.stgit@PASHA-ISP> <20160209055524.8208.16023.stgit@PASHA-ISP> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160209055524.8208.16023.stgit@PASHA-ISP> 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: Pavel Dovgalyuk 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 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. 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