[Qemu-devel] [for-4.2 PATCH 0/6] Block-related record/replay fixes

[Qemu-devel] [for-4.2 PATCH 0/6] Block-related record/replay fixes

[Pavel Dovgalyuk]

The set of patches include the block-related updates
of the record/replay icount feature:
 - application of 'snapshot' option on the file layer instead of
   the top one: command line and documentation fix
 - implementation of bdrv_snapshot_goto for blkreplay driver
 - start/stop fix in replay mode with attached block devices
 - record/replay of bh oneshot events, used in the block layer

---

Pavel Dovgaluk (6):
      block: implement bdrv_snapshot_goto for blkreplay
      replay: disable default snapshot for record/replay
      replay: update docs for record/replay with block devices
      replay: don't drain/flush bdrv queue while RR is working
      replay: finish record/replay before closing the disks
      replay: add BH oneshot event for block layer


 block/blkreplay.c        |    8 ++++++++
 block/block-backend.c    |    9 ++++++---
 block/io.c               |   32 ++++++++++++++++++++++++++++++--
 block/iscsi.c            |    5 +++--
 block/nfs.c              |    6 ++++--
 block/null.c             |    4 +++-
 block/nvme.c             |    6 ++++--
 block/rbd.c              |    5 +++--
 block/vxhs.c             |    5 +++--
 cpus.c                   |    2 --
 docs/replay.txt          |   12 +++++++++---
 include/sysemu/replay.h  |    4 ++++
 replay/replay-events.c   |   16 ++++++++++++++++
 replay/replay-internal.h |    1 +
 replay/replay.c          |    2 ++
 stubs/Makefile.objs      |    1 +
 stubs/replay-user.c      |    9 +++++++++
 vl.c                     |   11 +++++++++--
 18 files changed, 115 insertions(+), 23 deletions(-)
 create mode 100644 stubs/replay-user.c

-- 
Pavel Dovgalyuk

[Qemu-devel] [for-4.2 PATCH 1/6] block: implement bdrv_snapshot_goto for blkreplay

[Pavel Dovgalyuk]

From: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>

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.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
Acked-by: Kevin Wolf <kwolf@redhat.com>
---
 block/blkreplay.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/blkreplay.c b/block/blkreplay.c
index 2b7931b940..c96ac8f4bc 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -126,6 +126,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, NULL);
+}
+
 static BlockDriver bdrv_blkreplay = {
     .format_name            = "blkreplay",
     .instance_size          = 0,
@@ -140,6 +146,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] [for-4.2 PATCH 2/6] replay: disable default snapshot for record/replay

[Pavel Dovgalyuk]

From: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>

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>
Acked-by: Kevin Wolf <kwolf@redhat.com>
---
 vl.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index 630f5c5e9c..0241446df9 100644
--- a/vl.c
+++ b/vl.c
@@ -1203,7 +1203,7 @@ static void configure_blockdev(BlockdevOptionsQueue *bdo_queue,
         qapi_free_BlockdevOptions(bdo->bdo);
         g_free(bdo);
     }
-    if (snapshot || replay_mode != REPLAY_MODE_NONE) {
+    if (snapshot) {
         qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
                           NULL, NULL);
     }
