Re: [PATCH v2 3/3] Use g_new() & friends where that makes obvious sense
On 15.03.2022 17:41, Markus Armbruster wrote:
g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
for two reasons. One, it catches multiplication overflowing size_t.
Two, it returns T * rather than void *, which lets the compiler catch
more type errors.
This commit only touches allocations with size arguments of the form
sizeof(T).
Patch created mechanically with:
$ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \
--macro-file scripts/cocci-macro-file.h FILES...
Signed-off-by: Markus Armbruster
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Reviewed-by: Alex Bennée
Acked-by: Dr. David Alan Gilbert
---
replay/replay-char.c | 4 +--
replay/replay-events.c | 10 +++---
Reviewed-by: Pavel Dovgalyuk
diff --git a/replay/replay-char.c b/replay/replay-char.c
index dc0002367e..d2025948cf 100644
--- a/replay/replay-char.c
+++ b/replay/replay-char.c
@@ -50,7 +50,7 @@ void replay_register_char_driver(Chardev *chr)
void replay_chr_be_write(Chardev *s, uint8_t *buf, int len)
{
-CharEvent *event = g_malloc0(sizeof(CharEvent));
+CharEvent *event = g_new0(CharEvent, 1);
event->id = find_char_driver(s);
if (event->id < 0) {
@@ -85,7 +85,7 @@ void replay_event_char_read_save(void *opaque)
void *replay_event_char_read_load(void)
{
-CharEvent *event = g_malloc0(sizeof(CharEvent));
+CharEvent *event = g_new0(CharEvent, 1);
event->id = replay_get_byte();
replay_get_array_alloc(>buf, >len);
diff --git a/replay/replay-events.c b/replay/replay-events.c
index 15983dd250..ac47c89834 100644
--- a/replay/replay-events.c
+++ b/replay/replay-events.c
@@ -119,7 +119,7 @@ void replay_add_event(ReplayAsyncEventKind event_kind,
return;
}
-Event *event = g_malloc0(sizeof(Event));
+Event *event = g_new0(Event, 1);
event->event_kind = event_kind;
event->opaque = opaque;
event->opaque2 = opaque2;
@@ -243,17 +243,17 @@ static Event *replay_read_event(int checkpoint)
}
break;
case REPLAY_ASYNC_EVENT_INPUT:
-event = g_malloc0(sizeof(Event));
+event = g_new0(Event, 1);
event->event_kind = replay_state.read_event_kind;
event->opaque = replay_read_input_event();
return event;
case REPLAY_ASYNC_EVENT_INPUT_SYNC:
-event = g_malloc0(sizeof(Event));
+event = g_new0(Event, 1);
event->event_kind = replay_state.read_event_kind;
event->opaque = 0;
return event;
case REPLAY_ASYNC_EVENT_CHAR_READ:
-event = g_malloc0(sizeof(Event));
+event = g_new0(Event, 1);
event->event_kind = replay_state.read_event_kind;
event->opaque = replay_event_char_read_load();
return event;
@@ -263,7 +263,7 @@ static Event *replay_read_event(int checkpoint)
}
break;
case REPLAY_ASYNC_EVENT_NET:
-event = g_malloc0(sizeof(Event));
+event = g_new0(Event, 1);
event->event_kind = replay_state.read_event_kind;
event->opaque = replay_event_net_load();
return event;
Re: [PATCH 2/3] migration: Make save_snapshot() return bool, not 0/-1
On 08.10.2020 20:48, Philippe Mathieu-Daudé wrote:
Just for consistency, following the example documented since
commit e3fe3988d7 ("error: Document Error API usage rules").
Document the function.
Signed-off-by: Philippe Mathieu-Daudé
Acked-by: Pavel Dovgalyuk
---
include/migration/snapshot.h | 9 -
migration/savevm.c | 16
replay/replay-debugging.c| 2 +-
replay/replay-snapshot.c | 2 +-
4 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/include/migration/snapshot.h b/include/migration/snapshot.h
index c85b6ec75b..a40c34307b 100644
--- a/include/migration/snapshot.h
+++ b/include/migration/snapshot.h
@@ -15,7 +15,14 @@
#ifndef QEMU_MIGRATION_SNAPSHOT_H
#define QEMU_MIGRATION_SNAPSHOT_H
-int save_snapshot(const char *name, Error **errp);
+/**
+ * save_snapshot: Save a snapshot.
+ * @name: path to snapshot
+ * @errp: pointer to error object
+ * On success, return %true.
+ * On failure, store an error through @errp and return %false.
+ */
+bool save_snapshot(const char *name, Error **errp);
int load_snapshot(const char *name, Error **errp);
#endif
diff --git a/migration/savevm.c b/migration/savevm.c
index a52da440f4..fd2e5e8b66 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2658,7 +2658,7 @@ int qemu_load_device_state(QEMUFile *f)
return 0;
}
-int save_snapshot(const char *name, Error **errp)
+bool save_snapshot(const char *name, Error **errp)
{
BlockDriverState *bs;
QEMUSnapshotInfo sn1, *sn = , old_sn1, *old_sn = _sn1;
@@ -2671,29 +2671,29 @@ int save_snapshot(const char *name, Error **errp)
AioContext *aio_context;
if (migration_is_blocked(errp)) {
-return ret;
+return false;
}
if (!replay_can_snapshot()) {
error_setg(errp, "Record/replay does not allow making snapshot "
"right now. Try once more later.");
-return ret;
+return false;
}
if (!bdrv_all_can_snapshot(errp)) {
-return ret;
+return false;
}
/* Delete old snapshots of the same name */
if (name) {
if (bdrv_all_delete_snapshot(name, errp) < 0) {
-return ret;
+return false;
}
}
bs = bdrv_all_find_vmstate_bs(errp);
if (bs == NULL) {
-return ret;
+return false;
}
aio_context = bdrv_get_aio_context(bs);
@@ -2702,7 +2702,7 @@ int save_snapshot(const char *name, Error **errp)
ret = global_state_store();
if (ret) {
error_setg(errp, "Error saving global state");
-return ret;
+return false;
}
vm_stop(RUN_STATE_SAVE_VM);
@@ -2779,7 +2779,7 @@ int save_snapshot(const char *name, Error **errp)
if (saved_vm_running) {
vm_start();
}
-return ret;
+return ret == 0;
}
void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
diff --git a/replay/replay-debugging.c b/replay/replay-debugging.c
index 8785489c02..5458a73fce 100644
--- a/replay/replay-debugging.c
+++ b/replay/replay-debugging.c
@@ -327,7 +327,7 @@ void replay_gdb_attached(void)
*/
if (replay_mode == REPLAY_MODE_PLAY
&& !replay_snapshot) {
-if (save_snapshot("start_debugging", NULL) != 0) {
+if (!save_snapshot("start_debugging", NULL)) {
/* Can't create the snapshot. Continue conventional debugging. */
}
}
diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
index e26fa4c892..4f2560d156 100644
--- a/replay/replay-snapshot.c
+++ b/replay/replay-snapshot.c
@@ -77,7 +77,7 @@ void replay_vmstate_init(void)
if (replay_snapshot) {
if (replay_mode == REPLAY_MODE_RECORD) {
-if (save_snapshot(replay_snapshot, ) != 0) {
+if (!save_snapshot(replay_snapshot, )) {
error_report_err(err);
error_report("Could not create snapshot for icount record");
exit(1);
Re: acceptance-system-fedora failures
On 08.10.2020 14:50, Kevin Wolf wrote: Am 08.10.2020 um 12:26 hat Philippe Mathieu-Daudé geschrieben: On 10/7/20 3:11 PM, Pavel Dovgalyuk wrote: On 07.10.2020 15:49, Philippe Mathieu-Daudé wrote: On 10/7/20 2:20 PM, Pavel Dovgalyuk wrote: On 07.10.2020 14:22, Alex Bennée wrote: Philippe Mathieu-Daudé writes: On 10/7/20 10:51 AM, Pavel Dovgalyuk wrote: On 07.10.2020 11:23, Thomas Huth wrote: On 07/10/2020 09.13, Philippe Mathieu-Daudé wrote: Thanks, that was helpful. ... and the winner is:       commit  55adb3c45620c31f29978f209e2a44a08d34e2da       Author: John Snow       Date:   Fri Jul 24 01:23:00 2020 -0400       Subject: ide: cancel pending callbacks on SRST ... starting with this commit, the tests starts failing. John, any idea what might be causing this? This patch includes the following lines: +       aio_bh_schedule_oneshot(qemu_get_aio_context(), +                               ide_bus_perform_srst, bus); replay_bh_schedule_oneshot_event should be used instead of this function, because it synchronizes non-deterministic BHs. Why do we have 2 different functions? BH are already complex enough, and we need to also think about the replay API... What about the other cases such vhost-user (blk/net), virtio-blk? This does seem like something that should be wrapped up inside aio_bh_schedule_oneshot itself or maybe we need a aio_bh_schedule_transaction_oneshot to distinguish it from the other uses the function has. Maybe there should be two functions: - one for the guest modification aio_bh_schedule_oneshot_deterministic()? - one for internal qemu things Not sure why there is a difference, BH are used to avoid delaying the guest, so there always something related to "guest modification". Not exactly. At least there is one non-related-to-guest case in monitor_init_qmp:        /*         * We can't call qemu_chr_fe_set_handlers() directly here         * since chardev might be running in the monitor I/O         * thread. Schedule a bottom half.         */        aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),                                monitor_qmp_setup_handlers_bh, mon); I don't understand the documentation in docs/devel/replay.txt: --- Bottom halves = Bottom half callbacks, that affect the guest state, should be invoked through replay_bh_schedule_event or replay_bh_schedule_oneshot_event functions. Their invocations are saved in record mode and synchronized with the existing log in replay mode. --- But then it is only used in block drivers, which are not related to guest state: Pavel can tell you the details, but I think the idea was that you need to use this function not when the code calling it modifies guest state, but when the BH implementation can do so. In the case of generic callbacks like provided by the blk_aio_*() functions, we don't know whether this is the case, but it's generally device emulation code, so chances are relatively high that they do. I seem to remember that when reviewing the code that introduced replay_bh_schedule_event(), I was relatively sure that we didn't catch all necessary instances, but since it worked for Pavel and I didn't feel like getting too involved with replay code, we just merged it anyway. That's right. Block layer does not touch guest by itself. But it includes callbacks that may invoke interrupts on finishing disk operations. That is why we synchronize these callbacks with vCPU through the replay layer. Pavel Dovgalyuk
Re: [PATCH 3/3] migration: stop returning errno from load_snapshot()
On 08.10.2020 20:48, Philippe Mathieu-Daudé wrote:
From: Daniel P. Berrangé
None of the callers care about the errno value since there is a full
Error object populated. This gives consistency with save_snapshot()
which already just returns a boolean value.
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Daniel P. Berrangé
[PMD: Return false/true instead of -1/0, document function]
Signed-off-by: Philippe Mathieu-Daudé
Acked-by: Pavel Dovgalyuk
---
include/migration/snapshot.h | 9 -
migration/savevm.c | 19 +--
monitor/hmp-cmds.c | 2 +-
replay/replay-snapshot.c | 2 +-
softmmu/vl.c | 2 +-
5 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/include/migration/snapshot.h b/include/migration/snapshot.h
index a40c34307b..9bc989a6b4 100644
--- a/include/migration/snapshot.h
+++ b/include/migration/snapshot.h
@@ -23,6 +23,13 @@
* On failure, store an error through @errp and return %false.
*/
bool save_snapshot(const char *name, Error **errp);
-int load_snapshot(const char *name, Error **errp);
+/**
+ * save_snapshot: Load a snapshot.
+ * @name: path to snapshot
+ * @errp: pointer to error object
+ * On success, return %true.
+ * On failure, store an error through @errp and return %false.
+ */
+bool load_snapshot(const char *name, Error **errp);
#endif
diff --git a/migration/savevm.c b/migration/savevm.c
index fd2e5e8b66..531bb2eca1 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2864,7 +2864,7 @@ void qmp_xen_load_devices_state(const char *filename,
Error **errp)
migration_incoming_state_destroy();
}
-int load_snapshot(const char *name, Error **errp)
+bool load_snapshot(const char *name, Error **errp)
{
BlockDriverState *bs_vm_state;
QEMUSnapshotInfo sn;
@@ -2874,16 +2874,16 @@ int load_snapshot(const char *name, Error **errp)
MigrationIncomingState *mis = migration_incoming_get_current();
if (!bdrv_all_can_snapshot(errp)) {
-return -ENOTSUP;
+return false;
}
ret = bdrv_all_find_snapshot(name, errp);
if (ret < 0) {
-return ret;
+return false;
}
bs_vm_state = bdrv_all_find_vmstate_bs(errp);
if (!bs_vm_state) {
-return -ENOTSUP;
+return false;
}
aio_context = bdrv_get_aio_context(bs_vm_state);
@@ -2892,11 +2892,11 @@ int load_snapshot(const char *name, Error **errp)
ret = bdrv_snapshot_find(bs_vm_state, , name);
aio_context_release(aio_context);
if (ret < 0) {
-return ret;
+return false;
} else if (sn.vm_state_size == 0) {
error_setg(errp, "This is a disk-only snapshot. Revert to it "
" offline using qemu-img");
-return -EINVAL;
+return false;
}
/*
@@ -2917,7 +2917,6 @@ int load_snapshot(const char *name, Error **errp)
f = qemu_fopen_bdrv(bs_vm_state, 0);
if (!f) {
error_setg(errp, "Could not open VM state file");
-ret = -EINVAL;
goto err_drain;
}
@@ -2933,14 +2932,14 @@ int load_snapshot(const char *name, Error **errp)
if (ret < 0) {
error_setg(errp, "Error %d while loading VM state", ret);
-return ret;
+return false;
}
-return 0;
+return true;
err_drain:
bdrv_drain_all_end();
-return ret;
+return false;
}
void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 14848a3381..ff0e3df8a3 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1123,7 +1123,7 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict)
vm_stop(RUN_STATE_RESTORE_VM);
-if (load_snapshot(name, ) == 0 && saved_vm_running) {
+if (!load_snapshot(name, ) && saved_vm_running) {
vm_start();
}
hmp_handle_error(mon, err);
diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
index 4f2560d156..b289365937 100644
--- a/replay/replay-snapshot.c
+++ b/replay/replay-snapshot.c
@@ -83,7 +83,7 @@ void replay_vmstate_init(void)
exit(1);
}
} else if (replay_mode == REPLAY_MODE_PLAY) {
-if (load_snapshot(replay_snapshot, ) != 0) {
+if (!load_snapshot(replay_snapshot, )) {
error_report_err(err);
error_report("Could not load snapshot for icount replay");
exit(1);
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 5a11a62f78..6eaa6b3a09 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -4478,7 +4478,7 @@ void qemu_init(int argc, char **argv, char **envp)
register_global_state();
if (loadvm) {
Error *local_err = NULL;
-if (load_snapshot(loadvm, _err) < 0) {
+if (!load_snapshot(loadvm, _err)) {
error_report_err(local_err);
autostart = 0;
exit(1);
Re: [Qemu-block] [PATCH 4/5] block/blkreplay: Remove protocol-related fields
> From: Fabiano Rosas [mailto:faro...@linux.vnet.ibm.com]
> The blkreplay driver is not a protocol so it should implement bdrv_open
> instead of bdrv_file_open and not provide a protocol_name.
>
> Attempts to invoke this driver using protocol syntax
> (i.e. blkreplay:) will now fail gracefully:
>
> $ qemu-img info blkreplay:foo
> qemu-img: Could not open 'blkreplay:foo': Unknown protocol 'blkreplay'
>
> Signed-off-by: Fabiano Rosas <faro...@linux.vnet.ibm.com>
> ---
> block/blkreplay.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/block/blkreplay.c b/block/blkreplay.c
> index 61e44a1949..fe5a9b4a98 100755
> --- a/block/blkreplay.c
> +++ b/block/blkreplay.c
> @@ -129,10 +129,9 @@ static int coroutine_fn
> blkreplay_co_flush(BlockDriverState *bs)
>
> static BlockDriver bdrv_blkreplay = {
> .format_name= "blkreplay",
> -.protocol_name = "blkreplay",
> .instance_size = 0,
>
> -.bdrv_file_open = blkreplay_open,
> +.bdrv_open = blkreplay_open,
> .bdrv_close = blkreplay_close,
> .bdrv_child_perm= bdrv_filter_default_perms,
> .bdrv_getlength = blkreplay_getlength,
> --
> 2.13.6
Reviewed-off-by: Pavel Dovgalyuk <pavel.dovga...@ispras.ru>
Pavel Dovgalyuk
Re: [Qemu-block] [Qemu-devel] [PATCH v5 3/4] shutdown: Add source information to SHUTDOWN and RESET
> From: Markus Armbruster [mailto:arm...@redhat.com] > > Scandalously, replay/ is not covered by MAINTAINERS. Here it is. http://lists.nongnu.org/archive/html/qemu-devel/2017-05/msg00080.html I'll try to figure out the SHUTDOWN problem. Pavel Dovgalyuk
[Qemu-block] [PATCH v2 2/3] blkreplay: create temporary overlay for underlaying devices
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.
This patch creates similar temporary overlay (when it is needed)
under the blkreplay driver. Therefore all block operations are controlled
by blkreplay.
v2: removed useless ref/unref (as suggested by Kevin Wolf)
Signed-off-by: Pavel Dovgalyuk <pavel.dovga...@ispras.ru>
---
block/blkreplay.c | 65 +
stubs/replay.c|1 +
vl.c |2 +-
3 files changed, 67 insertions(+), 1 deletion(-)
diff --git a/block/blkreplay.c b/block/blkreplay.c
index ddcf344..de91def 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -14,12 +14,69 @@
#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, _abort);
+qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size, _abort);
+ret = bdrv_create(_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) {
+goto out;
+}
+
+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 +92,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, _err)) {
+ret = -EINVAL;
+error_propagate(errp, local_err);
+goto fail;
+}
+
ret = 0;
fail:
if (ret < 0) {
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 e10a27b..876a55a 100644
--- a/vl.c
+++ b/vl.c
@@ -4450,7 +4450,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);
}
[Qemu-block] [PATCH v2 3/3] replay: disable default snapshot for record/replay
This patch disables setting '-snapshot' option on by default
in record/replay mode. This is needed for creating vmstates in record
and replay modes.
Signed-off-by: Pavel Dovgalyuk <pavel.dovga...@ispras.ru>
---
vl.c |8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/vl.c b/vl.c
index 876a55a..1b85923 100644
--- a/vl.c
+++ b/vl.c
@@ -3140,7 +3140,13 @@ int main(int argc, char **argv, char **envp)
drive_add(IF_PFLASH, -1, optarg, PFLASH_OPTS);
break;
case QEMU_OPTION_snapshot:
-snapshot = 1;
+{
+Error *blocker = NULL;
+snapshot = 1;
+error_setg(, QERR_REPLAY_NOT_SUPPORTED,
+ "-snapshot");
+replay_add_blocker(blocker);
+}
break;
case QEMU_OPTION_hdachs:
{
[Qemu-block] [PATCH v2 0/3] block devices record/replay update
This set of patches includes several fixes for preserving the state of the block device images while recording and replaying virtual machine execution. blkreplay driver now creates temporary overlay for underlaying devices This patch implicitly enables '-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 with explicit '-snapshot' option breaks the determinism. v2 changes: - removed useless bdrv_ref/unref calls (as suggested by Kevin Wolf) - updated commit messages - minor changes --- Pavel Dovgalyuk (3): block: implement bdrv_snapshot_goto for blkreplay blkreplay: create temporary overlay for underlaying devices replay: disable default snapshot for record/replay block/blkreplay.c | 73 + stubs/replay.c|1 + vl.c | 10 ++- 3 files changed, 82 insertions(+), 2 deletions(-) -- Pavel Dovgalyuk
[Qemu-block] [PATCH v2 1/3] block: implement bdrv_snapshot_goto for blkreplay
This patch enables making snapshots with blkreplay used in
block devices.
This function is required to make bdrv_snapshot_goto without
calling .bdrv_open which is not implemented.
v2: updated commit message
Signed-off-by: Pavel Dovgalyuk <pavel.dovga...@ispras.ru>
---
block/blkreplay.c |8
1 file changed, 8 insertions(+)
diff --git a/block/blkreplay.c b/block/blkreplay.c
index cfc8c5b..ddcf344 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -130,6 +130,12 @@ static int coroutine_fn
blkreplay_co_flush(BlockDriverState *bs)
return ret;
}
+static int blkreplay_snapshot_goto(BlockDriverState *bs,
+ const char *snapshot_id)
+{
+return bdrv_snapshot_goto(bs->file->bs, snapshot_id);
+}
+
static BlockDriver bdrv_blkreplay = {
.format_name= "blkreplay",
.protocol_name = "blkreplay",
@@ -145,6 +151,8 @@ static BlockDriver bdrv_blkreplay = {
.bdrv_co_pwrite_zeroes = blkreplay_co_pwrite_zeroes,
.bdrv_co_pdiscard = blkreplay_co_pdiscard,
.bdrv_co_flush = blkreplay_co_flush,
+
+.bdrv_snapshot_goto = blkreplay_snapshot_goto,
};
static void bdrv_blkreplay_init(void)
Re: [Qemu-block] [PATCH 1/3] block: implement bdrv_snapshot_goto for blkreplay
> From: Kevin Wolf [mailto:kw...@redhat.com] > Am 31.01.2017 um 12:57 hat Pavel Dovgalyuk geschrieben: > > This patch enables making snapshots with blkreplay used in > > block devices. > > > > Signed-off-by: Pavel Dovgalyuk <pavel.dovga...@ispras.ru> > > Specifically, I think it avoids the blkreplay_open/close sequence. Is > this what is needed to make it work? Then I'll need to implement bdrv_open, because there is only bdrv_file_open for blkreplay now. Which way is better? > We should probably mention in the commit message the exact reason why > implementing .bdrv_snapshot_goto, but not the other snapshot related > callbacks, fixes things. If you confirm my assumption, I can add that > while applying. Pavel Dovgalyuk
Re: [Qemu-block] [PATCH 0/3] block devices record/replay update
Destination host unreachable. Ping again. Pavel Dovgalyuk > -Original Message- > From: Pavel Dovgalyuk [mailto:dovga...@ispras.ru] > Sent: Monday, February 13, 2017 8:05 AM > To: 'Pavel Dovgalyuk'; qemu-de...@nongnu.org > Cc: kw...@redhat.com; pbonz...@redhat.com; qemu-block@nongnu.org; > mre...@redhat.com > Subject: RE: [PATCH 0/3] block devices record/replay update > > Ping? > > Pavel Dovgalyuk > > > -Original Message- > > From: Pavel Dovgalyuk [mailto:pavel.dovga...@ispras.ru] > > Sent: Tuesday, January 31, 2017 2:57 PM > > To: qemu-de...@nongnu.org > > Cc: kw...@redhat.com; pbonz...@redhat.com; dovga...@ispras.ru; > > qemu-block@nongnu.org; > > mre...@redhat.com > > Subject: [PATCH 0/3] block devices record/replay update > > > > This set of patches includes several fixes for preserving > > the state of the block device images while recording and replaying > > virtual machine execution. > > > > blkreplay driver now creates temporary overlay for underlaying devices > > This patch implicitly enables '-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 > > with explicit '-snapshot' option breaks the determinism. > > > > --- > > > > Pavel Dovgalyuk (3): > > block: implement bdrv_snapshot_goto for blkreplay > > blkreplay: create temporary overlay for underlaying devices > > replay: disable default snapshot for record/replay > > > > > > block/blkreplay.c | 84 > > + > > stubs/replay.c|1 + > > vl.c | 10 +- > > 3 files changed, 93 insertions(+), 2 deletions(-) > > > > -- > > Pavel Dovgalyuk
Re: [Qemu-block] [PATCH 0/3] block devices record/replay update
Ping? Pavel Dovgalyuk > -Original Message- > From: Pavel Dovgalyuk [mailto:pavel.dovga...@ispras.ru] > Sent: Tuesday, January 31, 2017 2:57 PM > To: qemu-de...@nongnu.org > Cc: kw...@redhat.com; pbonz...@redhat.com; dovga...@ispras.ru; > qemu-block@nongnu.org; > mre...@redhat.com > Subject: [PATCH 0/3] block devices record/replay update > > This set of patches includes several fixes for preserving > the state of the block device images while recording and replaying > virtual machine execution. > > blkreplay driver now creates temporary overlay for underlaying devices > This patch implicitly enables '-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 > with explicit '-snapshot' option breaks the determinism. > > --- > > Pavel Dovgalyuk (3): > block: implement bdrv_snapshot_goto for blkreplay > blkreplay: create temporary overlay for underlaying devices > replay: disable default snapshot for record/replay > > > block/blkreplay.c | 84 > + > stubs/replay.c|1 + > vl.c | 10 +- > 3 files changed, 93 insertions(+), 2 deletions(-) > > -- > Pavel Dovgalyuk
[Qemu-block] [PATCH 3/3] replay: disable default snapshot for record/replay
This patch disables setting '-snapshot' option on by default
in record/replay mode. This is needed for creating vmstates in record
and replay modes.
Signed-off-by: Pavel Dovgalyuk <pavel.dovga...@ispras.ru>
---
vl.c |8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/vl.c b/vl.c
index c6fc5c9..671fc04 100644
--- a/vl.c
+++ b/vl.c
@@ -3129,7 +3129,13 @@ int main(int argc, char **argv, char **envp)
drive_add(IF_PFLASH, -1, optarg, PFLASH_OPTS);
break;
case QEMU_OPTION_snapshot:
-snapshot = 1;
+{
+Error *blocker = NULL;
+snapshot = 1;
+error_setg(, QERR_REPLAY_NOT_SUPPORTED,
+ "-snapshot");
+replay_add_blocker(blocker);
+}
break;
case QEMU_OPTION_hdachs:
{
[Qemu-block] [PATCH 1/3] block: implement bdrv_snapshot_goto for blkreplay
This patch enables making snapshots with blkreplay used in
block devices.
Signed-off-by: Pavel Dovgalyuk <pavel.dovga...@ispras.ru>
---
block/blkreplay.c |8
1 file changed, 8 insertions(+)
diff --git a/block/blkreplay.c b/block/blkreplay.c
index a741654..8a03d62 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -130,6 +130,12 @@ static int coroutine_fn
blkreplay_co_flush(BlockDriverState *bs)
return ret;
}
+static int blkreplay_snapshot_goto(BlockDriverState *bs,
+ const char *snapshot_id)
+{
+return bdrv_snapshot_goto(bs->file->bs, snapshot_id);
+}
+
static BlockDriver bdrv_blkreplay = {
.format_name= "blkreplay",
.protocol_name = "blkreplay",
@@ -145,6 +151,8 @@ static BlockDriver bdrv_blkreplay = {
.bdrv_co_pwrite_zeroes = blkreplay_co_pwrite_zeroes,
.bdrv_co_pdiscard = blkreplay_co_pdiscard,
.bdrv_co_flush = blkreplay_co_flush,
+
+.bdrv_snapshot_goto = blkreplay_snapshot_goto,
};
static void bdrv_blkreplay_init(void)
[Qemu-block] [PATCH 2/3] blkreplay: create temporary overlay for underlaying devices
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.dovga...@ispras.ru>
---
block/blkreplay.c | 76 +
stubs/replay.c|1 +
vl.c |2 +
3 files changed, 78 insertions(+), 1 deletion(-)
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, _abort);
+qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size, _abort);
+ret = bdrv_create(_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, _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);
}
[Qemu-block] [PATCH 0/3] block devices record/replay update
This set of patches includes several fixes for preserving the state of the block device images while recording and replaying virtual machine execution. blkreplay driver now creates temporary overlay for underlaying devices This patch implicitly enables '-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 with explicit '-snapshot' option breaks the determinism. --- Pavel Dovgalyuk (3): block: implement bdrv_snapshot_goto for blkreplay blkreplay: create temporary overlay for underlaying devices replay: disable default snapshot for record/replay block/blkreplay.c | 84 + stubs/replay.c|1 + vl.c | 10 +- 3 files changed, 93 insertions(+), 2 deletions(-) -- Pavel Dovgalyuk
Re: [Qemu-block] [PATCH v5 7/9] block: don't make snapshots for filters
Kevin, > From: Kevin Wolf [mailto:kw...@redhat.com] > Am 05.12.2016 um 12:49 hat Pavel Dovgalyuk geschrieben: > > > From: Kevin Wolf [mailto:kw...@redhat.com] > > > > It's not a requirement. But to make replay deterministic we have to > > start with the same image every time. As I know, this may be achieved by: > > 1. Restoring original disk image manually > > 2. Using vm snapshot to start execution from > > 3. Using -snapshot option > > 4. Not using disks at all > > > > > Because if it has to be > > > there, the next step could be that blkreplay creates temporary-overlay > > > internally in its .bdrv_open(). > > > > Here is your answer about such an approach :) > > https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg04687.html > > Right, and unfortunately these are still good points. > > Especially the part where you allowed to give the overlay filename > really needs to work the way it does now with the 'image' option. We > might not need to be that strict with temporary overlays, restricting to > qcow2 with default options could be acceptable there - but whatever I > think of to support both cases results in something that isn't really > easier than the manual way that we figured out above. What about the new version? Pavel Dovgalyuk
Re: [Qemu-block] [PATCH v5 7/9] block: don't make snapshots for filters
> From: Kevin Wolf [mailto:kw...@redhat.com] > Am 05.12.2016 um 12:49 hat Pavel Dovgalyuk geschrieben: > > > From: Kevin Wolf [mailto:kw...@redhat.com] > > > Am 05.12.2016 um 08:43 hat Pavel Dovgalyuk geschrieben: > > > Record/replay without this option uses '-snapshot' to preserve > > the state of the disk images. > > > > > Anyway, it seems that doing things manually is the safe way as long as > > > we don't know the final solution, so I think I agree. > > > > > > For a slightly more convenient way, one of the problems to solve seems > > > to be that snapshot=on always affects the top level node and you can't > > > create a temporary snapshot in the middle of the chain. Perhaps we > > > should introduce a 'temporary-overlay' driver or something like that, so > > > that you could specify things like this: > > > > > > -drive if=none,driver=file,filename=test.img,id=orig > > > -drive if=none,driver=temporary-overlay,file=orig,id=snap > > > -drive if=none,driver=blkreplay,image=snap > > > > This seems reasonable for manual way. > > Maybe another, easier to implement option could be something like this: > > -drive > if=none,driver=file,filename=test.img,snapshot=on,overlay.node-name=snap > -drive if=none,driver=blkreplay,image=snap > > It would require that we implement support for overlay.* options like we > already support backing.* options. Allowing to specify options for the > overlay node is probably nice to have anyway. > > However, there could be a few tricky parts there. For example, what > happens if someone uses overlay.backing=something-else? Perhaps > completely disallowing backing and backing.* for overlays would already > solve this. > > > > Which makes me wonder... Is blkreplay usable without the temporary > > > snapshot or is this pretty much a requirement? > > > > It's not a requirement. But to make replay deterministic we have to > > start with the same image every time. As I know, this may be achieved by: > > 1. Restoring original disk image manually > > 2. Using vm snapshot to start execution from > > 3. Using -snapshot option > > 4. Not using disks at all > > > > > Because if it has to be > > > there, the next step could be that blkreplay creates temporary-overlay > > > internally in its .bdrv_open(). > > > > Here is your answer about such an approach :) > > https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg04687.html > > Right, and unfortunately these are still good points. > > Especially the part where you allowed to give the overlay filename > really needs to work the way it does now with the 'image' option. We > might not need to be that strict with temporary overlays, restricting to > qcow2 with default options could be acceptable there - but whatever I > think of to support both cases results in something that isn't really > easier than the manual way that we figured out above. Can we stop on the following? 1. Don't create any overlays automatically when user wants to save/restore VM state 2. In the opposite case create snapshots, but do not use -snapshot option. Snapshots will be created by the blkreplay as in the link specified. Pavel Dovgalyuk
Re: [Qemu-block] [PATCH v5 7/9] block: don't make snapshots for filters
> From: Kevin Wolf [mailto:kw...@redhat.com] > Am 05.12.2016 um 08:43 hat Pavel Dovgalyuk geschrieben: > > Paolo, > > > > > From: Kevin Wolf [mailto:kw...@redhat.com] > > > Am 21.11.2016 um 13:22 hat Pavel Dovgalyuk geschrieben: > > > > > From: Kevin Wolf [mailto:kw...@redhat.com] > > > > > Am 16.11.2016 um 10:49 hat Pavel Dovgalyuk geschrieben: > > > > > > > > > > How does that bypass blkreplay? blk->root is supposed to be the > > > > > blkreply > > > > > node, do you see something different? If it were the qcow2 node, then > > > > > I > > > > > would expect that no requests at all go through the blkreplay layer. > > > > > > > > It seems, that the problem is in -snapshot option. > > > > We have one of the following block layers depending on command line: > > > > tmp_overlay1 -> blkreplay -> tmp_overlay2 -> disk_image > > > > tmp_overlay1 -> blkreplay -> disk_image > > > > > > > > But the correct scheme is intended to be the following: > > > > blkreplay -> tmp_overlay1 -> disk_image > > > > > > > > How can we fix it? > > > > Maybe we should add blkreplay automatically to all block devices and not > > > > to specify it in the command line? > > > > > > I think you found a pretty fundamental design problem with "global" > > > drive options that add a filter node such as -snapshot and replay mode > > > (replay mode isn't one of them today, but your suggestion to make it > > > automatic would turn it into one). > > > > > > At the core of the problem I think we have two questions: > > > > > > 1. Which block nodes should be affected and get a filter node added, and > > >which nodes shouldn't get one? In your case, disl_image is defined > > >with a -drive option, but shouldn't get the snapshot. > > > > > > 2. In which order should filter nodes be added? > > > > > > Both of these questions feel hard. As long as we haven't thought through > > > the concept as such (rather than discussing one-off hacks) and we're not > > > completely certain what the right answer to the questions is, we > > > shouldn't add more automatic filter nodes, because chances are that we > > > get it wrong and would regret it. > > > > > > The obvious answer for a workaround would be: Make everything manual, > > > i.e. don't use -snapshot, but create a qcow2 overlay manually. > > > > What about to switching to manual overlay creation by default? > > We can make rrsnapshot option mandatory. > > Therefore user will have to create snapshot in image or overlay and > > the disk image will not be corrupted. > > > > It is not very convenient, but we could disable rrsnapshot again when > > the solution for -snapshot will be found. > > Hm, what is this rrsnapshot option? git grep can't find it. It was a patch that was not included yet. This option creates/loads vm snapshot in record/replay mode leaving original disk image unchanged. Record/replay without this option uses '-snapshot' to preserve the state of the disk images. > Anyway, it seems that doing things manually is the safe way as long as > we don't know the final solution, so I think I agree. > > For a slightly more convenient way, one of the problems to solve seems > to be that snapshot=on always affects the top level node and you can't > create a temporary snapshot in the middle of the chain. Perhaps we > should introduce a 'temporary-overlay' driver or something like that, so > that you could specify things like this: > > -drive if=none,driver=file,filename=test.img,id=orig > -drive if=none,driver=temporary-overlay,file=orig,id=snap > -drive if=none,driver=blkreplay,image=snap This seems reasonable for manual way. > Which makes me wonder... Is blkreplay usable without the temporary > snapshot or is this pretty much a requirement? It's not a requirement. But to make replay deterministic we have to start with the same image every time. As I know, this may be achieved by: 1. Restoring original disk image manually 2. Using vm snapshot to start execution from 3. Using -snapshot option 4. Not using disks at all > Because if it has to be > there, the next step could be that blkreplay creates temporary-overlay > internally in its .bdrv_open(). Here is your answer about such an approach :) https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg04687.html Pavel Dovgalyuk
Re: [Qemu-block] [PATCH v5 7/9] block: don't make snapshots for filters
Paolo, > From: Kevin Wolf [mailto:kw...@redhat.com] > Am 21.11.2016 um 13:22 hat Pavel Dovgalyuk geschrieben: > > > From: Kevin Wolf [mailto:kw...@redhat.com] > > > Am 16.11.2016 um 10:49 hat Pavel Dovgalyuk geschrieben: > > > > > From: Pavel Dovgalyuk [mailto:dovga...@ispras.ru] > > > > > > > > I've investigated this issue. > > > > This command line works ok: > > > > -drive > > > > driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img- > > > blkreplay > > > > -device ide-hd,drive=img-blkreplay > > > > > > > > And this does not: > > > > -drive > > > > > > > > driver=blkreplay,if=none,image.driver=qcow2,image.file.driver=file,image.file.filename=testdis > > > k.qcow > > > > ,id=img-blkreplay > > > > -device ide-hd,drive=img-blkreplay > > > > > > > > QEMU hangs at some moment of replay. > > > > > > > > I found that some dma requests do not pass through the blkreplay driver > > > > due to the following line in block-backend.c: > > > > return bdrv_co_preadv(blk->root, offset, bytes, qiov, flags); > > > > > > > > This line passes read request directly to qcow driver and blkreplay > > > > cannot > > > > process it to make deterministic. > > > > > > How does that bypass blkreplay? blk->root is supposed to be the blkreply > > > node, do you see something different? If it were the qcow2 node, then I > > > would expect that no requests at all go through the blkreplay layer. > > > > It seems, that the problem is in -snapshot option. > > We have one of the following block layers depending on command line: > > tmp_overlay1 -> blkreplay -> tmp_overlay2 -> disk_image > > tmp_overlay1 -> blkreplay -> disk_image > > > > But the correct scheme is intended to be the following: > > blkreplay -> tmp_overlay1 -> disk_image > > > > How can we fix it? > > Maybe we should add blkreplay automatically to all block devices and not > > to specify it in the command line? > > I think you found a pretty fundamental design problem with "global" > drive options that add a filter node such as -snapshot and replay mode > (replay mode isn't one of them today, but your suggestion to make it > automatic would turn it into one). > > At the core of the problem I think we have two questions: > > 1. Which block nodes should be affected and get a filter node added, and >which nodes shouldn't get one? In your case, disl_image is defined >with a -drive option, but shouldn't get the snapshot. > > 2. In which order should filter nodes be added? > > Both of these questions feel hard. As long as we haven't thought through > the concept as such (rather than discussing one-off hacks) and we're not > completely certain what the right answer to the questions is, we > shouldn't add more automatic filter nodes, because chances are that we > get it wrong and would regret it. > > The obvious answer for a workaround would be: Make everything manual, > i.e. don't use -snapshot, but create a qcow2 overlay manually. What about to switching to manual overlay creation by default? We can make rrsnapshot option mandatory. Therefore user will have to create snapshot in image or overlay and the disk image will not be corrupted. It is not very convenient, but we could disable rrsnapshot again when the solution for -snapshot will be found. > For getting the automatic thing right, please give me some time to think > about the design. I'll also meet Markus (CCed) in person end of next > week and I think this is a design question that would be useful to > discuss with him then. Pavel Dovgalyuk
Re: [Qemu-block] [PATCH v5 7/9] block: don't make snapshots for filters
> From: Kevin Wolf [mailto:kw...@redhat.com] > Am 16.11.2016 um 10:49 hat Pavel Dovgalyuk geschrieben: > > > From: Pavel Dovgalyuk [mailto:dovga...@ispras.ru] > > > > I've investigated this issue. > > This command line works ok: > > -drive > > driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img- > blkreplay > > -device ide-hd,drive=img-blkreplay > > > > And this does not: > > -drive > > > driver=blkreplay,if=none,image.driver=qcow2,image.file.driver=file,image.file.filename=testdis > k.qcow > > ,id=img-blkreplay > > -device ide-hd,drive=img-blkreplay > > > > QEMU hangs at some moment of replay. > > > > I found that some dma requests do not pass through the blkreplay driver > > due to the following line in block-backend.c: > > return bdrv_co_preadv(blk->root, offset, bytes, qiov, flags); > > > > This line passes read request directly to qcow driver and blkreplay cannot > > process it to make deterministic. > > How does that bypass blkreplay? blk->root is supposed to be the blkreply > node, do you see something different? If it were the qcow2 node, then I > would expect that no requests at all go through the blkreplay layer. It seems, that the problem is in -snapshot option. We have one of the following block layers depending on command line: tmp_overlay1 -> blkreplay -> tmp_overlay2 -> disk_image tmp_overlay1 -> blkreplay -> disk_image But the correct scheme is intended to be the following: blkreplay -> tmp_overlay1 -> disk_image How can we fix it? Maybe we should add blkreplay automatically to all block devices and not to specify it in the command line? Pavel Dovgalyuk
Re: [Qemu-block] [PATCH v5 7/9] block: don't make snapshots for filters
> From: Paolo Bonzini [mailto:pbonz...@redhat.com] > > I've investigated this issue. > > This command line works ok: > > -drive > > > > driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img-blkreplay > > -device ide-hd,drive=img-blkreplay > > > > And this does not: > > -drive > > > driver=blkreplay,if=none,image.driver=qcow2,image.file.driver=file,image.file.filename=testdis > k.qcow > > ,id=img-blkreplay > > -device ide-hd,drive=img-blkreplay > > > > QEMU hangs at some moment of replay. > > > > I found that some dma requests do not pass through the blkreplay driver > > due to the following line in block-backend.c: > > return bdrv_co_preadv(blk->root, offset, bytes, qiov, flags); > > > > This line passes read request directly to qcow driver and blkreplay cannot > > process it to make deterministic. > > I don't understand, blk->root should be the blkreplay here. I've got some more logs. I used the disk image which references the backing file. It seems that some weird things happen with both command lines. == For the first command line (blkreplay separated from image): blk_co_preadv(img-blkreplay) -> bdrv_co_preadv(qcow2, temp_overlay1) -> bdrv_co_preadv(blkreplay, temp_overlay) -> bdrv_co_preadv(qcow2, temp_overlay2) -> bdrv_co_preadv(qcow2, image_overlay) -> bdrv_co_preadv(qcow2, image_backing) -> bdrv_co_preadv(file, image_backing) But sometimes it changes to: blk_co_preadv(img-blkreplay) -> bdrv_co_preadv(qcow2, temp_overlay1) -> bdrv_co_preadv(file, temp_overlay1) == For the second command line (blkreplay combined with image): In most cases we have the following call stack: blk_co_preadv(img-blkreplay) -> bdrv_co_preadv(qcow2, temp_overlay) -> bdrv_co_preadv(blkreplay, image_overlay) -> bdrv_co_preadv(qcow2, image_overlay) -> bdrv_co_preadv(qcow2, image_backing) -> bdrv_co_preadv(file, image_backing) But sometimes it changes to: blk_co_preadv(img-blkreplay) -> bdrv_co_preadv(qcow2, temp overlay) -> bdrv_co_preadv(file, temp overlay) It seems, that temporary overlay is created over blkreplay, which it intended to work as a simple filter. Is that correct? Pavel Dovgalyuk
Re: [Qemu-block] [PATCH v5 7/9] block: don't make snapshots for filters
Kevin, > From: Pavel Dovgalyuk [mailto:dovga...@ispras.ru] > > From: Kevin Wolf [mailto:kw...@redhat.com] > > Am 28.09.2016 um 11:32 hat Pavel Dovgalyuk geschrieben: > > > > From: Kevin Wolf [mailto:kw...@redhat.com] > > > > Am 27.09.2016 um 16:06 hat Pavel Dovgalyuk geschrieben: > > > > > > From: Kevin Wolf [mailto:kw...@redhat.com] > > > > > > Am 26.09.2016 um 11:51 hat Pavel Dovgalyuk geschrieben: > > > > > > > > From: Kevin Wolf [mailto:kw...@redhat.com] > > > > > > > > Am 26.09.2016 um 10:08 hat Pavel Dovgalyuk geschrieben: > > > > > > Originally, we only called bdrv_goto_snapshot() for all _top level_ > > > > > > BDSes, and this is still what you normally get. However, if you > > > > > > explicitly create a BDS (e.g. with its own -drive option), it is > > > > > > considered a top level BDS without actually being top level for the > > > > > > guest, and therefore the snapshotting function is called for it. > > > > > > > > > > > > Of course, this is highly inefficient because the goto_snapshot > > > > > > request > > > > > > is passed by the filter driver and then called another time for the > > > > > > lower node, effectively loading the snapshot a second time. > > > > > > Maybe double-saving/loading does the smallest damage then? > > > And we should just document how to use blkreplay effectively? > > > > > > > > > > > > > > > On the other hand if you use a single -drive option to create both > > > > > > the > > > > > > qcow2 BDS and the blkreplay filter, we do need to pass down the > > > > > > goto_snapshot request because it won't be called for the qcow2 layer > > > > > > otherwise. > > > > > > > > > > How this can be specified in command line? > > > > > I believed that separate -drive option is required. > > > > > > > > Something like this: > > > > > > > > -drive driver=blkreplay,image.driver=file,image.filename=test.img > > > > > > > > > > I tried the following command line, but VM does not detect the hard drive > > > and cannot boot. > > > > > > -drive > > > driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img- > > blkreplay > > > -device ide-hd,drive=img-blkreplay > > > > My command line was assuming a raw image. It looks like you're using a > > qcow (hopefully qcow2?) image. If so, then you need to include the qcow2 > > driver: > > > > -drive driver=blkreplay,if=none,image.driver=qcow2,\ > > image.file.driver=file,image.file.filename=testdisk.qcow,id=img-blkreplay > > This doesn't work for some reason. Replay just hangs at some moment. > > Maybe there exists some internal difference between command line with one or > two -drive > options? I've investigated this issue. This command line works ok: -drive driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img-blkreplay -device ide-hd,drive=img-blkreplay And this does not: -drive driver=blkreplay,if=none,image.driver=qcow2,image.file.driver=file,image.file.filename=testdisk.qcow ,id=img-blkreplay -device ide-hd,drive=img-blkreplay QEMU hangs at some moment of replay. I found that some dma requests do not pass through the blkreplay driver due to the following line in block-backend.c: return bdrv_co_preadv(blk->root, offset, bytes, qiov, flags); This line passes read request directly to qcow driver and blkreplay cannot process it to make deterministic. Pavel Dovgalyuk
Re: [Qemu-block] [PATCH v5 7/9] block: don't make snapshots for filters
> From: Kevin Wolf [mailto:kw...@redhat.com] > Am 28.09.2016 um 11:32 hat Pavel Dovgalyuk geschrieben: > > > From: Kevin Wolf [mailto:kw...@redhat.com] > > > Am 27.09.2016 um 16:06 hat Pavel Dovgalyuk geschrieben: > > > > > From: Kevin Wolf [mailto:kw...@redhat.com] > > > > > Am 26.09.2016 um 11:51 hat Pavel Dovgalyuk geschrieben: > > > > > > > From: Kevin Wolf [mailto:kw...@redhat.com] > > > > > > > Am 26.09.2016 um 10:08 hat Pavel Dovgalyuk geschrieben: > > > > > Originally, we only called bdrv_goto_snapshot() for all _top level_ > > > > > BDSes, and this is still what you normally get. However, if you > > > > > explicitly create a BDS (e.g. with its own -drive option), it is > > > > > considered a top level BDS without actually being top level for the > > > > > guest, and therefore the snapshotting function is called for it. > > > > > > > > > > Of course, this is highly inefficient because the goto_snapshot > > > > > request > > > > > is passed by the filter driver and then called another time for the > > > > > lower node, effectively loading the snapshot a second time. > > > > Maybe double-saving/loading does the smallest damage then? > > And we should just document how to use blkreplay effectively? > > > > > > > > > > > > On the other hand if you use a single -drive option to create both the > > > > > qcow2 BDS and the blkreplay filter, we do need to pass down the > > > > > goto_snapshot request because it won't be called for the qcow2 layer > > > > > otherwise. > > > > > > > > How this can be specified in command line? > > > > I believed that separate -drive option is required. > > > > > > Something like this: > > > > > > -drive driver=blkreplay,image.driver=file,image.filename=test.img > > > > > > > I tried the following command line, but VM does not detect the hard drive > > and cannot boot. > > > > -drive > > driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img- > blkreplay > > -device ide-hd,drive=img-blkreplay > > My command line was assuming a raw image. It looks like you're using a > qcow (hopefully qcow2?) image. If so, then you need to include the qcow2 > driver: > > -drive driver=blkreplay,if=none,image.driver=qcow2,\ > image.file.driver=file,image.file.filename=testdisk.qcow,id=img-blkreplay This doesn't work for some reason. Replay just hangs at some moment. Maybe there exists some internal difference between command line with one or two -drive options? Pavel Dovgalyuk
Re: [Qemu-block] [PATCH v5 7/9] block: don't make snapshots for filters
> From: Kevin Wolf [mailto:kw...@redhat.com] > Am 27.09.2016 um 16:06 hat Pavel Dovgalyuk geschrieben: > > > From: Kevin Wolf [mailto:kw...@redhat.com] > > > Am 26.09.2016 um 11:51 hat Pavel Dovgalyuk geschrieben: > > > > > From: Kevin Wolf [mailto:kw...@redhat.com] > > > > > Am 26.09.2016 um 10:08 hat Pavel Dovgalyuk geschrieben: > > > Originally, we only called bdrv_goto_snapshot() for all _top level_ > > > BDSes, and this is still what you normally get. However, if you > > > explicitly create a BDS (e.g. with its own -drive option), it is > > > considered a top level BDS without actually being top level for the > > > guest, and therefore the snapshotting function is called for it. > > > > > > Of course, this is highly inefficient because the goto_snapshot request > > > is passed by the filter driver and then called another time for the > > > lower node, effectively loading the snapshot a second time. Maybe double-saving/loading does the smallest damage then? And we should just document how to use blkreplay effectively? > > > > > > On the other hand if you use a single -drive option to create both the > > > qcow2 BDS and the blkreplay filter, we do need to pass down the > > > goto_snapshot request because it won't be called for the qcow2 layer > > > otherwise. > > > > How this can be specified in command line? > > I believed that separate -drive option is required. > > Something like this: > > -drive driver=blkreplay,image.driver=file,image.filename=test.img > I tried the following command line, but VM does not detect the hard drive and cannot boot. -drive driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img-blkreplay -device ide-hd,drive=img-blkreplay Pavel Dovgalyuk
Re: [Qemu-block] [PATCH v5 7/9] block: don't make snapshots for filters
> From: Kevin Wolf [mailto:kw...@redhat.com]
> Am 26.09.2016 um 11:51 hat Pavel Dovgalyuk geschrieben:
> > > From: Kevin Wolf [mailto:kw...@redhat.com]
> > > Am 26.09.2016 um 10:08 hat Pavel Dovgalyuk geschrieben:
> > > > This patch disables snapshotting for block driver filters.
> > > > It is needed, because snapshots should be created
> > > > in underlying disk images, not in filters itself.
> > > >
> > > > Signed-off-by: Pavel Dovgalyuk <pavel.dovga...@ispras.ru>
> > >
> > > But that's exactly what the existing code implements? If a driver
> > > doesn't provide .bdrv_snapshot_goto, the request is redirected to
> > > bs->file.
> > >
> > > > block/snapshot.c |3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/block/snapshot.c b/block/snapshot.c
> > > > index bf5c2ca..8998b8b 100644
> > > > --- a/block/snapshot.c
> > > > +++ b/block/snapshot.c
> > > > @@ -184,6 +184,9 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> > > > if (!drv) {
> > > > return -ENOMEDIUM;
> > > > }
> > > > +if (drv->is_filter) {
> > > > +return 0;
> > > > +}
> > >
> > > This, on the other hand, doesn't redirect the request, but silently
> > > ignores it. That is, loading the snapshot will apparently succeed, but
> > > it wouldn't actually load anything and the disk would stay in its
> > > current state.
> >
> > In my use case bdrv_all_goto_snapshot iterates all block drivers, including
> > filters and disk images. Therefore skipping goto for images is ok.
>
> Hm, this can happy today indeed.
>
> Originally, we only called bdrv_goto_snapshot() for all _top level_
> BDSes, and this is still what you normally get. However, if you
> explicitly create a BDS (e.g. with its own -drive option), it is
> considered a top level BDS without actually being top level for the
> guest, and therefore the snapshotting function is called for it.
>
> Of course, this is highly inefficient because the goto_snapshot request
> is passed by the filter driver and then called another time for the
> lower node, effectively loading the snapshot a second time.
>
> On the other hand if you use a single -drive option to create both the
> qcow2 BDS and the blkreplay filter, we do need to pass down the
> goto_snapshot request because it won't be called for the qcow2 layer
> otherwise.
How this can be specified in command line?
I believed that separate -drive option is required.
>
> I'm not completely sure yet what the right behaviour would be here.
Pavel Dovgalyuk
Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/8] block: set snapshot option for block devices in blkreplay module
> From: Kevin Wolf [mailto:kw...@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.dovga...@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,
> > +_abort);
> > +qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size,
> > _abort);
> > +ret = bdrv_create(_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;
