Am 07.10.2016 um 18:19 hat Paolo Bonzini geschrieben: > Unlike tracked_requests, this field also counts throttled requests [in > the BlockBackend layer], and remains non-zero if an AIO operation > needs a BH to be "really" completed.
Do you actually like this? I think this is an incredibly ugly layering violation that we would regret sooner or later. Requests that are pending in the BlockBackend layer should be drained by BlockBackend code. Intertwining it with the BlockDriverState feels wrong to me, the BDS shouldn't know about its parent's requests. At the BlockBackend level (or really anywhere in the graph), we have two different kinds of drain requests that we just fail to implement differently today: 1. blk_drain(), i.e. a request to actually drain all requests pending in this BlockBackend. This is what we implement today, even though only indirectly: We call bdrv_drain() on the root BDS and it calls back into what is case 2. 2. BdrvChildRole.drained_begin/end(), i.e. stop sending requests to a child node. The efficient way to do this is not what we're doing today (sending all throttled requests and waiting for their completion), but just stopping to process throttled requests. Your approach would permanently prevent us from doing this right and force us to do the full drain even for the second case. Maybe things become even a bit clearer if you extend your view of the graph by one level and take the users of BlockBackends into account. Nobody would expect block jobs or even guest devices to increment the refcount of BDSes for requests that they will ever want to issue. Instead, what we do is to stop them from sending requests while the child is drained. This is the model for BlockBackend. We're currently still using another layering violation there, which is aio_disable_external(), but this could be done in a clean way by having .drain_begin/end callbacks in BlockDevOps so that we propagate the requests properly through the graph instead of bypassing it. The advantage in fixing this would be that we don't stop devices in the same AioContext that aren't related to the BDS being drained. Not sure if it's important enough to actually fix it, but we should keep it in mind as the model for how things should really work if done properly. > With this change, it is no longer necessary to have a dummy > BdrvTrackedRequest for requests that are never serialising, and > it is no longer necessary to poll the AioContext once after > bdrv_requests_pending(bs) returns false. > > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com> Kevin