@@ -3066,7 +3066,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_numa:
                 opts = qemu_opts_parse_noisily(qemu_find_opts("numa"),

[Qemu-devel] [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Pavel Dovgalyuk]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

This patch updates the description of the command lines for using
record/replay with attached block devices.

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
---
 docs/replay.txt |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/docs/replay.txt b/docs/replay.txt
index ee6aee9861..ce97c3f72f 100644
--- a/docs/replay.txt
+++ b/docs/replay.txt
@@ -27,7 +27,7 @@ Usage of the record/replay:
  * First, record the execution with the following command line:
     qemu-system-i386 \
      -icount shift=7,rr=record,rrfile=replay.bin \
-     -drive file=disk.qcow2,if=none,id=img-direct \
+     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
      -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
      -device ide-hd,drive=img-blkreplay \
      -netdev user,id=net1 -device rtl8139,netdev=net1 \
@@ -35,7 +35,7 @@ Usage of the record/replay:
  * After recording, you can replay it by using another command line:
     qemu-system-i386 \
      -icount shift=7,rr=replay,rrfile=replay.bin \
-     -drive file=disk.qcow2,if=none,id=img-direct \
+     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
      -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
      -device ide-hd,drive=img-blkreplay \
      -netdev user,id=net1 -device rtl8139,netdev=net1 \
@@ -223,7 +223,7 @@ Block devices record/replay module intercepts calls of
 bdrv coroutine functions at the top of block drivers stack.
 To record and replay block operations the drive must be configured
 as following:
- -drive file=disk.qcow2,if=none,id=img-direct
+ -drive file=disk.qcow2,if=none,snapshot,id=img-direct
  -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
  -device ide-hd,drive=img-blkreplay
 
@@ -252,6 +252,12 @@ This snapshot is created at start of recording and restored at start
 of replaying. It also can be loaded while replaying to roll back
 the execution.
 
+'snapshot' flag of the disk image must be removed to save the snapshots
+in the overlay (or original image) instead of using the temporary overlay.
+ -drive file=disk.ovl,if=none,id=img-direct
+ -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
+ -device ide-hd,drive=img-blkreplay
+
 Use QEMU monitor to create additional snapshots. 'savevm <name>' command
 created the snapshot and 'loadvm <name>' restores it. To prevent corruption
 of the original disk image, use overlay files linked to the original images.

Re: [Qemu-devel] [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Kevin Wolf]

Am 17.09.2019 um 13:58 hat Pavel Dovgalyuk geschrieben:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> 
> This patch updates the description of the command lines for using
> record/replay with attached block devices.
> 
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> ---
>  docs/replay.txt |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/replay.txt b/docs/replay.txt
> index ee6aee9861..ce97c3f72f 100644
> --- a/docs/replay.txt
> +++ b/docs/replay.txt
> @@ -27,7 +27,7 @@ Usage of the record/replay:
>   * First, record the execution with the following command line:
>      qemu-system-i386 \
>       -icount shift=7,rr=record,rrfile=replay.bin \
> -     -drive file=disk.qcow2,if=none,id=img-direct \
> +     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
>       -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
>       -device ide-hd,drive=img-blkreplay \
>       -netdev user,id=net1 -device rtl8139,netdev=net1 \
> @@ -35,7 +35,7 @@ Usage of the record/replay:
>   * After recording, you can replay it by using another command line:
>      qemu-system-i386 \
>       -icount shift=7,rr=replay,rrfile=replay.bin \
> -     -drive file=disk.qcow2,if=none,id=img-direct \
> +     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
>       -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
>       -device ide-hd,drive=img-blkreplay \
>       -netdev user,id=net1 -device rtl8139,netdev=net1 \
> @@ -223,7 +223,7 @@ Block devices record/replay module intercepts calls of
>  bdrv coroutine functions at the top of block drivers stack.
>  To record and replay block operations the drive must be configured
>  as following:
> - -drive file=disk.qcow2,if=none,id=img-direct
> + -drive file=disk.qcow2,if=none,snapshot,id=img-direct
>   -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
>   -device ide-hd,drive=img-blkreplay
>  
> @@ -252,6 +252,12 @@ This snapshot is created at start of recording and restored at start
>  of replaying. It also can be loaded while replaying to roll back
>  the execution.
>  
> +'snapshot' flag of the disk image must be removed to save the snapshots
> +in the overlay (or original image) instead of using the temporary overlay.
> + -drive file=disk.ovl,if=none,id=img-direct
> + -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
> + -device ide-hd,drive=img-blkreplay

Wait, didn't patch 2 just make -snapshot unconditionally incompatible
with replay? So isn't saving the snapshot in the original image the only
supported mode now?

Kevin

Re: [Qemu-devel] [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Am 17.09.2019 um 13:58 hat Pavel Dovgalyuk geschrieben:
> > From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> >
> > This patch updates the description of the command lines for using
> > record/replay with attached block devices.
> >
> > Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> > ---
> >  docs/replay.txt |   12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/docs/replay.txt b/docs/replay.txt
> > index ee6aee9861..ce97c3f72f 100644
> > --- a/docs/replay.txt
> > +++ b/docs/replay.txt
> > @@ -27,7 +27,7 @@ Usage of the record/replay:
> >   * First, record the execution with the following command line:
> >      qemu-system-i386 \
> >       -icount shift=7,rr=record,rrfile=replay.bin \
> > -     -drive file=disk.qcow2,if=none,id=img-direct \
> > +     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
> >       -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
> >       -device ide-hd,drive=img-blkreplay \
> >       -netdev user,id=net1 -device rtl8139,netdev=net1 \
> > @@ -35,7 +35,7 @@ Usage of the record/replay:
> >   * After recording, you can replay it by using another command line:
> >      qemu-system-i386 \
> >       -icount shift=7,rr=replay,rrfile=replay.bin \
> > -     -drive file=disk.qcow2,if=none,id=img-direct \
> > +     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
> >       -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
> >       -device ide-hd,drive=img-blkreplay \
> >       -netdev user,id=net1 -device rtl8139,netdev=net1 \
> > @@ -223,7 +223,7 @@ Block devices record/replay module intercepts calls of
> >  bdrv coroutine functions at the top of block drivers stack.
> >  To record and replay block operations the drive must be configured
> >  as following:
> > - -drive file=disk.qcow2,if=none,id=img-direct
> > + -drive file=disk.qcow2,if=none,snapshot,id=img-direct
> >   -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
> >   -device ide-hd,drive=img-blkreplay
> >
> > @@ -252,6 +252,12 @@ This snapshot is created at start of recording and restored at start
> >  of replaying. It also can be loaded while replaying to roll back
> >  the execution.
> >
> > +'snapshot' flag of the disk image must be removed to save the snapshots
> > +in the overlay (or original image) instead of using the temporary overlay.
> > + -drive file=disk.ovl,if=none,id=img-direct
> > + -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
> > + -device ide-hd,drive=img-blkreplay
> 
> Wait, didn't patch 2 just make -snapshot unconditionally incompatible
> with replay? So isn't saving the snapshot in the original image the only
> supported mode now?

There are two ways to run record/replay:
1. Disk with snapshot option and any image to make it unchanged
2. Disk with overlay without snapshot option to save VM snapshots on it

Pavel Dovgalyuk

Re: [Qemu-devel] [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Kevin Wolf]

Am 18.09.2019 um 11:22 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Am 17.09.2019 um 13:58 hat Pavel Dovgalyuk geschrieben:
> > > From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> > >
> > > This patch updates the description of the command lines for using
> > > record/replay with attached block devices.
> > >
> > > Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> > > ---
> > >  docs/replay.txt |   12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/docs/replay.txt b/docs/replay.txt
> > > index ee6aee9861..ce97c3f72f 100644
> > > --- a/docs/replay.txt
> > > +++ b/docs/replay.txt
> > > @@ -27,7 +27,7 @@ Usage of the record/replay:
> > >   * First, record the execution with the following command line:
> > >      qemu-system-i386 \
> > >       -icount shift=7,rr=record,rrfile=replay.bin \
> > > -     -drive file=disk.qcow2,if=none,id=img-direct \
> > > +     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
> > >       -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
> > >       -device ide-hd,drive=img-blkreplay \
> > >       -netdev user,id=net1 -device rtl8139,netdev=net1 \
> > > @@ -35,7 +35,7 @@ Usage of the record/replay:
> > >   * After recording, you can replay it by using another command line:
> > >      qemu-system-i386 \
> > >       -icount shift=7,rr=replay,rrfile=replay.bin \
> > > -     -drive file=disk.qcow2,if=none,id=img-direct \
> > > +     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
> > >       -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
> > >       -device ide-hd,drive=img-blkreplay \
> > >       -netdev user,id=net1 -device rtl8139,netdev=net1 \
> > > @@ -223,7 +223,7 @@ Block devices record/replay module intercepts calls of
> > >  bdrv coroutine functions at the top of block drivers stack.
> > >  To record and replay block operations the drive must be configured
> > >  as following:
> > > - -drive file=disk.qcow2,if=none,id=img-direct
> > > + -drive file=disk.qcow2,if=none,snapshot,id=img-direct
> > >   -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
> > >   -device ide-hd,drive=img-blkreplay
> > >
> > > @@ -252,6 +252,12 @@ This snapshot is created at start of recording and restored at start
> > >  of replaying. It also can be loaded while replaying to roll back
> > >  the execution.
> > >
> > > +'snapshot' flag of the disk image must be removed to save the snapshots
> > > +in the overlay (or original image) instead of using the temporary overlay.
> > > + -drive file=disk.ovl,if=none,id=img-direct
> > > + -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
> > > + -device ide-hd,drive=img-blkreplay
> > 
> > Wait, didn't patch 2 just make -snapshot unconditionally incompatible
> > with replay? So isn't saving the snapshot in the original image the only
> > supported mode now?
> 
> There are two ways to run record/replay:
> 1. Disk with snapshot option and any image to make it unchanged
> 2. Disk with overlay without snapshot option to save VM snapshots on it

Yes, I think I understand the two options that you intend to make
available, but when -snapshot sets a replay blocker, how can 1. still
work? Is there some code that ignores replay blockers?

Kevin

Re: [Qemu-devel] [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Am 18.09.2019 um 11:22 hat Pavel Dovgalyuk geschrieben:
> > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > Am 17.09.2019 um 13:58 hat Pavel Dovgalyuk geschrieben:
> > > > From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> > > >
> > > > This patch updates the description of the command lines for using
> > > > record/replay with attached block devices.
> > > >
> > > > Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> > > > ---
> > > >  docs/replay.txt |   12 +++++++++---
> > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/docs/replay.txt b/docs/replay.txt
> > > > index ee6aee9861..ce97c3f72f 100644
> > > > --- a/docs/replay.txt
> > > > +++ b/docs/replay.txt
> > > > @@ -27,7 +27,7 @@ Usage of the record/replay:
> > > >   * First, record the execution with the following command line:
> > > >      qemu-system-i386 \
> > > >       -icount shift=7,rr=record,rrfile=replay.bin \
> > > > -     -drive file=disk.qcow2,if=none,id=img-direct \
> > > > +     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
> > > >       -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
> > > >       -device ide-hd,drive=img-blkreplay \
> > > >       -netdev user,id=net1 -device rtl8139,netdev=net1 \
> > > > @@ -35,7 +35,7 @@ Usage of the record/replay:
> > > >   * After recording, you can replay it by using another command line:
> > > >      qemu-system-i386 \
> > > >       -icount shift=7,rr=replay,rrfile=replay.bin \
> > > > -     -drive file=disk.qcow2,if=none,id=img-direct \
> > > > +     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
> > > >       -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
> > > >       -device ide-hd,drive=img-blkreplay \
> > > >       -netdev user,id=net1 -device rtl8139,netdev=net1 \
> > > > @@ -223,7 +223,7 @@ Block devices record/replay module intercepts calls of
> > > >  bdrv coroutine functions at the top of block drivers stack.
> > > >  To record and replay block operations the drive must be configured
> > > >  as following:
> > > > - -drive file=disk.qcow2,if=none,id=img-direct
> > > > + -drive file=disk.qcow2,if=none,snapshot,id=img-direct
> > > >   -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
> > > >   -device ide-hd,drive=img-blkreplay
> > > >
> > > > @@ -252,6 +252,12 @@ This snapshot is created at start of recording and restored at
> start
> > > >  of replaying. It also can be loaded while replaying to roll back
> > > >  the execution.
> > > >
> > > > +'snapshot' flag of the disk image must be removed to save the snapshots
> > > > +in the overlay (or original image) instead of using the temporary overlay.
> > > > + -drive file=disk.ovl,if=none,id=img-direct
> > > > + -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
> > > > + -device ide-hd,drive=img-blkreplay
> > >
> > > Wait, didn't patch 2 just make -snapshot unconditionally incompatible
> > > with replay? So isn't saving the snapshot in the original image the only
> > > supported mode now?
> >
> > There are two ways to run record/replay:
> > 1. Disk with snapshot option and any image to make it unchanged
> > 2. Disk with overlay without snapshot option to save VM snapshots on it
> 
> Yes, I think I understand the two options that you intend to make
> available, but when -snapshot sets a replay blocker, how can 1. still
> work? Is there some code that ignores replay blockers?

I checked the text and don't see anything about global "-snapshot" option.
All references are about drive snapshot flag.
Can you point me where did I missed that?

Pavel Dovgalyuk

Re: [Qemu-devel] [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Kevin Wolf]

Am 18.09.2019 um 11:37 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Am 18.09.2019 um 11:22 hat Pavel Dovgalyuk geschrieben:
> > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > Am 17.09.2019 um 13:58 hat Pavel Dovgalyuk geschrieben:
> > > > > From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> > > > >
> > > > > This patch updates the description of the command lines for using
> > > > > record/replay with attached block devices.
> > > > >
> > > > > Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> > > > > ---
> > > > >  docs/replay.txt |   12 +++++++++---
> > > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/docs/replay.txt b/docs/replay.txt
> > > > > index ee6aee9861..ce97c3f72f 100644
> > > > > --- a/docs/replay.txt
> > > > > +++ b/docs/replay.txt
> > > > > @@ -27,7 +27,7 @@ Usage of the record/replay:
> > > > >   * First, record the execution with the following command line:
> > > > >      qemu-system-i386 \
> > > > >       -icount shift=7,rr=record,rrfile=replay.bin \
> > > > > -     -drive file=disk.qcow2,if=none,id=img-direct \
> > > > > +     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
> > > > >       -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
> > > > >       -device ide-hd,drive=img-blkreplay \
> > > > >       -netdev user,id=net1 -device rtl8139,netdev=net1 \
> > > > > @@ -35,7 +35,7 @@ Usage of the record/replay:
> > > > >   * After recording, you can replay it by using another command line:
> > > > >      qemu-system-i386 \
> > > > >       -icount shift=7,rr=replay,rrfile=replay.bin \
> > > > > -     -drive file=disk.qcow2,if=none,id=img-direct \
> > > > > +     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
> > > > >       -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
> > > > >       -device ide-hd,drive=img-blkreplay \
> > > > >       -netdev user,id=net1 -device rtl8139,netdev=net1 \
> > > > > @@ -223,7 +223,7 @@ Block devices record/replay module intercepts calls of
> > > > >  bdrv coroutine functions at the top of block drivers stack.
> > > > >  To record and replay block operations the drive must be configured
> > > > >  as following:
> > > > > - -drive file=disk.qcow2,if=none,id=img-direct
> > > > > + -drive file=disk.qcow2,if=none,snapshot,id=img-direct
> > > > >   -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
> > > > >   -device ide-hd,drive=img-blkreplay
> > > > >
> > > > > @@ -252,6 +252,12 @@ This snapshot is created at start of recording and restored at
> > start
> > > > >  of replaying. It also can be loaded while replaying to roll back
> > > > >  the execution.
> > > > >
> > > > > +'snapshot' flag of the disk image must be removed to save the snapshots
> > > > > +in the overlay (or original image) instead of using the temporary overlay.
> > > > > + -drive file=disk.ovl,if=none,id=img-direct
> > > > > + -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
> > > > > + -device ide-hd,drive=img-blkreplay
> > > >
> > > > Wait, didn't patch 2 just make -snapshot unconditionally incompatible
> > > > with replay? So isn't saving the snapshot in the original image the only
> > > > supported mode now?
> > >
> > > There are two ways to run record/replay:
> > > 1. Disk with snapshot option and any image to make it unchanged
> > > 2. Disk with overlay without snapshot option to save VM snapshots on it
> > 
> > Yes, I think I understand the two options that you intend to make
> > available, but when -snapshot sets a replay blocker, how can 1. still
> > work? Is there some code that ignores replay blockers?
> 
> I checked the text and don't see anything about global "-snapshot" option.
> All references are about drive snapshot flag.
> Can you point me where did I missed that?

Oh, sorry, you're right and I misread.

However, global -snapshot is just a convenient shortcut for specifying
snapshot=on for all -drive arguments. So if -snapshot is incompatible
with replay, shouldn't manually marking all drives as snapshot=on be
incompatible as well?

Maybe you're really interested in some specific drive not having
snapshot=on? But then it might be better to check that specific drive
instad of forbidding just the shortcut for setting it.

Kevin

Re: [Qemu-devel] [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Am 18.09.2019 um 11:37 hat Pavel Dovgalyuk geschrieben:
> > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > Am 18.09.2019 um 11:22 hat Pavel Dovgalyuk geschrieben:
> > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > Am 17.09.2019 um 13:58 hat Pavel Dovgalyuk geschrieben:
> > > > > > From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> > > > > >
> > > > > > This patch updates the description of the command lines for using
> > > > > > record/replay with attached block devices.
> > > > > >
> > > > > > Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> > > > > > ---
> > > > > >  docs/replay.txt |   12 +++++++++---
> > > > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/docs/replay.txt b/docs/replay.txt
> > > > > > index ee6aee9861..ce97c3f72f 100644
> > > > > > --- a/docs/replay.txt
> > > > > > +++ b/docs/replay.txt
> > > > > > @@ -27,7 +27,7 @@ Usage of the record/replay:
> > > > > >   * First, record the execution with the following command line:
> > > > > >      qemu-system-i386 \
> > > > > >       -icount shift=7,rr=record,rrfile=replay.bin \
> > > > > > -     -drive file=disk.qcow2,if=none,id=img-direct \
> > > > > > +     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
> > > > > >       -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
> > > > > >       -device ide-hd,drive=img-blkreplay \
> > > > > >       -netdev user,id=net1 -device rtl8139,netdev=net1 \
> > > > > > @@ -35,7 +35,7 @@ Usage of the record/replay:
> > > > > >   * After recording, you can replay it by using another command line:
> > > > > >      qemu-system-i386 \
> > > > > >       -icount shift=7,rr=replay,rrfile=replay.bin \
> > > > > > -     -drive file=disk.qcow2,if=none,id=img-direct \
> > > > > > +     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
> > > > > >       -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
> > > > > >       -device ide-hd,drive=img-blkreplay \
> > > > > >       -netdev user,id=net1 -device rtl8139,netdev=net1 \
> > > > > > @@ -223,7 +223,7 @@ Block devices record/replay module intercepts calls of
> > > > > >  bdrv coroutine functions at the top of block drivers stack.
> > > > > >  To record and replay block operations the drive must be configured
> > > > > >  as following:
> > > > > > - -drive file=disk.qcow2,if=none,id=img-direct
> > > > > > + -drive file=disk.qcow2,if=none,snapshot,id=img-direct
> > > > > >   -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
> > > > > >   -device ide-hd,drive=img-blkreplay
> > > > > >
> > > > > > @@ -252,6 +252,12 @@ This snapshot is created at start of recording and restored at
> > > start
> > > > > >  of replaying. It also can be loaded while replaying to roll back
> > > > > >  the execution.
> > > > > >
> > > > > > +'snapshot' flag of the disk image must be removed to save the snapshots
> > > > > > +in the overlay (or original image) instead of using the temporary overlay.
> > > > > > + -drive file=disk.ovl,if=none,id=img-direct
> > > > > > + -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
> > > > > > + -device ide-hd,drive=img-blkreplay
> > > > >
> > > > > Wait, didn't patch 2 just make -snapshot unconditionally incompatible
> > > > > with replay? So isn't saving the snapshot in the original image the only
> > > > > supported mode now?
> > > >
> > > > There are two ways to run record/replay:
> > > > 1. Disk with snapshot option and any image to make it unchanged
> > > > 2. Disk with overlay without snapshot option to save VM snapshots on it
> > >
> > > Yes, I think I understand the two options that you intend to make
> > > available, but when -snapshot sets a replay blocker, how can 1. still
> > > work? Is there some code that ignores replay blockers?
> >
> > I checked the text and don't see anything about global "-snapshot" option.
> > All references are about drive snapshot flag.
> > Can you point me where did I missed that?
> 
> Oh, sorry, you're right and I misread.
> 
> However, global -snapshot is just a convenient shortcut for specifying
> snapshot=on for all -drive arguments. So if -snapshot is incompatible
> with replay, shouldn't manually marking all drives as snapshot=on be
> incompatible as well?
> 
> Maybe you're really interested in some specific drive not having
> snapshot=on? But then it might be better to check that specific drive
> instad of forbidding just the shortcut for setting it.

-snapshot adds the flag for top-level drive, making driver operations
dependent on temporary file structure.

Moving this overlay beneath blkreplay driver makes drive operations
deterministic for the top-level device.

Pavel Dovgalyuk

Re: [Qemu-devel] [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Kevin Wolf]

Am 18.09.2019 um 11:52 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Am 18.09.2019 um 11:37 hat Pavel Dovgalyuk geschrieben:
> > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > Am 18.09.2019 um 11:22 hat Pavel Dovgalyuk geschrieben:
> > > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > > Am 17.09.2019 um 13:58 hat Pavel Dovgalyuk geschrieben:
> > > > > > > From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> > > > > > >
> > > > > > > This patch updates the description of the command lines for using
> > > > > > > record/replay with attached block devices.
> > > > > > >
> > > > > > > Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> > > > > > > ---
> > > > > > >  docs/replay.txt |   12 +++++++++---
> > > > > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/docs/replay.txt b/docs/replay.txt
> > > > > > > index ee6aee9861..ce97c3f72f 100644
> > > > > > > --- a/docs/replay.txt
> > > > > > > +++ b/docs/replay.txt
> > > > > > > @@ -27,7 +27,7 @@ Usage of the record/replay:
> > > > > > >   * First, record the execution with the following command line:
> > > > > > >      qemu-system-i386 \
> > > > > > >       -icount shift=7,rr=record,rrfile=replay.bin \
> > > > > > > -     -drive file=disk.qcow2,if=none,id=img-direct \
> > > > > > > +     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
> > > > > > >       -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
> > > > > > >       -device ide-hd,drive=img-blkreplay \
> > > > > > >       -netdev user,id=net1 -device rtl8139,netdev=net1 \
> > > > > > > @@ -35,7 +35,7 @@ Usage of the record/replay:
> > > > > > >   * After recording, you can replay it by using another command line:
> > > > > > >      qemu-system-i386 \
> > > > > > >       -icount shift=7,rr=replay,rrfile=replay.bin \
> > > > > > > -     -drive file=disk.qcow2,if=none,id=img-direct \
> > > > > > > +     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
> > > > > > >       -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
> > > > > > >       -device ide-hd,drive=img-blkreplay \
> > > > > > >       -netdev user,id=net1 -device rtl8139,netdev=net1 \
> > > > > > > @@ -223,7 +223,7 @@ Block devices record/replay module intercepts calls of
> > > > > > >  bdrv coroutine functions at the top of block drivers stack.
> > > > > > >  To record and replay block operations the drive must be configured
> > > > > > >  as following:
> > > > > > > - -drive file=disk.qcow2,if=none,id=img-direct
> > > > > > > + -drive file=disk.qcow2,if=none,snapshot,id=img-direct
> > > > > > >   -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
> > > > > > >   -device ide-hd,drive=img-blkreplay
> > > > > > >
> > > > > > > @@ -252,6 +252,12 @@ This snapshot is created at start of recording and restored at
> > > > start
> > > > > > >  of replaying. It also can be loaded while replaying to roll back
> > > > > > >  the execution.
> > > > > > >
> > > > > > > +'snapshot' flag of the disk image must be removed to save the snapshots
> > > > > > > +in the overlay (or original image) instead of using the temporary overlay.
> > > > > > > + -drive file=disk.ovl,if=none,id=img-direct
> > > > > > > + -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
> > > > > > > + -device ide-hd,drive=img-blkreplay
> > > > > >
> > > > > > Wait, didn't patch 2 just make -snapshot unconditionally incompatible
> > > > > > with replay? So isn't saving the snapshot in the original image the only
> > > > > > supported mode now?
> > > > >
> > > > > There are two ways to run record/replay:
> > > > > 1. Disk with snapshot option and any image to make it unchanged
> > > > > 2. Disk with overlay without snapshot option to save VM snapshots on it
> > > >
> > > > Yes, I think I understand the two options that you intend to make
> > > > available, but when -snapshot sets a replay blocker, how can 1. still
> > > > work? Is there some code that ignores replay blockers?
> > >
> > > I checked the text and don't see anything about global "-snapshot" option.
> > > All references are about drive snapshot flag.
> > > Can you point me where did I missed that?
> > 
> > Oh, sorry, you're right and I misread.
> > 
> > However, global -snapshot is just a convenient shortcut for specifying
> > snapshot=on for all -drive arguments. So if -snapshot is incompatible
> > with replay, shouldn't manually marking all drives as snapshot=on be
> > incompatible as well?
> > 
> > Maybe you're really interested in some specific drive not having
> > snapshot=on? But then it might be better to check that specific drive
> > instad of forbidding just the shortcut for setting it.
> 
> -snapshot adds the flag for top-level drive, making driver operations
> dependent on temporary file structure.
> 
> Moving this overlay beneath blkreplay driver makes drive operations
> deterministic for the top-level device.

So the real requirement is that blkreplay is the top-level node of any
guest device, right? And only because of this, you can't use -snapshot
(or snapshot=on on the blkreplay driver).

If we instead check e.g. in blk_insert_bs() or blk_attach_dev() that in
record/replay mode, the root node of the BlockBackend is blkreplay,
wouldn't we catch many more incorrect setups?

Kevin

Re: [Qemu-devel] [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> > >
> > > However, global -snapshot is just a convenient shortcut for specifying
> > > snapshot=on for all -drive arguments. So if -snapshot is incompatible
> > > with replay, shouldn't manually marking all drives as snapshot=on be
> > > incompatible as well?
> > >
> > > Maybe you're really interested in some specific drive not having
> > > snapshot=on? But then it might be better to check that specific drive
> > > instad of forbidding just the shortcut for setting it.
> >
> > -snapshot adds the flag for top-level drive, making driver operations
> > dependent on temporary file structure.
> >
> > Moving this overlay beneath blkreplay driver makes drive operations
> > deterministic for the top-level device.
> 
> So the real requirement is that blkreplay is the top-level node of any
> guest device, right? And only because of this, you can't use -snapshot
> (or snapshot=on on the blkreplay driver).
> 
> If we instead check e.g. in blk_insert_bs() or blk_attach_dev() that in
> record/replay mode, the root node of the BlockBackend is blkreplay,
> wouldn't we catch many more incorrect setups?

That sounds interesting.
Will it help to check that every backend is connected to blkreplay?
How then this check has to be done?

Pavel Dovgalyuk

Re: [Qemu-devel] [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Kevin Wolf]

Am 19.09.2019 um 11:05 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > >
> > > > However, global -snapshot is just a convenient shortcut for specifying
> > > > snapshot=on for all -drive arguments. So if -snapshot is incompatible
> > > > with replay, shouldn't manually marking all drives as snapshot=on be
> > > > incompatible as well?
> > > >
> > > > Maybe you're really interested in some specific drive not having
> > > > snapshot=on? But then it might be better to check that specific drive
> > > > instad of forbidding just the shortcut for setting it.
> > >
> > > -snapshot adds the flag for top-level drive, making driver operations
> > > dependent on temporary file structure.
> > >
> > > Moving this overlay beneath blkreplay driver makes drive operations
> > > deterministic for the top-level device.
> > 
> > So the real requirement is that blkreplay is the top-level node of any
> > guest device, right? And only because of this, you can't use -snapshot
> > (or snapshot=on on the blkreplay driver).
> > 
> > If we instead check e.g. in blk_insert_bs() or blk_attach_dev() that in
> > record/replay mode, the root node of the BlockBackend is blkreplay,
> > wouldn't we catch many more incorrect setups?
> 
> That sounds interesting.
> Will it help to check that every backend is connected to blkreplay?

Yes, it would return an error when you try to attach a non-blkreplay
node to a BlockBackend (and every guest device uses a BlockBackend).

Note that this restriction would currently make block jobs unavailable
on non-blkreplay nodes as they also use BlockBackends internally (though
this is going to change in the long run). I believe this restriction is
harmless and the typical replay use case doesn't involve any block jobs,
but if you do think it's a problem, blk_attach_dev() would be the place
that affects only devices.

> How then this check has to be done?

Only compile-tested, but maybe something like below?

Kevin

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 0422acdf1c..9fa72bea51 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -955,6 +955,7 @@ static inline BlockDriverState *backing_bs(BlockDriverState *bs)
 extern BlockDriver bdrv_file;
 extern BlockDriver bdrv_raw;
 extern BlockDriver bdrv_qcow2;
+extern BlockDriver bdrv_blkreplay;
 
 int coroutine_fn bdrv_co_preadv(BdrvChild *child,
     int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
diff --git a/block/blkreplay.c b/block/blkreplay.c
index 2b7931b940..16a4f1df6a 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -126,7 +126,7 @@ static int coroutine_fn blkreplay_co_flush(BlockDriverState *bs)
     return ret;
 }
 
-static BlockDriver bdrv_blkreplay = {
+BlockDriver bdrv_blkreplay = {
     .format_name            = "blkreplay",
     .instance_size          = 0,
 
diff --git a/block/block-backend.c b/block/block-backend.c
index 1c605d5444..c57d3d9fdf 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -17,6 +17,7 @@
 #include "block/throttle-groups.h"
 #include "hw/qdev-core.h"
 #include "sysemu/blockdev.h"
+#include "sysemu/replay.h"
 #include "sysemu/runstate.h"
 #include "qapi/error.h"
 #include "qapi/qapi-events-block.h"
@@ -808,6 +809,12 @@ void blk_remove_bs(BlockBackend *blk)
 int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
 {
     ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
+
+    if (replay_mode != REPLAY_MODE_NONE && bs->drv != &bdrv_blkreplay) {
+        error_setg(errp, "Root node must be blkreplay");
+        return -ENOTSUP;
+    }
+
     bdrv_ref(bs);
     blk->root = bdrv_root_attach_child(bs, "root", &child_root, blk->ctx,
                                        blk->perm, blk->shared_perm, blk, errp);

Re: [Qemu-devel] [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Am 19.09.2019 um 11:05 hat Pavel Dovgalyuk geschrieben:
> > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > >
> > > > > However, global -snapshot is just a convenient shortcut for specifying
> > > > > snapshot=on for all -drive arguments. So if -snapshot is incompatible
> > > > > with replay, shouldn't manually marking all drives as snapshot=on be
> > > > > incompatible as well?
> > > > >
> > > > > Maybe you're really interested in some specific drive not having
> > > > > snapshot=on? But then it might be better to check that specific drive
> > > > > instad of forbidding just the shortcut for setting it.
> > > >
> > > > -snapshot adds the flag for top-level drive, making driver operations
> > > > dependent on temporary file structure.
> > > >
> > > > Moving this overlay beneath blkreplay driver makes drive operations
> > > > deterministic for the top-level device.
> > >
> > > So the real requirement is that blkreplay is the top-level node of any
> > > guest device, right? And only because of this, you can't use -snapshot
> > > (or snapshot=on on the blkreplay driver).
> > >
> > > If we instead check e.g. in blk_insert_bs() or blk_attach_dev() that in
> > > record/replay mode, the root node of the BlockBackend is blkreplay,
> > > wouldn't we catch many more incorrect setups?
> >
> > That sounds interesting.
> > Will it help to check that every backend is connected to blkreplay?
> 
> Yes, it would return an error when you try to attach a non-blkreplay
> node to a BlockBackend (and every guest device uses a BlockBackend).
> 
> Note that this restriction would currently make block jobs unavailable
> on non-blkreplay nodes as they also use BlockBackends internally (though
> this is going to change in the long run). I believe this restriction is
> harmless and the typical replay use case doesn't involve any block jobs,
> but if you do think it's a problem, blk_attach_dev() would be the place
> that affects only devices.
> 
> > How then this check has to be done?
> 
> Only compile-tested, but maybe something like below?
> 
> Kevin
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 0422acdf1c..9fa72bea51 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -955,6 +955,7 @@ static inline BlockDriverState *backing_bs(BlockDriverState *bs)
>  extern BlockDriver bdrv_file;
>  extern BlockDriver bdrv_raw;
>  extern BlockDriver bdrv_qcow2;
> +extern BlockDriver bdrv_blkreplay;
> 
>  int coroutine_fn bdrv_co_preadv(BdrvChild *child,
>      int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
> diff --git a/block/blkreplay.c b/block/blkreplay.c
> index 2b7931b940..16a4f1df6a 100644
> --- a/block/blkreplay.c
> +++ b/block/blkreplay.c
> @@ -126,7 +126,7 @@ static int coroutine_fn blkreplay_co_flush(BlockDriverState *bs)
>      return ret;
>  }
> 
> -static BlockDriver bdrv_blkreplay = {
> +BlockDriver bdrv_blkreplay = {
>      .format_name            = "blkreplay",
>      .instance_size          = 0,
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 1c605d5444..c57d3d9fdf 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -17,6 +17,7 @@
>  #include "block/throttle-groups.h"
>  #include "hw/qdev-core.h"
>  #include "sysemu/blockdev.h"
> +#include "sysemu/replay.h"
>  #include "sysemu/runstate.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-events-block.h"
> @@ -808,6 +809,12 @@ void blk_remove_bs(BlockBackend *blk)
>  int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
>  {
>      ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
> +
> +    if (replay_mode != REPLAY_MODE_NONE && bs->drv != &bdrv_blkreplay) {
> +        error_setg(errp, "Root node must be blkreplay");
> +        return -ENOTSUP;
> +    }

I guess this is opposite direction - bs->drv is bdrv_file.
And we should check its parent.

> +
>      bdrv_ref(bs);
>      blk->root = bdrv_root_attach_child(bs, "root", &child_root, blk->ctx,
>                                         blk->perm, blk->shared_perm, blk, errp);

Pavel Dovgalyuk

Re: [Qemu-devel] [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Kevin Wolf]

Am 19.09.2019 um 14:10 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Am 19.09.2019 um 11:05 hat Pavel Dovgalyuk geschrieben:
> > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > >
> > > > > > However, global -snapshot is just a convenient shortcut for specifying
> > > > > > snapshot=on for all -drive arguments. So if -snapshot is incompatible
> > > > > > with replay, shouldn't manually marking all drives as snapshot=on be
> > > > > > incompatible as well?
> > > > > >
> > > > > > Maybe you're really interested in some specific drive not having
> > > > > > snapshot=on? But then it might be better to check that specific drive
> > > > > > instad of forbidding just the shortcut for setting it.
> > > > >
> > > > > -snapshot adds the flag for top-level drive, making driver operations
> > > > > dependent on temporary file structure.
> > > > >
> > > > > Moving this overlay beneath blkreplay driver makes drive operations
> > > > > deterministic for the top-level device.
> > > >
> > > > So the real requirement is that blkreplay is the top-level node of any
> > > > guest device, right? And only because of this, you can't use -snapshot
> > > > (or snapshot=on on the blkreplay driver).
> > > >
> > > > If we instead check e.g. in blk_insert_bs() or blk_attach_dev() that in
> > > > record/replay mode, the root node of the BlockBackend is blkreplay,
> > > > wouldn't we catch many more incorrect setups?
> > >
> > > That sounds interesting.
> > > Will it help to check that every backend is connected to blkreplay?
> > 
> > Yes, it would return an error when you try to attach a non-blkreplay
> > node to a BlockBackend (and every guest device uses a BlockBackend).
> > 
> > Note that this restriction would currently make block jobs unavailable
> > on non-blkreplay nodes as they also use BlockBackends internally (though
> > this is going to change in the long run). I believe this restriction is
> > harmless and the typical replay use case doesn't involve any block jobs,
> > but if you do think it's a problem, blk_attach_dev() would be the place
> > that affects only devices.
> > 
> > > How then this check has to be done?
> > 
> > Only compile-tested, but maybe something like below?
> > 
> > Kevin
> > 
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index 0422acdf1c..9fa72bea51 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -955,6 +955,7 @@ static inline BlockDriverState *backing_bs(BlockDriverState *bs)
> >  extern BlockDriver bdrv_file;
> >  extern BlockDriver bdrv_raw;
> >  extern BlockDriver bdrv_qcow2;
> > +extern BlockDriver bdrv_blkreplay;
> > 
> >  int coroutine_fn bdrv_co_preadv(BdrvChild *child,
> >      int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
> > diff --git a/block/blkreplay.c b/block/blkreplay.c
> > index 2b7931b940..16a4f1df6a 100644
> > --- a/block/blkreplay.c
> > +++ b/block/blkreplay.c
> > @@ -126,7 +126,7 @@ static int coroutine_fn blkreplay_co_flush(BlockDriverState *bs)
> >      return ret;
> >  }
> > 
> > -static BlockDriver bdrv_blkreplay = {
> > +BlockDriver bdrv_blkreplay = {
> >      .format_name            = "blkreplay",
> >      .instance_size          = 0,
> > 
> > diff --git a/block/block-backend.c b/block/block-backend.c
> > index 1c605d5444..c57d3d9fdf 100644
> > --- a/block/block-backend.c
> > +++ b/block/block-backend.c
> > @@ -17,6 +17,7 @@
> >  #include "block/throttle-groups.h"
> >  #include "hw/qdev-core.h"
> >  #include "sysemu/blockdev.h"
> > +#include "sysemu/replay.h"
> >  #include "sysemu/runstate.h"
> >  #include "qapi/error.h"
> >  #include "qapi/qapi-events-block.h"
> > @@ -808,6 +809,12 @@ void blk_remove_bs(BlockBackend *blk)
> >  int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
> >  {
> >      ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
> > +
> > +    if (replay_mode != REPLAY_MODE_NONE && bs->drv != &bdrv_blkreplay) {
> > +        error_setg(errp, "Root node must be blkreplay");
> > +        return -ENOTSUP;
> > +    }
> 
> I guess this is opposite direction - bs->drv is bdrv_file.
> And we should check its parent.

If bs->drv is bdrv_file, you want this to fail because only
bdrv_blkreplay should be able to be attached to devices.

bs doesn't have any parents here in the common case, it's the root node
of the BlockBackend.

Kevin

RE: [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Am 19.09.2019 um 14:10 hat Pavel Dovgalyuk geschrieben:
> > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > Am 19.09.2019 um 11:05 hat Pavel Dovgalyuk geschrieben:
> > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > > >
> > > > > > > However, global -snapshot is just a convenient shortcut for specifying
> > > > > > > snapshot=on for all -drive arguments. So if -snapshot is incompatible
> > > > > > > with replay, shouldn't manually marking all drives as snapshot=on be
> > > > > > > incompatible as well?
> > > > > > >
> > > > > > > Maybe you're really interested in some specific drive not having
> > > > > > > snapshot=on? But then it might be better to check that specific drive
> > > > > > > instad of forbidding just the shortcut for setting it.
> > > > > >
> > > > > > -snapshot adds the flag for top-level drive, making driver operations
> > > > > > dependent on temporary file structure.
> > > > > >
> > > > > > Moving this overlay beneath blkreplay driver makes drive operations
> > > > > > deterministic for the top-level device.
> > > > >
> > > > > So the real requirement is that blkreplay is the top-level node of any
> > > > > guest device, right? And only because of this, you can't use -snapshot
> > > > > (or snapshot=on on the blkreplay driver).
> > > > >
> > > > > If we instead check e.g. in blk_insert_bs() or blk_attach_dev() that in
> > > > > record/replay mode, the root node of the BlockBackend is blkreplay,
> > > > > wouldn't we catch many more incorrect setups?
> > > >
> > > > That sounds interesting.
> > > > Will it help to check that every backend is connected to blkreplay?
> > >
> > > Yes, it would return an error when you try to attach a non-blkreplay
> > > node to a BlockBackend (and every guest device uses a BlockBackend).
> > >
> > > Note that this restriction would currently make block jobs unavailable
> > > on non-blkreplay nodes as they also use BlockBackends internally (though
> > > this is going to change in the long run). I believe this restriction is
> > > harmless and the typical replay use case doesn't involve any block jobs,
> > > but if you do think it's a problem, blk_attach_dev() would be the place
> > > that affects only devices.
> > >
> > > > How then this check has to be done?
> > >
> > > Only compile-tested, but maybe something like below?
> > >
> > > Kevin
> > >
> > > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > > index 0422acdf1c..9fa72bea51 100644
> > > --- a/include/block/block_int.h
> > > +++ b/include/block/block_int.h
> > > @@ -955,6 +955,7 @@ static inline BlockDriverState *backing_bs(BlockDriverState *bs)
> > >  extern BlockDriver bdrv_file;
> > >  extern BlockDriver bdrv_raw;
> > >  extern BlockDriver bdrv_qcow2;
> > > +extern BlockDriver bdrv_blkreplay;
> > >
> > >  int coroutine_fn bdrv_co_preadv(BdrvChild *child,
> > >      int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
> > > diff --git a/block/blkreplay.c b/block/blkreplay.c
> > > index 2b7931b940..16a4f1df6a 100644
> > > --- a/block/blkreplay.c
> > > +++ b/block/blkreplay.c
> > > @@ -126,7 +126,7 @@ static int coroutine_fn blkreplay_co_flush(BlockDriverState *bs)
> > >      return ret;
> > >  }
> > >
> > > -static BlockDriver bdrv_blkreplay = {
> > > +BlockDriver bdrv_blkreplay = {
> > >      .format_name            = "blkreplay",
> > >      .instance_size          = 0,
> > >
> > > diff --git a/block/block-backend.c b/block/block-backend.c
> > > index 1c605d5444..c57d3d9fdf 100644
> > > --- a/block/block-backend.c
> > > +++ b/block/block-backend.c
> > > @@ -17,6 +17,7 @@
> > >  #include "block/throttle-groups.h"
> > >  #include "hw/qdev-core.h"
> > >  #include "sysemu/blockdev.h"
> > > +#include "sysemu/replay.h"
> > >  #include "sysemu/runstate.h"
> > >  #include "qapi/error.h"
> > >  #include "qapi/qapi-events-block.h"
> > > @@ -808,6 +809,12 @@ void blk_remove_bs(BlockBackend *blk)
> > >  int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
> > >  {
> > >      ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
> > > +
> > > +    if (replay_mode != REPLAY_MODE_NONE && bs->drv != &bdrv_blkreplay) {
> > > +        error_setg(errp, "Root node must be blkreplay");
> > > +        return -ENOTSUP;
> > > +    }
> >
> > I guess this is opposite direction - bs->drv is bdrv_file.
> > And we should check its parent.
> 
> If bs->drv is bdrv_file, you want this to fail because only
> bdrv_blkreplay should be able to be attached to devices.

There was a regular rr invocation (as described in docs).
And bs->drv always was a pointer to bdrv_file: for original image,
and for temporary snapshot.

Pavel Dovgalyuk

Re: [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Kevin Wolf]

Am 20.09.2019 um 09:25 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Am 19.09.2019 um 14:10 hat Pavel Dovgalyuk geschrieben:
> > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > diff --git a/block/block-backend.c b/block/block-backend.c
> > > > index 1c605d5444..c57d3d9fdf 100644
> > > > --- a/block/block-backend.c
> > > > +++ b/block/block-backend.c
> > > > @@ -17,6 +17,7 @@
> > > >  #include "block/throttle-groups.h"
> > > >  #include "hw/qdev-core.h"
> > > >  #include "sysemu/blockdev.h"
> > > > +#include "sysemu/replay.h"
> > > >  #include "sysemu/runstate.h"
> > > >  #include "qapi/error.h"
> > > >  #include "qapi/qapi-events-block.h"
> > > > @@ -808,6 +809,12 @@ void blk_remove_bs(BlockBackend *blk)
> > > >  int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
> > > >  {
> > > >      ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
> > > > +
> > > > +    if (replay_mode != REPLAY_MODE_NONE && bs->drv != &bdrv_blkreplay) {
> > > > +        error_setg(errp, "Root node must be blkreplay");
> > > > +        return -ENOTSUP;
> > > > +    }
> > >
> > > I guess this is opposite direction - bs->drv is bdrv_file.
> > > And we should check its parent.
> > 
> > If bs->drv is bdrv_file, you want this to fail because only
> > bdrv_blkreplay should be able to be attached to devices.
> 
> There was a regular rr invocation (as described in docs).
> And bs->drv always was a pointer to bdrv_file: for original image,
> and for temporary snapshot.

Hm, what was the actual command line you used? I can see that you have a
separate -drive for the qcow2 file, so I can see how you get an unused
BlockBackend for the qcow2 node, but I don't see how it would be a file
node.

Anyway, this leaves us two options: Either change the recommended
command line to use -blockdev for the qcow2 file so that no BlockBackend
is created for it (I think this might be preferable), or restrict the
error to when the BlockBackend is used.

The second option would probably have to be done in blk_attach_dev().
The problem with this is that the only expected error so far is that the
BlockBackend is already attached to a different device. So if you return
a new error there, you will have to touch its callers as well to have
this error correctly passed to the user.

Kevin

RE: [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Am 20.09.2019 um 09:25 hat Pavel Dovgalyuk geschrieben:
> > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > Am 19.09.2019 um 14:10 hat Pavel Dovgalyuk geschrieben:
> > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > diff --git a/block/block-backend.c b/block/block-backend.c
> > > > > index 1c605d5444..c57d3d9fdf 100644
> > > > > --- a/block/block-backend.c
> > > > > +++ b/block/block-backend.c
> > > > > @@ -17,6 +17,7 @@
> > > > >  #include "block/throttle-groups.h"
> > > > >  #include "hw/qdev-core.h"
> > > > >  #include "sysemu/blockdev.h"
> > > > > +#include "sysemu/replay.h"
> > > > >  #include "sysemu/runstate.h"
> > > > >  #include "qapi/error.h"
> > > > >  #include "qapi/qapi-events-block.h"
> > > > > @@ -808,6 +809,12 @@ void blk_remove_bs(BlockBackend *blk)
> > > > >  int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
> > > > >  {
> > > > >      ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
> > > > > +
> > > > > +    if (replay_mode != REPLAY_MODE_NONE && bs->drv != &bdrv_blkreplay) {
> > > > > +        error_setg(errp, "Root node must be blkreplay");
> > > > > +        return -ENOTSUP;
> > > > > +    }
> > > >
> > > > I guess this is opposite direction - bs->drv is bdrv_file.
> > > > And we should check its parent.
> > >
> > > If bs->drv is bdrv_file, you want this to fail because only
> > > bdrv_blkreplay should be able to be attached to devices.
> >
> > There was a regular rr invocation (as described in docs).
> > And bs->drv always was a pointer to bdrv_file: for original image,
> > and for temporary snapshot.
> 
> Hm, what was the actual command line you used? I can see that you have a
> separate -drive for the qcow2 file, so I can see how you get an unused
> BlockBackend for the qcow2 node, but I don't see how it would be a file
> node.

The command line was almost the same as in docs:

-drive file=disk.img,format=raw,snapshot,id=img-direct \
-drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
-device virtio-blk-device,drive=img-blkreplay \

I've got the following blk_insert_bs invocations:
1. bs=ptr1 bs->drv=&bdrv_file, bs->filename=disk.img
2. bs=ptr2 bs->drv=&bdrv_file, bs->filename=<tmp filename>
3. bs=ptr2 bs->drv=&bdrv_file, bs->filename=<tmp filename>
4. bs=ptr2 bs->drv=&bdrv_file, bs->filename=<tmp filename>
5. bs=ptr3 bs->drv=&bdrv_file, bs->filename=<tmp filename>

> Anyway, this leaves us two options: Either change the recommended
> command line to use -blockdev for the qcow2 file so that no BlockBackend
> is created for it (I think this might be preferable), or restrict the
> error to when the BlockBackend is used.
> 
> The second option would probably have to be done in blk_attach_dev().
> The problem with this is that the only expected error so far is that the
> BlockBackend is already attached to a different device. So if you return
> a new error there, you will have to touch its callers as well to have
> this error correctly passed to the user.

Pavel Dovgalyuk

RE: [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Am 20.09.2019 um 09:25 hat Pavel Dovgalyuk geschrieben:
> > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > Am 19.09.2019 um 14:10 hat Pavel Dovgalyuk geschrieben:
> > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > diff --git a/block/block-backend.c b/block/block-backend.c
> > > > > index 1c605d5444..c57d3d9fdf 100644
> > > > > --- a/block/block-backend.c
> > > > > +++ b/block/block-backend.c
> > > > > @@ -17,6 +17,7 @@
> > > > >  #include "block/throttle-groups.h"
> > > > >  #include "hw/qdev-core.h"
> > > > >  #include "sysemu/blockdev.h"
> > > > > +#include "sysemu/replay.h"
> > > > >  #include "sysemu/runstate.h"
> > > > >  #include "qapi/error.h"
> > > > >  #include "qapi/qapi-events-block.h"
> > > > > @@ -808,6 +809,12 @@ void blk_remove_bs(BlockBackend *blk)
> > > > >  int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
> > > > >  {
> > > > >      ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
> > > > > +
> > > > > +    if (replay_mode != REPLAY_MODE_NONE && bs->drv != &bdrv_blkreplay) {
> > > > > +        error_setg(errp, "Root node must be blkreplay");
> > > > > +        return -ENOTSUP;
> > > > > +    }
> > > >
> > > > I guess this is opposite direction - bs->drv is bdrv_file.
> > > > And we should check its parent.
> > >
> > > If bs->drv is bdrv_file, you want this to fail because only
> > > bdrv_blkreplay should be able to be attached to devices.
> >
> > There was a regular rr invocation (as described in docs).
> > And bs->drv always was a pointer to bdrv_file: for original image,
> > and for temporary snapshot.
> 
> Hm, what was the actual command line you used? I can see that you have a
> separate -drive for the qcow2 file, so I can see how you get an unused
> BlockBackend for the qcow2 node, but I don't see how it would be a file
> node.
> 
> Anyway, this leaves us two options: Either change the recommended
> command line to use -blockdev for the qcow2 file so that no BlockBackend
> is created for it (I think this might be preferable), or restrict the
> error to when the BlockBackend is used.

I started playing with -blockdev: added new blockdev for blkreplay and
constructed the following command line:

-blockdev driver=file,filename=disk.img,node-name=hd0
-blockdev driver=blkreplay,file=hd0,node-name=hd0-rr
-device virtio-blk-device,drive=hd0-rr

However, I get an error: "Could not open 'disk.img': Permission denied"
Everything works when I use this file in '-drive' parameter.
What am I doing wrong?

Pavel Dovgalyuk

RE: [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Pavel Dovgalyuk]

Kevin, can you give any hint on using blockdev through the command line?

Pavel Dovgalyuk


> -----Original Message-----
> From: Pavel Dovgalyuk [mailto:dovgaluk@ispras.ru]
> Sent: Wednesday, September 25, 2019 12:03 PM
> To: 'Kevin Wolf'
> Cc: qemu-devel@nongnu.org; peter.maydell@linaro.org; crosthwaite.peter@gmail.com;
> boost.lists@gmail.com; artem.k.pisarenko@gmail.com; quintela@redhat.com;
> ciro.santilli@gmail.com; jasowang@redhat.com; mst@redhat.com; armbru@redhat.com;
> mreitz@redhat.com; maria.klimushenkova@ispras.ru; kraxel@redhat.com; pavel.dovgaluk@ispras.ru;
> thomas.dullien@googlemail.com; pbonzini@redhat.com; alex.bennee@linaro.org;
> dgilbert@redhat.com; rth@twiddle.net
> Subject: RE: [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices
> 
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Am 20.09.2019 um 09:25 hat Pavel Dovgalyuk geschrieben:
> > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > Am 19.09.2019 um 14:10 hat Pavel Dovgalyuk geschrieben:
> > > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > > diff --git a/block/block-backend.c b/block/block-backend.c
> > > > > > index 1c605d5444..c57d3d9fdf 100644
> > > > > > --- a/block/block-backend.c
> > > > > > +++ b/block/block-backend.c
> > > > > > @@ -17,6 +17,7 @@
> > > > > >  #include "block/throttle-groups.h"
> > > > > >  #include "hw/qdev-core.h"
> > > > > >  #include "sysemu/blockdev.h"
> > > > > > +#include "sysemu/replay.h"
> > > > > >  #include "sysemu/runstate.h"
> > > > > >  #include "qapi/error.h"
> > > > > >  #include "qapi/qapi-events-block.h"
> > > > > > @@ -808,6 +809,12 @@ void blk_remove_bs(BlockBackend *blk)
> > > > > >  int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
> > > > > >  {
> > > > > >      ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
> > > > > > +
> > > > > > +    if (replay_mode != REPLAY_MODE_NONE && bs->drv != &bdrv_blkreplay) {
> > > > > > +        error_setg(errp, "Root node must be blkreplay");
> > > > > > +        return -ENOTSUP;
> > > > > > +    }
> > > > >
> > > > > I guess this is opposite direction - bs->drv is bdrv_file.
> > > > > And we should check its parent.
> > > >
> > > > If bs->drv is bdrv_file, you want this to fail because only
> > > > bdrv_blkreplay should be able to be attached to devices.
> > >
> > > There was a regular rr invocation (as described in docs).
> > > And bs->drv always was a pointer to bdrv_file: for original image,
> > > and for temporary snapshot.
> >
> > Hm, what was the actual command line you used? I can see that you have a
> > separate -drive for the qcow2 file, so I can see how you get an unused
> > BlockBackend for the qcow2 node, but I don't see how it would be a file
> > node.
> >
> > Anyway, this leaves us two options: Either change the recommended
> > command line to use -blockdev for the qcow2 file so that no BlockBackend
> > is created for it (I think this might be preferable), or restrict the
> > error to when the BlockBackend is used.
> 
> I started playing with -blockdev: added new blockdev for blkreplay and
> constructed the following command line:
> 
> -blockdev driver=file,filename=disk.img,node-name=hd0
> -blockdev driver=blkreplay,file=hd0,node-name=hd0-rr
> -device virtio-blk-device,drive=hd0-rr
> 
> However, I get an error: "Could not open 'disk.img': Permission denied"
> Everything works when I use this file in '-drive' parameter.
> What am I doing wrong?
> 
> Pavel Dovgalyuk
> 

Re: [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Kevin Wolf]

Am 25.09.2019 um 11:02 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Am 20.09.2019 um 09:25 hat Pavel Dovgalyuk geschrieben:
> > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > Am 19.09.2019 um 14:10 hat Pavel Dovgalyuk geschrieben:
> > > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > > diff --git a/block/block-backend.c b/block/block-backend.c
> > > > > > index 1c605d5444..c57d3d9fdf 100644
> > > > > > --- a/block/block-backend.c
> > > > > > +++ b/block/block-backend.c
> > > > > > @@ -17,6 +17,7 @@
> > > > > >  #include "block/throttle-groups.h"
> > > > > >  #include "hw/qdev-core.h"
> > > > > >  #include "sysemu/blockdev.h"
> > > > > > +#include "sysemu/replay.h"
> > > > > >  #include "sysemu/runstate.h"
> > > > > >  #include "qapi/error.h"
> > > > > >  #include "qapi/qapi-events-block.h"
> > > > > > @@ -808,6 +809,12 @@ void blk_remove_bs(BlockBackend *blk)
> > > > > >  int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
> > > > > >  {
> > > > > >      ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
> > > > > > +
> > > > > > +    if (replay_mode != REPLAY_MODE_NONE && bs->drv != &bdrv_blkreplay) {
> > > > > > +        error_setg(errp, "Root node must be blkreplay");
> > > > > > +        return -ENOTSUP;
> > > > > > +    }
> > > > >
> > > > > I guess this is opposite direction - bs->drv is bdrv_file.
> > > > > And we should check its parent.
> > > >
> > > > If bs->drv is bdrv_file, you want this to fail because only
> > > > bdrv_blkreplay should be able to be attached to devices.
> > >
> > > There was a regular rr invocation (as described in docs).
> > > And bs->drv always was a pointer to bdrv_file: for original image,
> > > and for temporary snapshot.
> > 
> > Hm, what was the actual command line you used? I can see that you have a
> > separate -drive for the qcow2 file, so I can see how you get an unused
> > BlockBackend for the qcow2 node, but I don't see how it would be a file
> > node.
> > 
> > Anyway, this leaves us two options: Either change the recommended
> > command line to use -blockdev for the qcow2 file so that no BlockBackend
> > is created for it (I think this might be preferable), or restrict the
> > error to when the BlockBackend is used.
> 
> I started playing with -blockdev: added new blockdev for blkreplay and
> constructed the following command line:
> 
> -blockdev driver=file,filename=disk.img,node-name=hd0
> -blockdev driver=blkreplay,file=hd0,node-name=hd0-rr
> -device virtio-blk-device,drive=hd0-rr
> 
> However, I get an error: "Could not open 'disk.img': Permission denied"
> Everything works when I use this file in '-drive' parameter.
> What am I doing wrong?

The reason why I didn't reply immediately is because I don't see
anything wrong in the options you used.

Just to confirm, do you still get the same error when you use only the
first -blockdev option and no other options at all?


I've now tried out the options you gave, and it does fail for me, but
with a different error:

    qemu-system-x86_64: -blockdev driver=blkreplay,file=hd0,node-name=hd0-rr: Invalid parameter 'blkreplay'

This one is because the QAPI schema doesn't know blkreplay and should
easily be fixed by adding a blkreplay field to BlockdevOptions.


As soft freeze is coming closer, I'm considering taking this series as
it is (it's wrong in parts, but the old state is probably even more
wrong) and letting you fix up these checks on top. What do you think?

Kevin

RE: [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Am 25.09.2019 um 11:02 hat Pavel Dovgalyuk geschrieben:
> > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > Am 20.09.2019 um 09:25 hat Pavel Dovgalyuk geschrieben:
> > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > Am 19.09.2019 um 14:10 hat Pavel Dovgalyuk geschrieben:
> > > > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > > > diff --git a/block/block-backend.c b/block/block-backend.c
> > > > > > > index 1c605d5444..c57d3d9fdf 100644
> > > > > > > --- a/block/block-backend.c
> > > > > > > +++ b/block/block-backend.c
> > > > > > > @@ -17,6 +17,7 @@
> > > > > > >  #include "block/throttle-groups.h"
> > > > > > >  #include "hw/qdev-core.h"
> > > > > > >  #include "sysemu/blockdev.h"
> > > > > > > +#include "sysemu/replay.h"
> > > > > > >  #include "sysemu/runstate.h"
> > > > > > >  #include "qapi/error.h"
> > > > > > >  #include "qapi/qapi-events-block.h"
> > > > > > > @@ -808,6 +809,12 @@ void blk_remove_bs(BlockBackend *blk)
> > > > > > >  int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
> > > > > > >  {
> > > > > > >      ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
> > > > > > > +
> > > > > > > +    if (replay_mode != REPLAY_MODE_NONE && bs->drv != &bdrv_blkreplay) {
> > > > > > > +        error_setg(errp, "Root node must be blkreplay");
> > > > > > > +        return -ENOTSUP;
> > > > > > > +    }
> > > > > >
> > > > > > I guess this is opposite direction - bs->drv is bdrv_file.
> > > > > > And we should check its parent.
> > > > >
> > > > > If bs->drv is bdrv_file, you want this to fail because only
> > > > > bdrv_blkreplay should be able to be attached to devices.
> > > >
> > > > There was a regular rr invocation (as described in docs).
> > > > And bs->drv always was a pointer to bdrv_file: for original image,
> > > > and for temporary snapshot.
> > >
> > > Hm, what was the actual command line you used? I can see that you have a
> > > separate -drive for the qcow2 file, so I can see how you get an unused
> > > BlockBackend for the qcow2 node, but I don't see how it would be a file
> > > node.
> > >
> > > Anyway, this leaves us two options: Either change the recommended
> > > command line to use -blockdev for the qcow2 file so that no BlockBackend
> > > is created for it (I think this might be preferable), or restrict the
> > > error to when the BlockBackend is used.
> >
> > I started playing with -blockdev: added new blockdev for blkreplay and
> > constructed the following command line:
> >
> > -blockdev driver=file,filename=disk.img,node-name=hd0
> > -blockdev driver=blkreplay,file=hd0,node-name=hd0-rr
> > -device virtio-blk-device,drive=hd0-rr
> >
> > However, I get an error: "Could not open 'disk.img': Permission denied"
> > Everything works when I use this file in '-drive' parameter.
> > What am I doing wrong?
> 
> The reason why I didn't reply immediately is because I don't see
> anything wrong in the options you used.
> 
> Just to confirm, do you still get the same error when you use only the
> first -blockdev option and no other options at all?

Ok, I tried again and got different error, which was caused by incorrect
QAPI schema for blkreplay.
Now it seems ok, but I still can't boot.

> I've now tried out the options you gave, and it does fail for me, but
> with a different error:
> 
>     qemu-system-x86_64: -blockdev driver=blkreplay,file=hd0,node-name=hd0-rr: Invalid
> parameter 'blkreplay'
> 
> This one is because the QAPI schema doesn't know blkreplay and should
> easily be fixed by adding a blkreplay field to BlockdevOptions.

Right, I added the following schema:

--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2832,8 +2832,8 @@
 # Since: 2.9
 ##
 { 'enum': 'BlockdevDriver',
-  'data': [ 'blkdebug', 'blklogwrites', 'blkverify', 'bochs', 'cloop',
-            'copy-on-read', 'dmg', 'file', 'ftp', 'ftps', 'gluster',
+  'data': [ 'blkdebug', 'blklogwrites', 'blkreplay', 'blkverify', 'bochs',
+            'cloop', 'copy-on-read', 'dmg', 'file', 'ftp', 'ftps', 'gluster',
             'host_cdrom', 'host_device', 'http', 'https', 'iscsi', 'luks',
             'nbd', 'nfs', 'null-aio', 'null-co', 'nvme', 'parallels', 'qcow',
             'qcow2', 'qed', 'quorum', 'raw', 'rbd',
@@ -3446,6 +3446,18 @@
   'data': { 'test': 'BlockdevRef',
             'raw': 'BlockdevRef' } }
 
+##
+# @BlockdevOptionsBlkreplay:
+#
+# Driver specific block device options for blkreplay.
+#
+# @image:     disk image which should be controlled with blkreplay
+#
+# Since: 4.2
+##
+{ 'struct': 'BlockdevOptionsBlkreplay',
+  'data': { 'image': 'BlockdevRef' } }
+
 ##
 # @QuorumReadPattern:
 #
@@ -3973,6 +3985,7 @@
       'blkdebug':   'BlockdevOptionsBlkdebug',
       'blklogwrites':'BlockdevOptionsBlklogwrites',
       'blkverify':  'BlockdevOptionsBlkverify',
+      'blkreplay':  'BlockdevOptionsBlkreplay',
       'bochs':      'BlockdevOptionsGenericFormat',
       'cloop':      'BlockdevOptionsGenericFormat',
       'copy-on-read':'BlockdevOptionsGenericFormat',

> As soft freeze is coming closer, I'm considering taking this series as
> it is (it's wrong in parts, but the old state is probably even more
> wrong) and letting you fix up these checks on top. What do you think?

That sounds reasonable.

Pavel Dovgalyuk

Re: [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Kevin Wolf]

Am 11.10.2019 um 08:10 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Am 25.09.2019 um 11:02 hat Pavel Dovgalyuk geschrieben:
> > > I started playing with -blockdev: added new blockdev for blkreplay and
> > > constructed the following command line:
> > >
> > > -blockdev driver=file,filename=disk.img,node-name=hd0
> > > -blockdev driver=blkreplay,file=hd0,node-name=hd0-rr
> > > -device virtio-blk-device,drive=hd0-rr
> > >
> > > However, I get an error: "Could not open 'disk.img': Permission denied"
> > > Everything works when I use this file in '-drive' parameter.
> > > What am I doing wrong?
> > 
> > The reason why I didn't reply immediately is because I don't see
> > anything wrong in the options you used.
> > 
> > Just to confirm, do you still get the same error when you use only the
> > first -blockdev option and no other options at all?
> 
> Ok, I tried again and got different error, which was caused by incorrect
> QAPI schema for blkreplay.
> Now it seems ok, but I still can't boot.

Hm... Are you actually using a raw image? If not, you need the format
driver, too, and would end up with something like:

 -blockdev driver=file,filename=disk.qcow2,node-name=hd0
 -blockdev driver=qcow2,file=hd0,node-name=hd0-qcow2
 -blockdev driver=blkreplay,file=hd0-qcow2,node-name=hd0-rr
 -device virtio-blk-device,drive=hd0-rr

(The first two can be combined into a single option by using a syntax
like file.driver=file,file.filename=disk.qcow2, but defining each node
separately is a bit cleaner.)

> > I've now tried out the options you gave, and it does fail for me, but
> > with a different error:
> > 
> >     qemu-system-x86_64: -blockdev driver=blkreplay,file=hd0,node-name=hd0-rr: Invalid
> > parameter 'blkreplay'
> > 
> > This one is because the QAPI schema doesn't know blkreplay and should
> > easily be fixed by adding a blkreplay field to BlockdevOptions.
> 
> Right, I added the following schema:
> 
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2832,8 +2832,8 @@
>  # Since: 2.9
>  ##
>  { 'enum': 'BlockdevDriver',
> -  'data': [ 'blkdebug', 'blklogwrites', 'blkverify', 'bochs', 'cloop',
> -            'copy-on-read', 'dmg', 'file', 'ftp', 'ftps', 'gluster',
> +  'data': [ 'blkdebug', 'blklogwrites', 'blkreplay', 'blkverify', 'bochs',
> +            'cloop', 'copy-on-read', 'dmg', 'file', 'ftp', 'ftps', 'gluster',
>              'host_cdrom', 'host_device', 'http', 'https', 'iscsi', 'luks',
>              'nbd', 'nfs', 'null-aio', 'null-co', 'nvme', 'parallels', 'qcow',
>              'qcow2', 'qed', 'quorum', 'raw', 'rbd',

Please add a '@blkreplay: Since 4.2' note to the comment, too.

> @@ -3446,6 +3446,18 @@
>    'data': { 'test': 'BlockdevRef',
>              'raw': 'BlockdevRef' } }
>  
> +##
> +# @BlockdevOptionsBlkreplay:
> +#
> +# Driver specific block device options for blkreplay.
> +#
> +# @image:     disk image which should be controlled with blkreplay
> +#
> +# Since: 4.2
> +##
> +{ 'struct': 'BlockdevOptionsBlkreplay',
> +  'data': { 'image': 'BlockdevRef' } }
> +
>  ##
>  # @QuorumReadPattern:
>  #
> @@ -3973,6 +3985,7 @@
>        'blkdebug':   'BlockdevOptionsBlkdebug',
>        'blklogwrites':'BlockdevOptionsBlklogwrites',
>        'blkverify':  'BlockdevOptionsBlkverify',
> +      'blkreplay':  'BlockdevOptionsBlkreplay',
>        'bochs':      'BlockdevOptionsGenericFormat',
>        'cloop':      'BlockdevOptionsGenericFormat',
>        'copy-on-read':'BlockdevOptionsGenericFormat',

Otherwise, this looks good to me. Can you turn it into a proper patch?

> > As soft freeze is coming closer, I'm considering taking this series as
> > it is (it's wrong in parts, but the old state is probably even more
> > wrong) and letting you fix up these checks on top. What do you think?
> 
> That sounds reasonable.

Okay, then that's what I will do.

Kevin

RE: [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Am 11.10.2019 um 08:10 hat Pavel Dovgalyuk geschrieben:
> > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > Am 25.09.2019 um 11:02 hat Pavel Dovgalyuk geschrieben:
> > > > I started playing with -blockdev: added new blockdev for blkreplay and
> > > > constructed the following command line:
> > > >
> > > > -blockdev driver=file,filename=disk.img,node-name=hd0
> > > > -blockdev driver=blkreplay,file=hd0,node-name=hd0-rr
> > > > -device virtio-blk-device,drive=hd0-rr
> > > >
> > > > However, I get an error: "Could not open 'disk.img': Permission denied"
> > > > Everything works when I use this file in '-drive' parameter.
> > > > What am I doing wrong?
> > >
> > > The reason why I didn't reply immediately is because I don't see
> > > anything wrong in the options you used.
> > >
> > > Just to confirm, do you still get the same error when you use only the
> > > first -blockdev option and no other options at all?
> >
> > Ok, I tried again and got different error, which was caused by incorrect
> > QAPI schema for blkreplay.
> > Now it seems ok, but I still can't boot.
> 
> Hm... Are you actually using a raw image? If not, you need the format
> driver, too, and would end up with something like:
> 
>  -blockdev driver=file,filename=disk.qcow2,node-name=hd0
>  -blockdev driver=qcow2,file=hd0,node-name=hd0-qcow2
>  -blockdev driver=blkreplay,file=hd0-qcow2,node-name=hd0-rr
>  -device virtio-blk-device,drive=hd0-rr
> 
> (The first two can be combined into a single option by using a syntax
> like file.driver=file,file.filename=disk.qcow2, but defining each node
> separately is a bit cleaner.)

Ok, this works.
Now I'm trying to check root of the nodes in blk_insert_bs.
This command line leads to 2 invocations of this function:
1. bs->drv is file
2. bs->drv is blkreplay

How then can we check "snapshot" node attachment?

Pavel Dovgalyuk

Re: [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Kevin Wolf]

Am 18.10.2019 um 14:06 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Am 11.10.2019 um 08:10 hat Pavel Dovgalyuk geschrieben:
> > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > Am 25.09.2019 um 11:02 hat Pavel Dovgalyuk geschrieben:
> > > > > I started playing with -blockdev: added new blockdev for blkreplay and
> > > > > constructed the following command line:
> > > > >
> > > > > -blockdev driver=file,filename=disk.img,node-name=hd0
> > > > > -blockdev driver=blkreplay,file=hd0,node-name=hd0-rr
> > > > > -device virtio-blk-device,drive=hd0-rr
> > > > >
> > > > > However, I get an error: "Could not open 'disk.img': Permission denied"
> > > > > Everything works when I use this file in '-drive' parameter.
> > > > > What am I doing wrong?
> > > >
> > > > The reason why I didn't reply immediately is because I don't see
> > > > anything wrong in the options you used.
> > > >
> > > > Just to confirm, do you still get the same error when you use only the
> > > > first -blockdev option and no other options at all?
> > >
> > > Ok, I tried again and got different error, which was caused by incorrect
> > > QAPI schema for blkreplay.
> > > Now it seems ok, but I still can't boot.
> > 
> > Hm... Are you actually using a raw image? If not, you need the format
> > driver, too, and would end up with something like:
> > 
> >  -blockdev driver=file,filename=disk.qcow2,node-name=hd0
> >  -blockdev driver=qcow2,file=hd0,node-name=hd0-qcow2
> >  -blockdev driver=blkreplay,file=hd0-qcow2,node-name=hd0-rr
> >  -device virtio-blk-device,drive=hd0-rr
> > 
> > (The first two can be combined into a single option by using a syntax
> > like file.driver=file,file.filename=disk.qcow2, but defining each node
> > separately is a bit cleaner.)
> 
> Ok, this works.
> Now I'm trying to check root of the nodes in blk_insert_bs.
> This command line leads to 2 invocations of this function:
> 1. bs->drv is file
> 2. bs->drv is blkreplay
> 
> How then can we check "snapshot" node attachment?

Ah, I see what's going on there... :-/

bdrv_open_inherit() creates an internal temporary BlockBackend for image
format probing. And it creates that BlockBackend even when we don't need
to do image probing because we already explicitly selected a driver. It
would be better if it didn't do that.

Kevin

[Qemu-devel] [for-4.2 PATCH 4/6] replay: don't drain/flush bdrv queue while RR is working

[Pavel Dovgalyuk]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

In record/replay mode bdrv queue is controlled by replay mechanism.
It does not allow saving or loading the snapshots
when bdrv queue is not empty. Stopping the VM is not blocked by nonempty
queue, but flushing the queue is still impossible there,
because it may cause deadlocks in replay mode.
This patch disables bdrv_drain_all and bdrv_flush_all in
record/replay mode.

Stopping the machine when the IO requests are not finished is needed
for the debugging. E.g., breakpoint may be set at the specified step,
and forcing the IO requests to finish may break the determinism
of the execution.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
Acked-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c |   28 ++++++++++++++++++++++++++++
 cpus.c     |    2 --
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index f8c3596131..fdb586cb72 100644
--- a/block/io.c
+++ b/block/io.c
@@ -33,6 +33,7 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
+#include "sysemu/replay.h"
 
 #define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */
 
@@ -600,6 +601,15 @@ void bdrv_drain_all_begin(void)
         return;
     }
 
+    /*
+     * bdrv queue is managed by record/replay,
+     * waiting for finishing the I/O requests may
+     * be infinite
+     */
+    if (replay_events_enabled()) {
+        return;
+    }
+
     /* AIO_WAIT_WHILE() with a NULL context can only be called from the main
      * loop AioContext, so make sure we're in the main context. */
     assert(qemu_get_current_aio_context() == qemu_get_aio_context());
@@ -629,6 +639,15 @@ void bdrv_drain_all_end(void)
     BlockDriverState *bs = NULL;
     int drained_end_counter = 0;
 
+    /*
+     * bdrv queue is managed by record/replay,
+     * waiting for finishing the I/O requests may
+     * be endless
+     */
+    if (replay_events_enabled()) {
+        return;
+    }
+
     while ((bs = bdrv_next_all_states(bs))) {
         AioContext *aio_context = bdrv_get_aio_context(bs);
 
@@ -2111,6 +2130,15 @@ int bdrv_flush_all(void)
     BlockDriverState *bs = NULL;
     int result = 0;
 
+    /*
+     * bdrv queue is managed by record/replay,
+     * creating new flush request for stopping
+     * the VM may break the determinism
+     */
+    if (replay_events_enabled()) {
+        return result;
+    }
+
     for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
         AioContext *aio_context = bdrv_get_aio_context(bs);
         int ret;
diff --git a/cpus.c b/cpus.c
index 85cd451a86..eb1c65ebc8 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1086,7 +1086,6 @@ static int do_vm_stop(RunState state, bool send_stop)
     }
 
     bdrv_drain_all();
-    replay_disable_events();
     ret = bdrv_flush_all();
 
     return ret;
@@ -2172,7 +2171,6 @@ int vm_prepare_start(void)
     /* We are sending this now, but the CPUs will be resumed shortly later */
     qapi_event_send_resume();
 
-    replay_enable_events();
     cpu_enable_ticks();
     runstate_set(RUN_STATE_RUNNING);
     vm_state_notify(1, RUN_STATE_RUNNING);

[Qemu-devel] [for-4.2 PATCH 5/6] replay: finish record/replay before closing the disks

[Pavel Dovgalyuk]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

After recent updates block devices cannot be closed on qemu exit.
This happens due to the block request polling when replay is not finished.
Therefore now we stop execution recording before closing the block devices.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 replay/replay.c |    2 ++
 vl.c            |    1 +
 2 files changed, 3 insertions(+)

diff --git a/replay/replay.c b/replay/replay.c
index 713395b33d..5cc25bd2f8 100644
--- a/replay/replay.c
+++ b/replay/replay.c
@@ -385,6 +385,8 @@ void replay_finish(void)
     g_free(replay_snapshot);
     replay_snapshot = NULL;
 
+    replay_mode = REPLAY_MODE_NONE;
+
     replay_finish_events();
 }
 
diff --git a/vl.c b/vl.c
index 0241446df9..bf9f7ddec4 100644
--- a/vl.c
+++ b/vl.c
@@ -4514,6 +4514,7 @@ int main(int argc, char **argv, char **envp)
 
     /* No more vcpu or device emulation activity beyond this point */
     vm_shutdown();
+    replay_finish();
 
     job_cancel_sync_all();
     bdrv_close_all();

[Qemu-devel] [for-4.2 PATCH 6/6] replay: add BH oneshot event for block layer

[Pavel Dovgalyuk]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

Replay is capable of recording normal BH events, but sometimes
there are single use callbacks scheduled with aio_bh_schedule_oneshot
function. This patch enables recording and replaying such callbacks.
Block layer uses these events for calling the completion function.
Replaying these calls makes the execution deterministic.

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
Acked-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c    |    9 ++++++---
 block/io.c               |    4 ++--
 block/iscsi.c            |    5 +++--
 block/nfs.c              |    6 ++++--
 block/null.c             |    4 +++-
 block/nvme.c             |    6 ++++--
 block/rbd.c              |    5 +++--
 block/vxhs.c             |    5 +++--
 include/sysemu/replay.h  |    4 ++++
 replay/replay-events.c   |   16 ++++++++++++++++
 replay/replay-internal.h |    1 +
 stubs/Makefile.objs      |    1 +
 stubs/replay-user.c      |    9 +++++++++
 13 files changed, 59 insertions(+), 16 deletions(-)
 create mode 100644 stubs/replay-user.c

diff --git a/block/block-backend.c b/block/block-backend.c
index 1c605d5444..eb22ff306e 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -18,6 +18,8 @@
 #include "hw/qdev-core.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/runstate.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/replay.h"
 #include "qapi/error.h"
 #include "qapi/qapi-events-block.h"
 #include "qemu/id.h"
@@ -1306,7 +1308,8 @@ BlockAIOCB *blk_abort_aio_request(BlockBackend *blk,
     acb->blk = blk;
     acb->ret = ret;
 
-    aio_bh_schedule_oneshot(blk_get_aio_context(blk), error_callback_bh, acb);
+    replay_bh_schedule_oneshot_event(blk_get_aio_context(blk),
+                                     error_callback_bh, acb);
     return &acb->common;
 }
 
@@ -1362,8 +1365,8 @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset, int bytes,
 
     acb->has_returned = true;
     if (acb->rwco.ret != NOT_DONE) {
-        aio_bh_schedule_oneshot(blk_get_aio_context(blk),
-                                blk_aio_complete_bh, acb);
+        replay_bh_schedule_oneshot_event(blk_get_aio_context(blk),
+                                         blk_aio_complete_bh, acb);
     }
 
     return &acb->common;
diff --git a/block/io.c b/block/io.c
index fdb586cb72..2536dba0b4 100644
--- a/block/io.c
+++ b/block/io.c
@@ -369,8 +369,8 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
     if (bs) {
         bdrv_inc_in_flight(bs);
     }
-    aio_bh_schedule_oneshot(bdrv_get_aio_context(bs),
-                            bdrv_co_drain_bh_cb, &data);
+    replay_bh_schedule_oneshot_event(bdrv_get_aio_context(bs),
+                                     bdrv_co_drain_bh_cb, &data);
 
     qemu_coroutine_yield();
     /* If we are resumed from some other event (such as an aio completion or a
diff --git a/block/iscsi.c b/block/iscsi.c
index 506bf5f875..2ced15066a 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -40,6 +40,7 @@
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "qemu/uuid.h"
+#include "sysemu/replay.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-misc.h"
 #include "qapi/qmp/qdict.h"
@@ -280,8 +281,8 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
     }
 
     if (iTask->co) {
-        aio_bh_schedule_oneshot(iTask->iscsilun->aio_context,
-                                 iscsi_co_generic_bh_cb, iTask);
+        replay_bh_schedule_oneshot_event(iTask->iscsilun->aio_context,
+                                         iscsi_co_generic_bh_cb, iTask);
     } else {
         iTask->complete = 1;
     }
diff --git a/block/nfs.c b/block/nfs.c
index f39acfdb28..40f23495a0 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -37,6 +37,8 @@
 #include "qemu/option.h"
 #include "qemu/uri.h"
 #include "qemu/cutils.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/replay.h"
 #include "qapi/qapi-visit-block-core.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qstring.h"
@@ -257,8 +259,8 @@ nfs_co_generic_cb(int ret, struct nfs_context *nfs, void *data,
     if (task->ret < 0) {
         error_report("NFS Error: %s", nfs_get_error(nfs));
     }
-    aio_bh_schedule_oneshot(task->client->aio_context,
-                            nfs_co_generic_bh_cb, task);
+    replay_bh_schedule_oneshot_event(task->client->aio_context,
+                                     nfs_co_generic_bh_cb, task);
 }
 
 static int coroutine_fn nfs_co_preadv(BlockDriverState *bs, uint64_t offset,
diff --git a/block/null.c b/block/null.c
index 699aa295cb..15e1d56746 100644
--- a/block/null.c
+++ b/block/null.c
@@ -17,6 +17,7 @@
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "block/block_int.h"
+#include "sysemu/replay.h"
 
 #define NULL_OPT_LATENCY "latency-ns"
 #define NULL_OPT_ZEROES  "read-zeroes"
@@ -179,7 +180,8 @@ static inline BlockAIOCB *null_aio_common(BlockDriverState *bs,
         timer_mod_ns(&acb->timer,
                      qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + s->latency_ns);
     } else {
-        aio_bh_schedule_oneshot(bdrv_get_aio_context(bs), null_bh_cb, acb);
+        replay_bh_schedule_oneshot_event(bdrv_get_aio_context(bs),
+                                         null_bh_cb, acb);
     }
     return &acb->common;
 }
diff --git a/block/nvme.c b/block/nvme.c
index 5be3a39b63..910872ec59 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -23,6 +23,7 @@
 #include "qemu/option.h"
 #include "qemu/vfio-helpers.h"
 #include "block/block_int.h"
+#include "sysemu/replay.h"
 #include "trace.h"
 
 #include "block/nvme.h"
@@ -351,7 +352,8 @@ static bool nvme_process_completion(BDRVNVMeState *s, NVMeQueuePair *q)
         smp_mb_release();
         *q->cq.doorbell = cpu_to_le32(q->cq.head);
         if (!qemu_co_queue_empty(&q->free_req_queue)) {
-            aio_bh_schedule_oneshot(s->aio_context, nvme_free_req_queue_cb, q);
+            replay_bh_schedule_oneshot_event(s->aio_context,
+                                             nvme_free_req_queue_cb, q);
         }
     }
     q->busy = false;
@@ -935,7 +937,7 @@ static void nvme_rw_cb(void *opaque, int ret)
         /* The rw coroutine hasn't yielded, don't try to enter. */
         return;
     }
-    aio_bh_schedule_oneshot(data->ctx, nvme_rw_cb_bh, data);
+    replay_bh_schedule_oneshot_event(data->ctx, nvme_rw_cb_bh, data);
 }
 
 static coroutine_fn int nvme_co_prw_aligned(BlockDriverState *bs,
diff --git a/block/rbd.c b/block/rbd.c
index 057af43d48..c71e45d7c3 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -22,6 +22,7 @@
 #include "block/qdict.h"
 #include "crypto/secret.h"
 #include "qemu/cutils.h"
+#include "sysemu/replay.h"
 #include "qapi/qmp/qstring.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qjson.h"
@@ -884,8 +885,8 @@ static void rbd_finish_aiocb(rbd_completion_t c, RADOSCB *rcb)
     rcb->ret = rbd_aio_get_return_value(c);
     rbd_aio_release(c);
 
-    aio_bh_schedule_oneshot(bdrv_get_aio_context(acb->common.bs),
-                            rbd_finish_bh, rcb);
+    replay_bh_schedule_oneshot_event(bdrv_get_aio_context(acb->common.bs),
+                                     rbd_finish_bh, rcb);
 }
 
 static int rbd_aio_discard_wrapper(rbd_image_t image,
diff --git a/block/vxhs.c b/block/vxhs.c
index 77fd5eb20d..d79fc97df6 100644
--- a/block/vxhs.c
+++ b/block/vxhs.c
@@ -22,6 +22,7 @@
 #include "qapi/error.h"
 #include "qemu/uuid.h"
 #include "crypto/tlscredsx509.h"
+#include "sysemu/replay.h"
 
 #define VXHS_OPT_FILENAME           "filename"
 #define VXHS_OPT_VDISK_ID           "vdisk-id"
@@ -105,8 +106,8 @@ static void vxhs_iio_callback(void *ctx, uint32_t opcode, uint32_t error)
             trace_vxhs_iio_callback(error);
         }
 
-        aio_bh_schedule_oneshot(bdrv_get_aio_context(acb->common.bs),
-                                vxhs_complete_aio_bh, acb);
+        replay_bh_schedule_oneshot_event(bdrv_get_aio_context(acb->common.bs),
+                                         vxhs_complete_aio_bh, acb);
         break;
 
     default:
diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
index dfc7a31c66..8df517298c 100644
--- a/include/sysemu/replay.h
+++ b/include/sysemu/replay.h
@@ -15,6 +15,7 @@
 #include "qapi/qapi-types-misc.h"
 #include "qapi/qapi-types-run-state.h"
 #include "qapi/qapi-types-ui.h"
+#include "block/aio.h"
 
 /* replay clock kinds */
 enum ReplayClockKind {
@@ -140,6 +141,9 @@ void replay_enable_events(void);
 bool replay_events_enabled(void);
 /*! Adds bottom half event to the queue */
 void replay_bh_schedule_event(QEMUBH *bh);
+/* Adds oneshot bottom half event to the queue */
+void replay_bh_schedule_oneshot_event(AioContext *ctx,
+    QEMUBHFunc *cb, void *opaque);
 /*! Adds input event to the queue */
 void replay_input_event(QemuConsole *src, InputEvent *evt);
 /*! Adds input sync event to the queue */
diff --git a/replay/replay-events.c b/replay/replay-events.c
index 008e80f636..302b84043a 100644
--- a/replay/replay-events.c
+++ b/replay/replay-events.c
@@ -36,6 +36,9 @@ static void replay_run_event(Event *event)
     case REPLAY_ASYNC_EVENT_BH:
         aio_bh_call(event->opaque);
         break;
+    case REPLAY_ASYNC_EVENT_BH_ONESHOT:
+        ((QEMUBHFunc *)event->opaque)(event->opaque2);
+        break;
     case REPLAY_ASYNC_EVENT_INPUT:
         qemu_input_event_send_impl(NULL, (InputEvent *)event->opaque);
         qapi_free_InputEvent((InputEvent *)event->opaque);
@@ -131,6 +134,17 @@ void replay_bh_schedule_event(QEMUBH *bh)
     }
 }
 
+void replay_bh_schedule_oneshot_event(AioContext *ctx,
+    QEMUBHFunc *cb, void *opaque)
+{
+    if (events_enabled) {
+        uint64_t id = replay_get_current_icount();
+        replay_add_event(REPLAY_ASYNC_EVENT_BH_ONESHOT, cb, opaque, id);
+    } else {
+        aio_bh_schedule_oneshot(ctx, cb, opaque);
+    }
+}
+
 void replay_add_input_event(struct InputEvent *event)
 {
     replay_add_event(REPLAY_ASYNC_EVENT_INPUT, event, NULL, 0);
@@ -161,6 +175,7 @@ static void replay_save_event(Event *event, int checkpoint)
         /* save event-specific data */
         switch (event->event_kind) {
         case REPLAY_ASYNC_EVENT_BH:
+        case REPLAY_ASYNC_EVENT_BH_ONESHOT:
             replay_put_qword(event->id);
             break;
         case REPLAY_ASYNC_EVENT_INPUT:
@@ -216,6 +231,7 @@ static Event *replay_read_event(int checkpoint)
     /* Events that has not to be in the queue */
     switch (replay_state.read_event_kind) {
     case REPLAY_ASYNC_EVENT_BH:
+    case REPLAY_ASYNC_EVENT_BH_ONESHOT:
         if (replay_state.read_event_id == -1) {
             replay_state.read_event_id = replay_get_qword();
         }
diff --git a/replay/replay-internal.h b/replay/replay-internal.h
index afba9a3e0c..55fca1ac6b 100644
--- a/replay/replay-internal.h
+++ b/replay/replay-internal.h
@@ -51,6 +51,7 @@ enum ReplayEvents {
 
 enum ReplayAsyncEventKind {
     REPLAY_ASYNC_EVENT_BH,
+    REPLAY_ASYNC_EVENT_BH_ONESHOT,
     REPLAY_ASYNC_EVENT_INPUT,
     REPLAY_ASYNC_EVENT_INPUT_SYNC,
     REPLAY_ASYNC_EVENT_CHAR_READ,
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 9c7393b08c..4a50e95ec3 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -20,6 +20,7 @@ stub-obj-y += monitor.o
 stub-obj-y += notify-event.o
 stub-obj-y += qtest.o
 stub-obj-y += replay.o
+stub-obj-y += replay-user.o
 stub-obj-y += runstate-check.o
 stub-obj-y += set-fd-handler.o
 stub-obj-y += sysbus.o
diff --git a/stubs/replay-user.c b/stubs/replay-user.c
new file mode 100644
index 0000000000..2ad9e27203
--- /dev/null
+++ b/stubs/replay-user.c
@@ -0,0 +1,9 @@
+#include "qemu/osdep.h"
+#include "sysemu/replay.h"
+#include "sysemu/sysemu.h"
+
+void replay_bh_schedule_oneshot_event(AioContext *ctx,
+    QEMUBHFunc *cb, void *opaque)
+{
+    aio_bh_schedule_oneshot(ctx, cb, opaque);
+}

Re: [Qemu-devel] [for-4.2 PATCH 0/6] Block-related record/replay fixes

[no-reply]

Patchew URL: https://patchew.org/QEMU/156872146565.1757.3033215873677512474.stgit@pasha-Precision-3630-Tower/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

./tests/docker/docker.py --engine auto build qemu:fedora tests/docker/dockerfiles/fedora.docker   --add-current-user  
Image is up to date.
  LD      docker-test-debug@fedora.mo
cc: fatal error: no input files
compilation terminated.
make: *** [docker-test-debug@fedora.mo] Error 4



The full log is available at
http://patchew.org/logs/156872146565.1757.3033215873677512474.stgit@pasha-Precision-3630-Tower/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [for-4.2 PATCH 0/6] Block-related record/replay fixes

[no-reply]

Patchew URL: https://patchew.org/QEMU/156872146565.1757.3033215873677512474.stgit@pasha-Precision-3630-Tower/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

./tests/docker/docker.py --engine auto build qemu:fedora tests/docker/dockerfiles/fedora.docker   --add-current-user  
Image is up to date.
  LD      docker-test-mingw@fedora.mo
cc: fatal error: no input files
compilation terminated.
make: *** [docker-test-mingw@fedora.mo] Error 4



The full log is available at
http://patchew.org/logs/156872146565.1757.3033215873677512474.stgit@pasha-Precision-3630-Tower/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [for-4.2 PATCH 0/6] Block-related record/replay fixes

[Alex Bennée]

Pavel Dovgalyuk <pavel.dovgaluk@gmail.com> writes:

> The set of patches include the block-related updates
> of the record/replay icount feature:
>  - application of 'snapshot' option on the file layer instead of
>    the top one: command line and documentation fix
>  - implementation of bdrv_snapshot_goto for blkreplay driver
>  - start/stop fix in replay mode with attached block devices
>  - record/replay of bh oneshot events, used in the block layer

Can we come up with a reasonable smoke test to verify record/replay is
functional? AIUI we don't need a full OS but we do need a block device
to store the replay data. Could we use one of the simple system tests
like "memory" and run that through a record and replay cycle?

If we can defend the basic functionally in "make check" then breakage is
less likely to slip through and you'll have less bisecting to do.

>
> ---
>
> Pavel Dovgaluk (6):
>       block: implement bdrv_snapshot_goto for blkreplay
>       replay: disable default snapshot for record/replay
>       replay: update docs for record/replay with block devices
>       replay: don't drain/flush bdrv queue while RR is working
>       replay: finish record/replay before closing the disks
>       replay: add BH oneshot event for block layer
>
>
>  block/blkreplay.c        |    8 ++++++++
>  block/block-backend.c    |    9 ++++++---
>  block/io.c               |   32 ++++++++++++++++++++++++++++++--
>  block/iscsi.c            |    5 +++--
>  block/nfs.c              |    6 ++++--
>  block/null.c             |    4 +++-
>  block/nvme.c             |    6 ++++--
>  block/rbd.c              |    5 +++--
>  block/vxhs.c             |    5 +++--
>  cpus.c                   |    2 --
>  docs/replay.txt          |   12 +++++++++---
>  include/sysemu/replay.h  |    4 ++++
>  replay/replay-events.c   |   16 ++++++++++++++++
>  replay/replay-internal.h |    1 +
>  replay/replay.c          |    2 ++
>  stubs/Makefile.objs      |    1 +
>  stubs/replay-user.c      |    9 +++++++++
>  vl.c                     |   11 +++++++++--
>  18 files changed, 115 insertions(+), 23 deletions(-)
>  create mode 100644 stubs/replay-user.c


--
Alex Bennée

Re: [Qemu-devel] [for-4.2 PATCH 0/6] Block-related record/replay fixes

[Pavel Dovgalyuk]

> -----Original Message-----
> From: Alex Bennée [mailto:alex.bennee@linaro.org]
> Sent: Tuesday, September 17, 2019 10:02 PM
> To: Pavel Dovgalyuk
> Cc: qemu-devel@nongnu.org; kwolf@redhat.com; peter.maydell@linaro.org;
> crosthwaite.peter@gmail.com; boost.lists@gmail.com; artem.k.pisarenko@gmail.com;
> quintela@redhat.com; ciro.santilli@gmail.com; jasowang@redhat.com; mst@redhat.com;
> armbru@redhat.com; mreitz@redhat.com; maria.klimushenkova@ispras.ru; dovgaluk@ispras.ru;
> kraxel@redhat.com; pavel.dovgaluk@ispras.ru; thomas.dullien@googlemail.com;
> pbonzini@redhat.com; dgilbert@redhat.com; rth@twiddle.net
> Subject: Re: [for-4.2 PATCH 0/6] Block-related record/replay fixes
> 
> 
> Pavel Dovgalyuk <pavel.dovgaluk@gmail.com> writes:
> 
> > The set of patches include the block-related updates
> > of the record/replay icount feature:
> >  - application of 'snapshot' option on the file layer instead of
> >    the top one: command line and documentation fix
> >  - implementation of bdrv_snapshot_goto for blkreplay driver
> >  - start/stop fix in replay mode with attached block devices
> >  - record/replay of bh oneshot events, used in the block layer
> 
> Can we come up with a reasonable smoke test to verify record/replay is
> functional? AIUI we don't need a full OS but we do need a block device
> to store the replay data. Could we use one of the simple system tests
> like "memory" and run that through a record and replay cycle?

That's would be great.
I'm not familiar with testing framework and couldn't find "memory" test that you refer to.
Replay test must at least the following:
1. record some execution
2. replay that execution

And there could be several endings:
1. record stalled
2. record crashed
3. replay stalled
4. replay crashed
5. all executions finished successfully

Pavel Dovgalyuk

> 
> >
> > ---
> >
> > Pavel Dovgaluk (6):
> >       block: implement bdrv_snapshot_goto for blkreplay
> >       replay: disable default snapshot for record/replay
> >       replay: update docs for record/replay with block devices
> >       replay: don't drain/flush bdrv queue while RR is working
> >       replay: finish record/replay before closing the disks
> >       replay: add BH oneshot event for block layer
> >
> >
> >  block/blkreplay.c        |    8 ++++++++
> >  block/block-backend.c    |    9 ++++++---
> >  block/io.c               |   32 ++++++++++++++++++++++++++++++--
> >  block/iscsi.c            |    5 +++--
> >  block/nfs.c              |    6 ++++--
> >  block/null.c             |    4 +++-
> >  block/nvme.c             |    6 ++++--
> >  block/rbd.c              |    5 +++--
> >  block/vxhs.c             |    5 +++--
> >  cpus.c                   |    2 --
> >  docs/replay.txt          |   12 +++++++++---
> >  include/sysemu/replay.h  |    4 ++++
> >  replay/replay-events.c   |   16 ++++++++++++++++
> >  replay/replay-internal.h |    1 +
> >  replay/replay.c          |    2 ++
> >  stubs/Makefile.objs      |    1 +
> >  stubs/replay-user.c      |    9 +++++++++
> >  vl.c                     |   11 +++++++++--
> >  18 files changed, 115 insertions(+), 23 deletions(-)
> >  create mode 100644 stubs/replay-user.c
> 
> 
> --
> Alex Bennée

Re: [Qemu-devel] [for-4.2 PATCH 0/6] Block-related record/replay fixes

[Alex Bennée]

Pavel Dovgalyuk <dovgaluk@ispras.ru> writes:

>> -----Original Message-----
>> From: Alex Bennée [mailto:alex.bennee@linaro.org]
>> Sent: Tuesday, September 17, 2019 10:02 PM
>> To: Pavel Dovgalyuk
>> Cc: qemu-devel@nongnu.org; kwolf@redhat.com; peter.maydell@linaro.org;
>> crosthwaite.peter@gmail.com; boost.lists@gmail.com; artem.k.pisarenko@gmail.com;
>> quintela@redhat.com; ciro.santilli@gmail.com; jasowang@redhat.com; mst@redhat.com;
>> armbru@redhat.com; mreitz@redhat.com; maria.klimushenkova@ispras.ru; dovgaluk@ispras.ru;
>> kraxel@redhat.com; pavel.dovgaluk@ispras.ru; thomas.dullien@googlemail.com;
>> pbonzini@redhat.com; dgilbert@redhat.com; rth@twiddle.net
>> Subject: Re: [for-4.2 PATCH 0/6] Block-related record/replay fixes
>>
>>
>> Pavel Dovgalyuk <pavel.dovgaluk@gmail.com> writes:
>>
>> > The set of patches include the block-related updates
>> > of the record/replay icount feature:
>> >  - application of 'snapshot' option on the file layer instead of
>> >    the top one: command line and documentation fix
>> >  - implementation of bdrv_snapshot_goto for blkreplay driver
>> >  - start/stop fix in replay mode with attached block devices
>> >  - record/replay of bh oneshot events, used in the block layer
>>
>> Can we come up with a reasonable smoke test to verify record/replay is
>> functional? AIUI we don't need a full OS but we do need a block device
>> to store the replay data. Could we use one of the simple system tests
>> like "memory" and run that through a record and replay cycle?
>
> That's would be great.
> I'm not familiar with testing framework and couldn't find "memory"
> test that you refer to.

The memory test code is in tests/tcg/multiarch/system and gets combined
with the boot code for whichever target can build it. For example on
aarch64 it is run like:

  timeout 15  $BLD/aarch64-softmmu/qemu-system-aarch64 -monitor none -display none -chardev file,path=memory.out,id=output  -M virt -cpu max -display none -semihosting-config enable=on,target=native,chardev=output -kernel memory

The test binaries will be in $BLD/tests/tcg/aarch64-softmmu and are
built when you run "make check-tcg" and have either the appropriate
cross compilers installed on your system or docker enabled and setup
(see docs/devel/testing.rst).

> Replay test must at least the following:
> 1. record some execution
> 2. replay that execution
>
> And there could be several endings:
> 1. record stalled
> 2. record crashed
> 3. replay stalled
> 4. replay crashed
> 5. all executions finished successfully

If you can provide an appropriate set of invocations I can plumb them
into the Makefiles for you.

>
> Pavel Dovgalyuk
>
>>
>> >
>> > ---
>> >
>> > Pavel Dovgaluk (6):
>> >       block: implement bdrv_snapshot_goto for blkreplay
>> >       replay: disable default snapshot for record/replay
>> >       replay: update docs for record/replay with block devices
>> >       replay: don't drain/flush bdrv queue while RR is working
>> >       replay: finish record/replay before closing the disks
>> >       replay: add BH oneshot event for block layer
>> >
>> >
>> >  block/blkreplay.c        |    8 ++++++++
>> >  block/block-backend.c    |    9 ++++++---
>> >  block/io.c               |   32 ++++++++++++++++++++++++++++++--
>> >  block/iscsi.c            |    5 +++--
>> >  block/nfs.c              |    6 ++++--
>> >  block/null.c             |    4 +++-
>> >  block/nvme.c             |    6 ++++--
>> >  block/rbd.c              |    5 +++--
>> >  block/vxhs.c             |    5 +++--
>> >  cpus.c                   |    2 --
>> >  docs/replay.txt          |   12 +++++++++---
>> >  include/sysemu/replay.h  |    4 ++++
>> >  replay/replay-events.c   |   16 ++++++++++++++++
>> >  replay/replay-internal.h |    1 +
>> >  replay/replay.c          |    2 ++
>> >  stubs/Makefile.objs      |    1 +
>> >  stubs/replay-user.c      |    9 +++++++++
>> >  vl.c                     |   11 +++++++++--
>> >  18 files changed, 115 insertions(+), 23 deletions(-)
>> >  create mode 100644 stubs/replay-user.c
>>
>>
>> --
>> Alex Bennée


--
Alex Bennée

Re: [Qemu-devel] [for-4.2 PATCH 0/6] Block-related record/replay fixes

[Pavel Dovgalyuk]

> From: Alex Bennée [mailto:alex.bennee@linaro.org]
> Pavel Dovgalyuk <dovgaluk@ispras.ru> writes:
> 
> >> -----Original Message-----
> >> From: Alex Bennée [mailto:alex.bennee@linaro.org]
> >> Sent: Tuesday, September 17, 2019 10:02 PM
> >> To: Pavel Dovgalyuk
> >> Cc: qemu-devel@nongnu.org; kwolf@redhat.com; peter.maydell@linaro.org;
> >> crosthwaite.peter@gmail.com; boost.lists@gmail.com; artem.k.pisarenko@gmail.com;
> >> quintela@redhat.com; ciro.santilli@gmail.com; jasowang@redhat.com; mst@redhat.com;
> >> armbru@redhat.com; mreitz@redhat.com; maria.klimushenkova@ispras.ru; dovgaluk@ispras.ru;
> >> kraxel@redhat.com; pavel.dovgaluk@ispras.ru; thomas.dullien@googlemail.com;
> >> pbonzini@redhat.com; dgilbert@redhat.com; rth@twiddle.net
> >> Subject: Re: [for-4.2 PATCH 0/6] Block-related record/replay fixes
> >>
> >>
> >> Pavel Dovgalyuk <pavel.dovgaluk@gmail.com> writes:
> >>
> >> > The set of patches include the block-related updates
> >> > of the record/replay icount feature:
> >> >  - application of 'snapshot' option on the file layer instead of
> >> >    the top one: command line and documentation fix
> >> >  - implementation of bdrv_snapshot_goto for blkreplay driver
> >> >  - start/stop fix in replay mode with attached block devices
> >> >  - record/replay of bh oneshot events, used in the block layer
> >>
> >> Can we come up with a reasonable smoke test to verify record/replay is
> >> functional? AIUI we don't need a full OS but we do need a block device
> >> to store the replay data. Could we use one of the simple system tests
> >> like "memory" and run that through a record and replay cycle?
> >
> > That's would be great.
> > I'm not familiar with testing framework and couldn't find "memory"
> > test that you refer to.
> 
> The memory test code is in tests/tcg/multiarch/system and gets combined
> with the boot code for whichever target can build it. For example on
> aarch64 it is run like:
> 
>   timeout 15  $BLD/aarch64-softmmu/qemu-system-aarch64 -monitor none -display none -chardev
> file,path=memory.out,id=output  -M virt -cpu max -display none -semihosting-config
> enable=on,target=native,chardev=output -kernel memory

Yes, rr testing could be that simple, but in full system emulation mode.
Simplest tests may be run even without the block devices:

qemu-system-aarch64 -monitor none -display none -chardev file,path=memory.out,id=output
-M virt -cpu max -kernel memory -net none -icount shift=5,rr=record,rrfile=record.bin

Better testing must include some block device like Linux boot image or something similar.

> The test binaries will be in $BLD/tests/tcg/aarch64-softmmu and are
> built when you run "make check-tcg" and have either the appropriate
> cross compilers installed on your system or docker enabled and setup
> (see docs/devel/testing.rst).
> 
> > Replay test must at least the following:
> > 1. record some execution
> > 2. replay that execution
> >
> > And there could be several endings:
> > 1. record stalled
> > 2. record crashed
> > 3. replay stalled
> > 4. replay crashed
> > 5. all executions finished successfully
> 
> If you can provide an appropriate set of invocations I can plumb them
> into the Makefiles for you.

That will be great, thank you.

Pavel Dovgalyuk

Re: [for-4.2 PATCH 0/6] Block-related record/replay fixes

[Kevin Wolf]

Am 17.09.2019 um 13:57 hat Pavel Dovgalyuk geschrieben:
> The set of patches include the block-related updates
> of the record/replay icount feature:
>  - application of 'snapshot' option on the file layer instead of
>    the top one: command line and documentation fix
>  - implementation of bdrv_snapshot_goto for blkreplay driver
>  - start/stop fix in replay mode with attached block devices
>  - record/replay of bh oneshot events, used in the block layer

Thanks, applied to the block branch.

Kevin

[Qemu-devel] [for-4.2 PATCH 0/6] Block-related record/replay fixes

[Pavel Dovgalyuk]

The set of patches include the block-related updates
of the record/replay icount feature:
 - application of 'snapshot' option on the file layer instead of
   the top one: command line and documentation fix
 - implementation of bdrv_snapshot_goto for blkreplay driver
 - start/stop fix in replay mode with attached block devices
 - record/replay of bh oneshot events, used in the block layer

These patches are rebased upon the following series:
https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg05568.html

---

Pavel Dovgalyuk (6):
      block: implement bdrv_snapshot_goto for blkreplay
      replay: disable default snapshot for record/replay
      replay: update docs for record/replay with block devices
      replay: don't drain/flush bdrv queue while RR is working
      replay: finish record/replay before closing the disks
      replay: add BH oneshot event for block layer


 block/blkreplay.c        |    8 ++++++++
 block/block-backend.c    |    8 +++++---
 block/io.c               |   32 ++++++++++++++++++++++++++++++--
 block/iscsi.c            |    5 +++--
 block/nfs.c              |    5 +++--
 block/null.c             |    4 +++-
 block/nvme.c             |    6 ++++--
 block/rbd.c              |    5 +++--
 block/vxhs.c             |    5 +++--
 cpus.c                   |    2 --
 docs/replay.txt          |   12 +++++++++---
 include/sysemu/replay.h  |    3 +++
 replay/replay-events.c   |   16 ++++++++++++++++
 replay/replay-internal.h |    1 +
 replay/replay.c          |    4 +++-
 stubs/Makefile.objs      |    1 +
 stubs/replay-user.c      |    9 +++++++++
 vl.c                     |   11 +++++++++--
 18 files changed, 113 insertions(+), 24 deletions(-)
 create mode 100644 stubs/replay-user.c

-- 
Pavel Dovgalyuk

[Qemu-devel] [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Pavel Dovgalyuk]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

This patch updates the description of the command lines for using
record/replay with attached block devices.

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
---
 docs/replay.txt |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/docs/replay.txt b/docs/replay.txt
index ee6aee9861..ce97c3f72f 100644
--- a/docs/replay.txt
+++ b/docs/replay.txt
@@ -27,7 +27,7 @@ Usage of the record/replay:
  * First, record the execution with the following command line:
     qemu-system-i386 \
      -icount shift=7,rr=record,rrfile=replay.bin \
-     -drive file=disk.qcow2,if=none,id=img-direct \
+     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
      -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
      -device ide-hd,drive=img-blkreplay \
      -netdev user,id=net1 -device rtl8139,netdev=net1 \
@@ -35,7 +35,7 @@ Usage of the record/replay:
  * After recording, you can replay it by using another command line:
     qemu-system-i386 \
      -icount shift=7,rr=replay,rrfile=replay.bin \
-     -drive file=disk.qcow2,if=none,id=img-direct \
+     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
      -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
      -device ide-hd,drive=img-blkreplay \
      -netdev user,id=net1 -device rtl8139,netdev=net1 \
@@ -223,7 +223,7 @@ Block devices record/replay module intercepts calls of
 bdrv coroutine functions at the top of block drivers stack.
 To record and replay block operations the drive must be configured
 as following:
- -drive file=disk.qcow2,if=none,id=img-direct
+ -drive file=disk.qcow2,if=none,snapshot,id=img-direct
  -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
  -device ide-hd,drive=img-blkreplay
 
@@ -252,6 +252,12 @@ This snapshot is created at start of recording and restored at start
 of replaying. It also can be loaded while replaying to roll back
 the execution.
 
+'snapshot' flag of the disk image must be removed to save the snapshots
+in the overlay (or original image) instead of using the temporary overlay.
+ -drive file=disk.ovl,if=none,id=img-direct
+ -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
+ -device ide-hd,drive=img-blkreplay
+
 Use QEMU monitor to create additional snapshots. 'savevm <name>' command
 created the snapshot and 'loadvm <name>' restores it. To prevent corruption
 of the original disk image, use overlay files linked to the original images.