[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 21/25] replay: replay BH for IDE trim operati
From: |
John Snow |
Subject: |
Re: [Qemu-devel] [PATCH v6 21/25] replay: replay BH for IDE trim operation |
Date: |
Mon, 17 Sep 2018 15:33:46 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 |
On 09/17/2018 08:00 AM, Pavel Dovgalyuk wrote:
>> From: John Snow [mailto:address@hidden
>> On 09/14/2018 03:27 AM, Pavel Dovgalyuk wrote:
>>>> From: Pavel Dovgalyuk [mailto:address@hidden
>>>>> From: John Snow [mailto:address@hidden
>>>>> On 09/12/2018 04:19 AM, Pavel Dovgalyuk wrote:
>>>>>> This patch makes IDE trim BH deterministic, because it affects
>>>>>> the device state. Therefore its invocation should be replayed
>>>>>> instead of running at the random moment.
>>>>>>
>>>>>> Signed-off-by: Pavel Dovgalyuk <address@hidden>
>>>>>> Reviewed-by: Paolo Bonzini <address@hidden>
>>>>>> ---
>>>>>> hw/ide/core.c | 3 ++-
>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>>>>> index 2c62efc..04e22e7 100644
>>>>>> --- a/hw/ide/core.c
>>>>>> +++ b/hw/ide/core.c
>>>>>> @@ -35,6 +35,7 @@
>>>>>> #include "sysemu/block-backend.h"
>>>>>> #include "qapi/error.h"
>>>>>> #include "qemu/cutils.h"
>>>>>> +#include "sysemu/replay.h"
>>>>>>
>>>>>> #include "hw/ide/internal.h"
>>>>>> #include "trace.h"
>>>>>> @@ -479,7 +480,7 @@ static void ide_issue_trim_cb(void *opaque, int ret)
>>>>>> done:
>>>>>> iocb->aiocb = NULL;
>>>>>> if (iocb->bh) {
>>>>>> - qemu_bh_schedule(iocb->bh);
>>>>>> + replay_bh_schedule_event(iocb->bh);
>>>>>> }
>>>>>> }
>>>>>>
>>>>> Just passing by: Why do we need to change this call, but nothing else in
>>>>> IDE?
>>>>
>>>> This call is responsible for a bug that was reproducible.
>>>>
>>>>> I don't mind conceptually, but it's odd to me that of all the calls I
>>>>> make in this emulator that change state somewhere that this is the only
>>>>> one you need to hijack for the replay feature.
>>>>>
>>>>> Is this a necessarily complete change?
>>>
>>>
>>> I found one more BH in ide/core:
>>>
>>> static void ide_restart_cb(void *opaque, int running, RunState state)
>>> {
>>> IDEBus *bus = opaque;
>>>
>>> if (!running)
>>> return;
>>>
>>> if (!bus->bh) {
>>> bus->bh = qemu_bh_new(ide_restart_bh, bus);
>>> qemu_bh_schedule(bus->bh);
>>> }
>>> }
>>>
>>> void ide_register_restart_cb(IDEBus *bus)
>>> {
>>> if (bus->dma->ops->restart_dma) {
>>> bus->vmstate = qemu_add_vm_change_state_handler(ide_restart_cb,
>>> bus);
>>> }
>>> }
>>>
>>> As I understand, it is called when VM start/stop event happen.
>>> These events are not related to the guest state.
>>>
>>> Does this BH change the guest state somehow?
>>
>> Shouldn't change guest state all by itself.
>>
>> ide_restart_bh does, though. (Changes device registers, can cause block
>> IO to occur, etc.)
>
> Why is this needed?
> I mean that start/stop should not change the state of the guest.
> Why should we restart IDE controller operations?
>
> Pavel Dovgalyuk
>
This is part of the rerror/werror=stop model where if we hit a host IO
problem we pause the guest instead of reporting the error. When we
re-engage the VM, the IO is re-submitted, which may change guest-visible
data.
BTW, I'm fine with the changes presented so far, I was just trying to
understand them.
Paolo, please go ahead.
--js
- [Qemu-devel] [PATCH v6 19/25] replay: allow loading any snapshots before recording, (continued)
- Re: [Qemu-devel] [PATCH v6 21/25] replay: replay BH for IDE trim operation, John Snow, 2018/09/14
- Re: [Qemu-devel] [PATCH v6 21/25] replay: replay BH for IDE trim operation, Paolo Bonzini, 2018/09/14
- Re: [Qemu-devel] [PATCH v6 21/25] replay: replay BH for IDE trim operation, Peter Maydell, 2018/09/14
- Re: [Qemu-devel] [PATCH v6 21/25] replay: replay BH for IDE trim operation, Pavel Dovgalyuk, 2018/09/17
- Re: [Qemu-devel] [PATCH v6 21/25] replay: replay BH for IDE trim operation, Paolo Bonzini, 2018/09/17
[Qemu-devel] [PATCH v6 22/25] replay: add BH oneshot event for block layer, Pavel Dovgalyuk, 2018/09/12