[Qemu-devel] [PATCH 0/3] block devices record/replay update

[Qemu-devel] [PATCH 0/3] block devices record/replay update

[Pavel Dovgalyuk]

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-devel] [PATCH 1/3] block: implement bdrv_snapshot_goto for blkreplay

[Pavel Dovgalyuk]

This patch enables making snapshots with blkreplay used in
block devices.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@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-devel] [PATCH 2/3] blkreplay: create temporary overlay for underlaying devices

[Pavel Dovgalyuk]

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(-)

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);
     }

[Qemu-devel] [PATCH 3/3] replay: disable default snapshot for record/replay

[Pavel Dovgalyuk]

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.dovgaluk@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(&blocker, QERR_REPLAY_NOT_SUPPORTED,
+                               "-snapshot");
+                    replay_add_blocker(blocker);
+                }
                 break;
             case QEMU_OPTION_hdachs:
                 {

Re: [Qemu-devel] [PATCH 0/3] block devices record/replay update

[Pavel Dovgalyuk]

Ping?

Pavel Dovgalyuk

> -----Original Message-----
> From: Pavel Dovgalyuk [mailto:pavel.dovgaluk@ispras.ru]
> Sent: Tuesday, January 31, 2017 2:57 PM
> To: qemu-devel@nongnu.org
> Cc: kwolf@redhat.com; pbonzini@redhat.com; dovgaluk@ispras.ru; qemu-block@nongnu.org;
> mreitz@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-devel] [PATCH 0/3] block devices record/replay update

[Pavel Dovgalyuk]

Destination host unreachable.

Ping again.

Pavel Dovgalyuk

> -----Original Message-----
> From: Pavel Dovgalyuk [mailto:dovgaluk@ispras.ru]
> Sent: Monday, February 13, 2017 8:05 AM
> To: 'Pavel Dovgalyuk'; qemu-devel@nongnu.org
> Cc: kwolf@redhat.com; pbonzini@redhat.com; qemu-block@nongnu.org; mreitz@redhat.com
> Subject: RE: [PATCH 0/3] block devices record/replay update
> 
> Ping?
> 
> Pavel Dovgalyuk
> 
> > -----Original Message-----
> > From: Pavel Dovgalyuk [mailto:pavel.dovgaluk@ispras.ru]
> > Sent: Tuesday, January 31, 2017 2:57 PM
> > To: qemu-devel@nongnu.org
> > Cc: kwolf@redhat.com; pbonzini@redhat.com; dovgaluk@ispras.ru; qemu-block@nongnu.org;
> > mreitz@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-devel] [PATCH 1/3] block: implement bdrv_snapshot_goto for blkreplay

[Kevin Wolf]

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.dovgaluk@ispras.ru>

Specifically, I think it avoids the blkreplay_open/close sequence. Is
this what is needed to make it work?

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.

Kevin

Re: [Qemu-devel] [PATCH 2/3] blkreplay: create temporary overlay for underlaying devices

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 3/3] replay: disable default snapshot for record/replay

[Kevin Wolf]

Am 31.01.2017 um 12:57 hat Pavel Dovgalyuk geschrieben:
> 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.dovgaluk@ispras.ru>

Looks good the me. (Though obviously, record/replay mode really comes
with a lot more restrictions for block devices that aren't checked yet.
But it's a start.)

Kevin

Re: [Qemu-devel] [PATCH 1/3] block: implement bdrv_snapshot_goto for blkreplay

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kwolf@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.dovgaluk@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-devel] [PATCH 1/3] block: implement bdrv_snapshot_goto for blkreplay

[Kevin Wolf]

Am 27.02.2017 um 13:07 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@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.dovgaluk@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?

I was just checking whether I understood the reason for this correctly.

If I did, then I think your solution is fine and we should just make the
commit message a bit more explicit.

Kevin

> > 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
>