Re: [Qemu-devel] Block layer complexity: what to do to keep it under control?

[Fam Zheng <famz@redhat.com>]

On Wed, 11/29 12:00, Stefan Hajnoczi wrote:
> On Wed, Nov 29, 2017 at 11:55:02AM +0800, Fam Zheng wrote:
> > As we move forwards with new features in the block layer, the chances of tricky
> > bugs happening have been increasing alongside - block jobs, coroutines,
> > throttling, AioContext, op blockers and image locking combined together make a
> > large and complex picture that is hard to fully understand and work with. Some
> > bugs we've encountered are quite challenging already.  Examples are:
> > 
> > - segfault in parallel blockjobs (iotest 30)
> >   https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg01144.html
> > 
> > - Intermittent hang of iotest 194 (bdrv_drain_all after non-shared storage
> >   migration)
> >   https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg01626.html
> > 
> > - Drainage in bdrv_replace_child_noperm()
> >   https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg00868.html
> > 
> > - Regression from 2.8: stuck in bdrv_drain()
> >   https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg02193.html
> > 
> > So in principle, what should we do to make the block layer easy to understand,
> > develop with and debug?
> 
> The assumptions that the code relies on are unclear so it's easy to
> introduce new bugs.

Is that one thing we could do better in documenting?

> 
> We are at a point where code review isn't finding certain bugs because
> no single person knows all the assumptions.  Previously the problem was
> contained because maintainers spotted problems before patches were
> merged.
> 
> This is not primarily a documentation problem though.  We cannot
> document our way out of this because no single person (patch author or
> code reviewer) can know or check everything anymore due to the scale.
> 
> I think it's a (lack of) design problem because we have many incomplete
> abstractions like block jobs, IOThreads, block graph, image locking,
> etc.  They do not cover all possibly states and interactions today.
> Extending them leads to complex bugs.
> 
> A little progress has been made with defining higher-level APIs for
> block drivers and block jobs.  This way they either don't deal with
> low-level details of the concurrency and event loop models (e.g.
> bdrv_coroutine_enter()) or there is an interface that prompts them to
> integrate properly like bdrv_attach/detach_aio_context().

Sounds good.

> 
> Event loops and coroutines are good but they should not be used directly
> by block drivers and block jobs.  We need safe, high-level APIs that
> implement commonly-used operations.
> 
> > - Documentation
> > 
> >   There is no central developer doc about block layer, especially how all pieces
> >   fit together. Having one will make it a lot easier for new contributors to
> >   understand better. Of course, we're facing the old problem: the code is
> >   moving, maintaining an updated document needs effort.
> > 
> >   Idea: add ./doc/deve/block.txt?
> 
> IOThreads and AioContexts are addressed here:
> docs/devel/multiple-iothreads.txt
> 
> The game has become significantly more complex than what the document
> describes.  It's lacking aio_co_wake() and aio_co_schedule() for
> example.
> 
> > - Simplified code, or more orthogonal/modularized architecture.
> > 
> >   Each aspect of block layer is complex enough so isolating them as much as
> >   possible is a reasonable approach to control the complexity. Block jobs and
> >   throttling becoming block filters is a good example, we should identify more.
> > 
> >   Idea: rethink event loops. Create coroutines ubiquitously (for example for
> >   each fd handler, BH and timer), so that many nested aio_poll() can be removed.
> > 
> >   Crazy idea: move the whole block layer to a vhost process, and implement
> >   existing features differently, especially in terms of multi-threading (hint:
> >   rust?).
> 
> A reimplementation will not solve the problem because:
> 
> 1. If it still has the same feature set and requirements then the level
>    of complexity will be comparable.
> 
> 2. We can reduce accidental (inessential) complexity by continuing the
>    various efforts around the block graph, block jobs, multi-queue block
>    layer with an eye towards higher level APIs.

Starting over is certainly not the motivation to do qemu-vhost, but it would be
an opportunity to use different async/concurrency paradigms if that is going to
happen. I think in current block layer, event loop + coroutine is a good
combination, but having nested aio_poll()'s made it worse, then mixing IOThreads
in makes it a lot more complicated.

Fam