On 11/04/2018 18:39, Kevin Wolf wrote:
> +bool bdrv_drain_poll(BlockDriverState *bs, bool top_level)
> {
> /* Execute pending BHs first and check everything else only after the BHs
> * have executed. */
> - while (aio_poll(bs->aio_context, false));
> + if (top_level) {
> + while (aio_poll(bs->aio_context, false));
> + }
> +
> + if (bdrv_parent_drained_poll(bs)) {
> + return true;
> + }
> +
> return atomic_read(&bs->in_flight);
> }
>
Up until now I liked very much this series, but I like this patch a bit
less for two reasons.
1) I think I would prefer to have the !top_level case in a separate
function---making the API easier to use in the BdrvChildRole callback
because there is no need to pass false. In addition, the callback is
not really polling anything, but rather returning whether the children
are quiescent. So a better name would be bdrv_children_drained and
c->role->drained.
2) Worse, the main idea behind the first drain restructuring was that
draining could proceed in topological order: first drain the roots' I/O,
then call bdrv_drain to send the last requests to their children, then
recurse. It is not clear to me why you need to introduce this recursive
step, which is also O(n^2) in the worst case.
Paolo
