From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54380) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adcWZ-0005gv-Ji for qemu-devel@nongnu.org; Wed, 09 Mar 2016 06:43:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adcWU-0000Bn-JP for qemu-devel@nongnu.org; Wed, 09 Mar 2016 06:43:31 -0500 Received: from mx1.redhat.com ([209.132.183.28]:50134) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adcWU-0000BK-Bl for qemu-devel@nongnu.org; Wed, 09 Mar 2016 06:43:26 -0500 Date: Wed, 9 Mar 2016 12:43:21 +0100 From: Kevin Wolf Message-ID: <20160309114321.GE5205@noname.redhat.com> References: <20160301110732.10104.2019.stgit@PASHA-ISP> <20160301110803.10104.77305.stgit@PASHA-ISP> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160301110803.10104.77305.stgit@PASHA-ISP> Subject: Re: [Qemu-devel] [PATCH v3 5/5] 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 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 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