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