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

[Stefan Hajnoczi]

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

signature.asc
Description: PGP signature