| Message ID | 20170131115740.5244.87406.stgit@PASHA-ISP (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 31.01.2017 um 12:57 hat Pavel Dovgalyuk geschrieben: > This patch allows using '-snapshot' behavior in record/replay mode. > blkreplay layer creates temporary overlays on top of underlaying > disk images. It is needed, because creating an overlay over blkreplay > breaks the determinism. > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> Is replacing the '-snapshot' behaviour (which was automatically enabled before this patch) with custom code what this patch really does? In other words, does it fix that the old behaviour didn't work correctly rather than adding a new feature? If so, the commit message is prone to misunderstanding, I think. > diff --git a/block/blkreplay.c b/block/blkreplay.c > index 8a03d62..172642f 100644 > --- a/block/blkreplay.c > +++ b/block/blkreplay.c > @@ -14,12 +14,76 @@ > #include "block/block_int.h" > #include "sysemu/replay.h" > #include "qapi/error.h" > +#include "qapi/qmp/qstring.h" > > typedef struct Request { > Coroutine *co; > QEMUBH *bh; > } Request; > > +static BlockDriverState *blkreplay_append_snapshot(BlockDriverState *bs, > + Error **errp) > +{ > + int ret; > + BlockDriverState *bs_snapshot; > + int64_t total_size; > + QemuOpts *opts = NULL; > + char tmp_filename[PATH_MAX + 1]; > + QDict *snapshot_options = qdict_new(); > + > + /* Prepare options QDict for the overlay file */ > + qdict_put(snapshot_options, "file.driver", > + qstring_from_str("file")); > + qdict_put(snapshot_options, "driver", > + qstring_from_str("qcow2")); Both of these statements fit each in a single line. > + /* Create temporary file */ > + ret = get_tmp_filename(tmp_filename, PATH_MAX + 1); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Could not get temporary filename"); > + goto out; > + } > + qdict_put(snapshot_options, "file.filename", > + qstring_from_str(tmp_filename)); > + > + /* Get the required size from the image */ > + total_size = bdrv_getlength(bs); > + if (total_size < 0) { > + error_setg_errno(errp, -total_size, "Could not get image size"); > + goto out; > + } > + > + opts = qemu_opts_create(bdrv_qcow2.create_opts, NULL, 0, &error_abort); > + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size, &error_abort); > + ret = bdrv_create(&bdrv_qcow2, tmp_filename, opts, errp); > + qemu_opts_del(opts); > + if (ret < 0) { > + error_prepend(errp, "Could not create temporary overlay '%s': ", > + tmp_filename); > + goto out; > + } > + > + bs_snapshot = bdrv_open(NULL, NULL, snapshot_options, > + BDRV_O_RDWR | BDRV_O_TEMPORARY, errp); > + snapshot_options = NULL; > + if (!bs_snapshot) { > + ret = -EINVAL; The value of ret is unused, so why set it here? > + goto out; > + } > + > + /* bdrv_append() consumes a strong reference to bs_snapshot (i.e. it will > + * call bdrv_unref() on it), so in order to be able to return one, we have > + * to increase bs_snapshot's refcount here */ > + bdrv_ref(bs_snapshot); > + bdrv_append(bs_snapshot, bs); Actually, your case is exactly what bdrv_append() is designed for: You don't need to return a strong reference here, the caller just checks for NULL and that's it. So: 1. bdrv_open() return a strong reference (refcount = 1) 2. You do bdrv_ref() (refcount = 2) 3. bdrv_append() takes a reference and uses it for bs->file ... 4. bdrv_close() calls blkreplay_close(), which does a bdrv_unref() (refcount = 1) 5. bdrv_close() unrefs all child nodes (refcount = 0 --> delete) You can simplify this by just not doing the extra bdrv_ref/unref (i.e. remove steps 2 and 4). The correct reference counting is already taken care of by bdrv_append() and bdrv_close(). > + return bs_snapshot; > + > +out: > + QDECREF(snapshot_options); > + return NULL; > +} Kevin
diff --git a/block/blkreplay.c b/block/blkreplay.c index 8a03d62..172642f 100644 --- a/block/blkreplay.c +++ b/block/blkreplay.c @@ -14,12 +14,76 @@ #include "block/block_int.h" #include "sysemu/replay.h" #include "qapi/error.h" +#include "qapi/qmp/qstring.h" typedef struct Request { Coroutine *co; QEMUBH *bh; } Request; +static BlockDriverState *blkreplay_append_snapshot(BlockDriverState *bs, + Error **errp) +{ + int ret; + BlockDriverState *bs_snapshot; + int64_t total_size; + QemuOpts *opts = NULL; + char tmp_filename[PATH_MAX + 1]; + QDict *snapshot_options = qdict_new(); + + /* Prepare options QDict for the overlay file */ + qdict_put(snapshot_options, "file.driver", + qstring_from_str("file")); + qdict_put(snapshot_options, "driver", + qstring_from_str("qcow2")); + + /* Create temporary file */ + ret = get_tmp_filename(tmp_filename, PATH_MAX + 1); + if (ret < 0) { + error_setg_errno(errp, -ret, "Could not get temporary filename"); + goto out; + } + qdict_put(snapshot_options, "file.filename", + qstring_from_str(tmp_filename)); + + /* Get the required size from the image */ + total_size = bdrv_getlength(bs); + if (total_size < 0) { + error_setg_errno(errp, -total_size, "Could not get image size"); + goto out; + } + + opts = qemu_opts_create(bdrv_qcow2.create_opts, NULL, 0, &error_abort); + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size, &error_abort); + ret = bdrv_create(&bdrv_qcow2, tmp_filename, opts, errp); + qemu_opts_del(opts); + if (ret < 0) { + error_prepend(errp, "Could not create temporary overlay '%s': ", + tmp_filename); + goto out; + } + + bs_snapshot = bdrv_open(NULL, NULL, snapshot_options, + BDRV_O_RDWR | BDRV_O_TEMPORARY, errp); + snapshot_options = NULL; + if (!bs_snapshot) { + ret = -EINVAL; + goto out; + } + + /* bdrv_append() consumes a strong reference to bs_snapshot (i.e. it will + * call bdrv_unref() on it), so in order to be able to return one, we have + * to increase bs_snapshot's refcount here */ + bdrv_ref(bs_snapshot); + bdrv_append(bs_snapshot, bs); + + return bs_snapshot; + +out: + QDECREF(snapshot_options); + return NULL; +} + static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { @@ -35,6 +99,14 @@ static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags, goto fail; } + /* Add temporary snapshot to preserve the image */ + if (!replay_snapshot + && !blkreplay_append_snapshot(bs->file->bs, &local_err)) { + ret = -EINVAL; + error_propagate(errp, local_err); + goto fail; + } + ret = 0; fail: if (ret < 0) { @@ -45,6 +117,10 @@ fail: static void blkreplay_close(BlockDriverState *bs) { + if (!replay_snapshot) { + /* Unref created snapshot file */ + bdrv_unref(bs->file->bs); + } } static int64_t blkreplay_getlength(BlockDriverState *bs) diff --git a/stubs/replay.c b/stubs/replay.c index 9c8aa48..9991ee5 100644 --- a/stubs/replay.c +++ b/stubs/replay.c @@ -3,6 +3,7 @@ #include "sysemu/sysemu.h" ReplayMode replay_mode; +char *replay_snapshot; int64_t replay_save_clock(unsigned int kind, int64_t clock) { diff --git a/vl.c b/vl.c index 0b72b12..c6fc5c9 100644 --- a/vl.c +++ b/vl.c @@ -4414,7 +4414,7 @@ int main(int argc, char **argv, char **envp) } /* open the virtual block devices */ - if (snapshot || replay_mode != REPLAY_MODE_NONE) { + if (snapshot) { qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot, NULL, NULL); }
This patch allows using '-snapshot' behavior in record/replay mode. blkreplay layer creates temporary overlays on top of underlaying disk images. It is needed, because creating an overlay over blkreplay breaks the determinism. Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> --- block/blkreplay.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++ stubs/replay.c | 1 + vl.c | 2 + 3 files changed, 78 insertions(+), 1 deletion(-)