Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3683 bytes --]

On Wed, Mar 02, 2022 at 02:07:30PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 01.03.2022 17:21, Emanuele Giuseppe Esposito wrote:
> > This serie tries to provide a proof of concept and a clear explanation
> > on why we need to use drains (and more precisely subtree_drains)
> > to replace the aiocontext lock, especially to protect BlockDriverState
> > ->children and ->parent lists.
> > 
> > Just a small recap on the key concepts:
> > * We split block layer APIs in "global state" (GS), "I/O", and
> > "global state or I/O".
> >    GS are running in the main loop, under BQL, and are the only
> >    one allowed to modify the BlockDriverState graph.
> > 
> >    I/O APIs are thread safe and can run in any thread
> > 
> >    "global state or I/O" are essentially all APIs that use
> >    BDRV_POLL_WHILE. This is because there can be only 2 threads
> >    that can use BDRV_POLL_WHILE: main loop and the iothread that
> >    runs the aiocontext.
> > 
> > * Drains allow the caller (either main loop or iothread running
> > the context) to wait all in_flights requests and operations
> > of a BDS: normal drains target a given node and is parents, while
> > subtree ones also include the subgraph of the node. Siblings are
> > not affected by any of these two kind of drains.
> > After bdrv_drained_begin, no more request is allowed to come
> > from the affected nodes. Therefore the only actor left working
> > on a drained part of the graph should be the main loop.
> > 
> > What do we intend to do
> > -----------------------
> > We want to remove the AioContext lock. It is not 100% clear on how
> > many things we are protecting with it, and why.
> > As a starter, we want to protect BlockDriverState ->parents and
> > ->children lists, since they are read by main loop and I/O but
> > only written by main loop under BQL. The function that modifies
> > these lists is bdrv_replace_child_common().
> > 
> > How do we want to do it
> > -----------------------
> > We individuated as ideal subtitute of AioContext lock
> > the subtree_drain API. The reason is simple: draining prevents the iothread to read or write the nodes, so once the main loop finishes
> 
> I'm not sure it's ideal. Unfortunately I'm not really good in all that BQL, AioContext locks and drains. So, I can't give good advice. But here are my doubts:
> 
> Draining is very restrictive measure: even if drained section is very short, at least on bdrv_drained_begin() we have to wait for all current requests, and don't start new ones. That slows down the guest. In the same time there are operations that don't require to stop guest IO requests. For example manipulation with dirty bitmaps - qmp commands block-dirty-bitmap-add block-dirty-bitmap-remove. Or different query requests..
> 
> I see only two real cases, where we do need drain:
> 
> 1. When we need a consistent "point-in-time". For example, when we start backup in transaction with some dirty-bitmap manipulation commands.
> 
> 2. When we need to modify block-graph: if we are going to break relation A -> B, there must not be any in-flight request that want to use this relation.
> 
> All other operations, for which we want some kind of lock (like AioContext lock or something) we actually don't want to stop guest IO.

Yes, I think so too.

Block drivers should already be at a point where block-dirty-bitmap-add
and similar commands can be synchronized with just the qcow2 CoMutex.

Not every aio_context_acquire() needs to be replace by a drained
section. Some can safely be dropped because there are fine-grained locks
in place that provide sufficient protection.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]