| Message ID | 20160920123138.5400.67174.stgit@PASHA-ISP.def.inno |
|---|---|
| State | New |
| Headers | show |
On 20/09/2016 14:31, Pavel Dovgalyuk wrote: > /* Open the image file */ > bs->file = bdrv_open_child(NULL, options, "image", > @@ -40,6 +112,43 @@ static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags, > goto fail; > } > > + opts = qemu_opts_create(&blkreplay_runtime_opts, NULL, 0, &error_abort); > + qemu_opts_absorb_qdict(opts, options, &local_err); > + if (local_err) { > + ret = -EINVAL; > + goto fail; > + } > + > + /* 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")); > + > + snapshot = qemu_opt_get(opts, "overlay"); > + if (snapshot) { > + qdict_put(snapshot_options, "file.filename", > + qstring_from_str(snapshot)); > + } else { > + char tmp_filename[PATH_MAX + 1]; > + ret = get_tmp_filename(tmp_filename, PATH_MAX + 1); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Could not get temporary filename"); > + goto fail; > + } No, this is unnecessary. The image is unused in this case, so you can specify the overlay as image=foo.qcow2. Paolo
> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > On 20/09/2016 14:31, Pavel Dovgalyuk wrote: > > /* Open the image file */ > > bs->file = bdrv_open_child(NULL, options, "image", > > @@ -40,6 +112,43 @@ static int blkreplay_open(BlockDriverState *bs, QDict *options, int > flags, > > goto fail; > > } > > > > + opts = qemu_opts_create(&blkreplay_runtime_opts, NULL, 0, &error_abort); > > + qemu_opts_absorb_qdict(opts, options, &local_err); > > + if (local_err) { > > + ret = -EINVAL; > > + goto fail; > > + } > > + > > + /* 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")); > > + > > + snapshot = qemu_opt_get(opts, "overlay"); > > + if (snapshot) { > > + qdict_put(snapshot_options, "file.filename", > > + qstring_from_str(snapshot)); > > + } else { > > + char tmp_filename[PATH_MAX + 1]; > > + ret = get_tmp_filename(tmp_filename, PATH_MAX + 1); > > + if (ret < 0) { > > + error_setg_errno(errp, -ret, "Could not get temporary filename"); > > + goto fail; > > + } > > No, this is unnecessary. The image is unused in this case, so you can > specify the overlay as image=foo.qcow2. This branch allows user do not bother about overlays at all. Driver will automatically create temporary snapshot. Pavel Dovgalyuk
On 20/09/2016 14:43, Pavel Dovgalyuk wrote: > > No, this is unnecessary. The image is unused in this case, so you can > > specify the overlay as image=foo.qcow2. > > This branch allows user do not bother about overlays at all. > Driver will automatically create temporary snapshot. See other message. There is already a way to do this in QEMU, which is -snapshot. I think it's a requirement that -snapshot works, and also that your temporary overlay is implemented on top of -snapshot. You can add some magic ways to enable -snapshot if you want, of course. Paolo
[ Cc: qemu-block ] Am 20.09.2016 um 14:31 hat Pavel Dovgalyuk geschrieben: > This patch adds overlay option for blkreplay filter. > It allows creating persistent overlay file for saving and reloading > VM snapshots in record/replay modes. > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> > --- > block/blkreplay.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > docs/replay.txt | 8 ++++ > vl.c | 2 - > 3 files changed, 128 insertions(+), 1 deletion(-) > > diff --git a/block/blkreplay.c b/block/blkreplay.c > index 30f9d5f..e90d2c6 100644 > --- a/block/blkreplay.c > +++ b/block/blkreplay.c > @@ -14,6 +14,7 @@ > #include "block/block_int.h" > #include "sysemu/replay.h" > #include "qapi/error.h" > +#include "qapi/qmp/qstring.h" > > typedef struct Request { > Coroutine *co; > @@ -25,11 +26,82 @@ typedef struct Request { > block devices should not get overlapping ids. */ > static uint64_t request_id; > > +static BlockDriverState *blkreplay_append_snapshot(BlockDriverState *bs, > + int flags, > + QDict *snapshot_options, > + Error **errp) > +{ > + int ret; > + BlockDriverState *bs_snapshot; > + > + /* Create temporary file, if needed */ > + if ((flags & BDRV_O_TEMPORARY) || replay_mode == REPLAY_MODE_RECORD) { > + int64_t total_size; > + QemuOpts *opts = NULL; > + const char *tmp_filename = qdict_get_str(snapshot_options, > + "file.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, flags, 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 QemuOptsList blkreplay_runtime_opts = { > + .name = "blkreplay", > + .head = QTAILQ_HEAD_INITIALIZER(blkreplay_runtime_opts.head), > + .desc = { > + { > + .name = "overlay", > + .type = QEMU_OPT_STRING, > + .help = "Optional overlay file for snapshots", > + }, > + { /* end of list */ } > + }, > +}; > + > static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags, > Error **errp) > { > Error *local_err = NULL; > int ret; > + QDict *snapshot_options = qdict_new(); > + int snapshot_flags = BDRV_O_RDWR; > + const char *snapshot; > + QemuOpts *opts = NULL; > > /* Open the image file */ > bs->file = bdrv_open_child(NULL, options, "image", > @@ -40,6 +112,43 @@ static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags, > goto fail; > } > > + opts = qemu_opts_create(&blkreplay_runtime_opts, NULL, 0, &error_abort); > + qemu_opts_absorb_qdict(opts, options, &local_err); > + if (local_err) { > + ret = -EINVAL; > + goto fail; > + } Starting from here... > + /* 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")); > + > + snapshot = qemu_opt_get(opts, "overlay"); > + if (snapshot) { > + qdict_put(snapshot_options, "file.filename", > + qstring_from_str(snapshot)); > + } else { > + char tmp_filename[PATH_MAX + 1]; > + ret = get_tmp_filename(tmp_filename, PATH_MAX + 1); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Could not get temporary filename"); > + goto fail; > + } > + qdict_put(snapshot_options, "file.filename", > + qstring_from_str(tmp_filename)); > + snapshot_flags |= BDRV_O_TEMPORARY; > + } > + > + /* Add temporary snapshot to preserve the image */ > + if (!blkreplay_append_snapshot(bs->file->bs, snapshot_flags, > + snapshot_options, &local_err)) { > + ret = -EINVAL; > + error_propagate(errp, local_err); > + goto fail; > + } ...to here, this is code that is written according to a fundamentally wrong design. The problem here is that you're hard-coding the options that are used for the overlay image. This restricts users to using only qcow2 (there are many other formats that support backing files), only on local files (there are quite a few more protocols over which qcow2 works), only with default options (e.g. cache mode, discard behaviour, lazy refcounts). The correct way would be to get not a filename option, but what is called a BlockdevRef in QAPI: Either a node-name of a separately created BDS or an inline definition of a new BDS. Have a look at other filter drivers for how to do this. The thing to look for is bdrv_open_child() (though of course an overlay may look a bit different from this). > + > ret = 0; > fail: > if (ret < 0) { Kevin
> From: Kevin Wolf [mailto:kwolf@redhat.com] > Am 20.09.2016 um 14:31 hat Pavel Dovgalyuk geschrieben: > > This patch adds overlay option for blkreplay filter. > > It allows creating persistent overlay file for saving and reloading > > VM snapshots in record/replay modes. > > > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> > > --- > > block/blkreplay.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > > docs/replay.txt | 8 ++++ > > vl.c | 2 - > > 3 files changed, 128 insertions(+), 1 deletion(-) > > > > diff --git a/block/blkreplay.c b/block/blkreplay.c > > index 30f9d5f..e90d2c6 100644 > > --- a/block/blkreplay.c > > +++ b/block/blkreplay.c > > @@ -14,6 +14,7 @@ > > #include "block/block_int.h" > > #include "sysemu/replay.h" > > #include "qapi/error.h" > > +#include "qapi/qmp/qstring.h" > > > > typedef struct Request { > > Coroutine *co; > > @@ -25,11 +26,82 @@ typedef struct Request { > > block devices should not get overlapping ids. */ > > static uint64_t request_id; > > > > +static BlockDriverState *blkreplay_append_snapshot(BlockDriverState *bs, > > + int flags, > > + QDict *snapshot_options, > > + Error **errp) > > +{ > > + int ret; > > + BlockDriverState *bs_snapshot; > > + > > + /* Create temporary file, if needed */ > > + if ((flags & BDRV_O_TEMPORARY) || replay_mode == REPLAY_MODE_RECORD) { > > + int64_t total_size; > > + QemuOpts *opts = NULL; > > + const char *tmp_filename = qdict_get_str(snapshot_options, > > + "file.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, flags, 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 QemuOptsList blkreplay_runtime_opts = { > > + .name = "blkreplay", > > + .head = QTAILQ_HEAD_INITIALIZER(blkreplay_runtime_opts.head), > > + .desc = { > > + { > > + .name = "overlay", > > + .type = QEMU_OPT_STRING, > > + .help = "Optional overlay file for snapshots", > > + }, > > + { /* end of list */ } > > + }, > > +}; > > + > > static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags, > > Error **errp) > > { > > Error *local_err = NULL; > > int ret; > > + QDict *snapshot_options = qdict_new(); > > + int snapshot_flags = BDRV_O_RDWR; > > + const char *snapshot; > > + QemuOpts *opts = NULL; > > > > /* Open the image file */ > > bs->file = bdrv_open_child(NULL, options, "image", > > @@ -40,6 +112,43 @@ static int blkreplay_open(BlockDriverState *bs, QDict *options, int > flags, > > goto fail; > > } > > > > + opts = qemu_opts_create(&blkreplay_runtime_opts, NULL, 0, &error_abort); > > + qemu_opts_absorb_qdict(opts, options, &local_err); > > + if (local_err) { > > + ret = -EINVAL; > > + goto fail; > > + } > > Starting from here... > > > + /* 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")); > > + > > + snapshot = qemu_opt_get(opts, "overlay"); > > + if (snapshot) { > > + qdict_put(snapshot_options, "file.filename", > > + qstring_from_str(snapshot)); > > + } else { > > + char tmp_filename[PATH_MAX + 1]; > > + ret = get_tmp_filename(tmp_filename, PATH_MAX + 1); > > + if (ret < 0) { > > + error_setg_errno(errp, -ret, "Could not get temporary filename"); > > + goto fail; > > + } > > + qdict_put(snapshot_options, "file.filename", > > + qstring_from_str(tmp_filename)); > > + snapshot_flags |= BDRV_O_TEMPORARY; > > + } > > + > > + /* Add temporary snapshot to preserve the image */ > > + if (!blkreplay_append_snapshot(bs->file->bs, snapshot_flags, > > + snapshot_options, &local_err)) { > > + ret = -EINVAL; > > + error_propagate(errp, local_err); > > + goto fail; > > + } > > ...to here, this is code that is written according to a fundamentally > wrong design. > > The problem here is that you're hard-coding the options that are used > for the overlay image. This restricts users to using only qcow2 (there > are many other formats that support backing files), only on local files > (there are quite a few more protocols over which qcow2 works), only with > default options (e.g. cache mode, discard behaviour, lazy refcounts). > > The correct way would be to get not a filename option, but what is > called a BlockdevRef in QAPI: Either a node-name of a separately created > BDS or an inline definition of a new BDS. Have a look at other filter > drivers for how to do this. The thing to look for is bdrv_open_child() > (though of course an overlay may look a bit different from this). Thanks. Then I'll better apply Paolo's approach to creating overlays. Pavel Dovgalyuk
diff --git a/block/blkreplay.c b/block/blkreplay.c index 30f9d5f..e90d2c6 100644 --- a/block/blkreplay.c +++ b/block/blkreplay.c @@ -14,6 +14,7 @@ #include "block/block_int.h" #include "sysemu/replay.h" #include "qapi/error.h" +#include "qapi/qmp/qstring.h" typedef struct Request { Coroutine *co; @@ -25,11 +26,82 @@ typedef struct Request { block devices should not get overlapping ids. */ static uint64_t request_id; +static BlockDriverState *blkreplay_append_snapshot(BlockDriverState *bs, + int flags, + QDict *snapshot_options, + Error **errp) +{ + int ret; + BlockDriverState *bs_snapshot; + + /* Create temporary file, if needed */ + if ((flags & BDRV_O_TEMPORARY) || replay_mode == REPLAY_MODE_RECORD) { + int64_t total_size; + QemuOpts *opts = NULL; + const char *tmp_filename = qdict_get_str(snapshot_options, + "file.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, flags, 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 QemuOptsList blkreplay_runtime_opts = { + .name = "blkreplay", + .head = QTAILQ_HEAD_INITIALIZER(blkreplay_runtime_opts.head), + .desc = { + { + .name = "overlay", + .type = QEMU_OPT_STRING, + .help = "Optional overlay file for snapshots", + }, + { /* end of list */ } + }, +}; + static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { Error *local_err = NULL; int ret; + QDict *snapshot_options = qdict_new(); + int snapshot_flags = BDRV_O_RDWR; + const char *snapshot; + QemuOpts *opts = NULL; /* Open the image file */ bs->file = bdrv_open_child(NULL, options, "image", @@ -40,6 +112,43 @@ static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags, goto fail; } + opts = qemu_opts_create(&blkreplay_runtime_opts, NULL, 0, &error_abort); + qemu_opts_absorb_qdict(opts, options, &local_err); + if (local_err) { + ret = -EINVAL; + goto fail; + } + + /* 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")); + + snapshot = qemu_opt_get(opts, "overlay"); + if (snapshot) { + qdict_put(snapshot_options, "file.filename", + qstring_from_str(snapshot)); + } else { + char tmp_filename[PATH_MAX + 1]; + ret = get_tmp_filename(tmp_filename, PATH_MAX + 1); + if (ret < 0) { + error_setg_errno(errp, -ret, "Could not get temporary filename"); + goto fail; + } + qdict_put(snapshot_options, "file.filename", + qstring_from_str(tmp_filename)); + snapshot_flags |= BDRV_O_TEMPORARY; + } + + /* Add temporary snapshot to preserve the image */ + if (!blkreplay_append_snapshot(bs->file->bs, snapshot_flags, + snapshot_options, &local_err)) { + ret = -EINVAL; + error_propagate(errp, local_err); + goto fail; + } + ret = 0; fail: if (ret < 0) { @@ -50,6 +159,7 @@ fail: static void blkreplay_close(BlockDriverState *bs) { + bdrv_unref(bs->file->bs); } static int64_t blkreplay_getlength(BlockDriverState *bs) @@ -135,6 +245,12 @@ static int coroutine_fn blkreplay_co_flush(BlockDriverState *bs) return ret; } +static bool blkreplay_recurse_is_first_non_filter(BlockDriverState *bs, + BlockDriverState *candidate) +{ + return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate); +} + static BlockDriver bdrv_blkreplay = { .format_name = "blkreplay", .protocol_name = "blkreplay", @@ -150,6 +266,9 @@ static BlockDriver bdrv_blkreplay = { .bdrv_co_pwrite_zeroes = blkreplay_co_pwrite_zeroes, .bdrv_co_pdiscard = blkreplay_co_pdiscard, .bdrv_co_flush = blkreplay_co_flush, + + .is_filter = true, + .bdrv_recurse_is_first_non_filter = blkreplay_recurse_is_first_non_filter, }; static void bdrv_blkreplay_init(void) diff --git a/docs/replay.txt b/docs/replay.txt index 347b2ff..5be8f25 100644 --- a/docs/replay.txt +++ b/docs/replay.txt @@ -196,6 +196,14 @@ 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. +blkdriver also supports overlay option, which allows creating persistent +overlay file for saving and reloading VM snapshots in record/replay modes. +Replay mechanism automatically creates one snapshot named 'replay_init' to +allow rewinding execution while replaying. +Overlay file may be specified as follows: + -drive driver=blkreplay,if=none,image=img-direct, + overlay=overlay.qcow2,id=img-blkreplay + Network devices --------------- diff --git a/vl.c b/vl.c index fd7f17e..9adca19 100644 --- a/vl.c +++ b/vl.c @@ -4410,7 +4410,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 adds overlay option for blkreplay filter. It allows creating persistent overlay file for saving and reloading VM snapshots in record/replay modes. Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> --- block/blkreplay.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++ docs/replay.txt | 8 ++++ vl.c | 2 - 3 files changed, 128 insertions(+), 1 deletion(-)