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

From: Kevin Wolf <kwolf@redhat.com>
To: Pavel Dovgalyuk <dovgaluk@ispras.ru>
Cc: 'Pavel Dovgalyuk' <Pavel.Dovgaluk@ispras.ru>,
	qemu-devel@nongnu.org, peter.maydell@linaro.org,
	war2jordan@live.com, mst@redhat.com, jasowang@redhat.com,
	zuban32s@gmail.com, kraxel@redhat.com,
	thomas.dullien@googlemail.com, artem.k.pisarenko@gmail.com,
	quintela@redhat.com, ciro.santilli@gmail.com, armbru@redhat.com,
	dgilbert@redhat.com, boost.lists@gmail.com,
	alex.bennee@linaro.org, rth@twiddle.net,
	crosthwaite.peter@gmail.com, mreitz@redhat.com,
	maria.klimushenkova@ispras.ru, pbonzini@redhat.com
Subject: Re: [Qemu-devel] [PATCH v7 17/19] replay: add BH oneshot event for block layer
Date: Fri, 30 Nov 2018 12:18:08 +0100	[thread overview]
Message-ID: <20181130111808.GC5106@localhost.localdomain> (raw)
In-Reply-To: <002201d48885$b2f251c0$18d6f540$@ru>

Am 30.11.2018 um 09:21 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > 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 <Pavel.Dovgaluk@ispras.ru>
> > 
> > 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.

So nobody really understands the code, but we're just fixing symptoms
whenever something crashes, without ever thinking about the design? And
then the code will organically grow to maybe approximate what we wanted
it to do?

Honestly, that's not the way to build reliable and maintainable
software.

> 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.

Are there any BHs that can never affect the guest state?

Maybe you should really intercept this inside qemu_bh_schedule() and
aio_bh_schedule_oneshot() to catch all instances. This would look more
likely to address the root cause rather than twenty different special
cases and forgetting the other hundred instances.

But why do you even need to record and replay BHs? Aren't they already
fully deterministic if the code that schedules them is deterministic? Is
this hinting at problems in a different part of the code, so that the
caller isn't deterministic even though we expect it to be?

Kevin