Re: [Qemu-block] [PATCH 1/3] block: add BDS field to count in-flight requests

[Kevin Wolf]

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 <address@hidden>

Kevin