Re: [Qemu-devel] [PATCH 2/3] replay: allow replay stopping and restarting

[Paolo Bonzini <pbonzini@redhat.com>]

On 20/06/2016 08:26, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>>> From: "Pavel Dovgalyuk" <pavel.dovgaluk@ispras.ru>
>>> This patch fixes bug with stopping and restarting replay
>>> through monitor.
>>>
>>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
>>> ---
>>>  block/blkreplay.c        |   18 +++++++++++++-----
>>>  cpus.c                   |    1 +
>>>  include/sysemu/replay.h  |    2 ++
>>>  replay/replay-internal.h |    2 --
>>>  vl.c                     |    1 +
>>>  5 files changed, 17 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/block/blkreplay.c b/block/blkreplay.c
>>> index 42f1813..438170c 100644
>>> --- a/block/blkreplay.c
>>> +++ b/block/blkreplay.c
>>> @@ -70,6 +70,14 @@ static void blkreplay_bh_cb(void *opaque)
>>>      g_free(req);
>>>  }
>>>
>>> +static uint64_t blkreplay_next_id(void)
>>> +{
>>> +    if (replay_events_enabled()) {
>>> +        return request_id++;
>>> +    }
>>> +    return 0;
>>> +}
>>
>> What happens if 0 is returned?  
> 
> It could be any value. When replay events are disables,
> it means that either replay is disabled or execution is stopped.
> In first case we won't pass this requests through the replay queue
> and therefore id is useless.
> In stopped mode we have to keep request_id unchanged to make
> record/replay deterministic.
> 
>> I think that you want to call
>> replay_disable_events...
>>
>>>      bdrv_drain_all();
>>
>> ... after this bdrv_drain_all.
> 
> Why? We disable replay events to avoid adding new block requests
> to the queue. How this is related to drain all?

drain all completes the guest's pending requests.  If you disable events
before drain all, doesn't that cause a mismatch between record and replay?

>>
>> I was going to suggest using qemu_add_vm_change_state_handler
>> in replay_start (which could have replaced the existing call
>> to replay_enable_events), but that's not possible if you have
>> to do your calls after bdrv_drain_all.
> 
> Pavel Dovgalyuk
> 
> 
>