Re: [Qemu-devel] [PATCH v7 17/19] replay: add BH oneshot event for block layer

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:address@hidden
> Am 10.10.2018 um 15:35 hat Pavel Dovgalyuk geschrieben:
> > 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 <address@hidden>
> 
> What are the rules for using aio_bh_schedule_oneshot() vs.
> replay_bh_schedule_oneshot_event()? You are modifying one place, but if
> I grep for aio_bh_schedule_oneshot(), I find many other places that
> use it. Why is this one place special?

This one is special, because it caused a record/replay bug.
I can't validate all other places, because I don't understand block layer
good enough.

That's why our current strategy is fixing replay, when it is breaks.
It's too hard to create the test for verifying the modification. And for the 
current
patch there was the bug which was fixed.

The rule is the following: when the event is asynchronous and its finalization
affects the guest state, then this event should be processed by the 
record/replay queue.

> 
> > --
> >
> > v6:
> >  - moved stub function to the separate file for fixing linux-user build
> > ---
> >  block/block-backend.c    |    3 ++-
> >  include/sysemu/replay.h  |    3 +++
> >  replay/replay-events.c   |   16 ++++++++++++++++
> >  replay/replay-internal.h |    1 +
> >  replay/replay.c          |    2 +-
> >  stubs/Makefile.objs      |    1 +
> >  stubs/replay-user.c      |    9 +++++++++
> >  7 files changed, 33 insertions(+), 2 deletions(-)
> >  create mode 100644 stubs/replay-user.c
> >
> > diff --git a/block/block-backend.c b/block/block-backend.c
> > index dc0cd57..04f5554 100644
> > --- a/block/block-backend.c
> > +++ b/block/block-backend.c
> > @@ -17,6 +17,7 @@
> >  #include "block/throttle-groups.h"
> >  #include "sysemu/blockdev.h"
> >  #include "sysemu/sysemu.h"
> > +#include "sysemu/replay.h"
> >  #include "qapi/error.h"
> >  #include "qapi/qapi-events-block.h"
> >  #include "qemu/id.h"
> > @@ -1379,7 +1380,7 @@ 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),
> > +        replay_bh_schedule_oneshot_event(blk_get_aio_context(blk),
> >                                  blk_aio_complete_bh, acb);
> 
> Indentation is off.

Thanks.

Pavel Dovgalyuk