Re: [Qemu-devel] lock-free monitor?

[Markus Armbruster <armbru@redhat.com>]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Sun, Feb 14, 2016 at 02:22:10PM +0800, Fam Zheng wrote:
>> On Tue, 02/09 13:47, Stefan Hajnoczi wrote:
>> > On Mon, Feb 08, 2016 at 03:17:23PM +0000, Dr. David Alan Gilbert wrote:
>> > > Does this make sense to everyone else, or does anyone have any better
>> > > suggestions?
>> > 
>> > As a concrete example, any monitor command that calls bdrv_drain_all()
>> > can hang forever with the QEMU global mutex held if I/O requests are
>> > stuck (e.g. NFS mount is unreachable).
>> > 
>> > bdrv_aio_cancel() can also hang but is mostly exposed to device
>> > emulation, not the monitor.
>> > 
>> > One solution for these block layer functions is to add a timeout
>> > argument and let them return an error.  This way the monitor and device
>> > emulation do not hang forever.
>> 
>> Yes, there are a few places in block layer invoking aio_poll() in a loop
>> waiting for certain events, and a disconnected network link could make QEMU
>> hang. In these cases a timeout is a huge improvement.  Maybe we can mark the
>> BDS as "hanging" (-EIO is returned for all further requests) and let
>> bdrv_drain_all() return.
>
> No, we need to be very careful about hung requests that are still in
> flight.  They could complete after a long time and modify the disk or
> guest RAM.
>
> The only thing bdrv_drain_all() can return is -ETIME.  Beyond that it
> cannot pretend that requests have been drained since that could lead to
> data corruption/loss.
>
> The caller has to abort whatever it was trying to do since it has no
> guarantee that requests have drained.  Maybe the remaining requests will
> fail and go away, maybe they will complete.  We don't know...
>
>> > 
>> > The benefit of the timeout is that both monitor and device emulation
>> > hangs are tackled.  It also doesn't require monitor changes.
>> > 
>> > I'm not sure who chooses the timeout value and which value makes sense
>> > (policy vs mechanism separation)...
>> 
>> Default to 30 seconds like Linux, and make it tunable through command line
>> options as well as QMP?
>
> Yes.  Either commands that block can take an optional timeout (seconds)
> parameter, or we could have a global disk I/O timeout value.  I prefer
> passing an optional per-command value.
>
> Libvirt and other sophisticated clients could begin using it in a
> backwards-compatible way.  Existing code wouldn't use it and still
> suffer from the hang problem (but at least QEMU is
> backwards-compatible).

I'm not arguing against timeouts, I just want to remind everyone that
blocking while holding the BQL is a bug, and limiting the block time
with timeouts is a work-around, no more, no less.