| Message ID | 156872148244.1757.2888672921697745021.stgit@pasha-Precision-3630-Tower (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Block-related record/replay fixes | expand |
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
> 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
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
> 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
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
> 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
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
> 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
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);
> 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
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
> 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
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
> 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
> 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
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 >
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
> 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
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
> 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
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
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.