Re: [PATCH v2 14/15] block: Don't poll in bdrv_replace_child_noperm()

[Vladimir Sementsov-Ogievskiy]

On 11/18/22 20:41, Kevin Wolf wrote:
> In order to make sure that bdrv_replace_child_noperm() doesn't have to
> poll any more, get rid of the bdrv_parent_drained_begin_single() call.
>
> This is possible now because we can require that the parent is already
> drained through the child in question when the function is called and we
> don't call the parent drain callbacks more than once.
>
> The additional drain calls needed in callers cause the test case to run
> its code in the drain handler too early (bdrv_attach_child() drains
> now), so modify it to only enable the code after the test setup has
> completed.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

It's still hard to keep this all in mind, so weak:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

> ---
>   include/block/block-io.h     |   8 +++
>   block.c                      | 103 ++++++++++++++++++++++++++++++-----
>   block/io.c                   |   2 +-
>   tests/unit/test-bdrv-drain.c |  10 ++++
>   4 files changed, 108 insertions(+), 15 deletions(-)
>
> diff --git a/include/block/block-io.h b/include/block/block-io.h
> index 8f5e75756a..65e6d2569b 100644
> --- a/include/block/block-io.h
> +++ b/include/block/block-io.h
> @@ -292,6 +292,14 @@ bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector 
> *qiov, int64_t pos);
>    */
>   void bdrv_parent_drained_begin_single(BdrvChild *c, bool poll);
>   
> +/**
> + * bdrv_parent_drained_poll_single:
> + *
> + * Returns true if there is any pending activity to cease before @c can be
> + * called quiesced, false otherwise.
> + */
> +bool bdrv_parent_drained_poll_single(BdrvChild *c);
> +
>   /**
>    * bdrv_parent_drained_end_single:
>    *
> diff --git a/block.c b/block.c
> index d18512944d..3f12aba6ce 100644
> --- a/block.c
> +++ b/block.c

[..]

> @@ -2863,11 +2905,9 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
>       }
>   
>       /*
> -     * If the old child node was drained but the new one is not, allow
> -     * requests to come in only after the new node has been attached.
> -     *
> -     * Update new_bs_quiesce_counter because bdrv_parent_drained_begin_single()
> -     * polls, which could have changed the value.
> +     * If the parent was drained through this BdrvChild previously, but new_bs
> +     * is not drained, allow requests to come in only after the new node has
> +     * been attached.

As I understand,the main reason why we MUST do this automatic undrain, is the 
contract with the user:

User things that:

1. It's enough to drain node X to drain all its parents (thanks to recursion)
2. User should undrain exactly same nodes that was drained by hand, everything 
that was drained automatically would be automatically undrained.

So here we break the connection between X and its parent, therefore recursion 
will not help on final undrain. So, we should do the automation here.



I have an idea how to (probably) make things even more simple.

1. drop .quiesced_parent

2. consider the Full graph, including non-bds parents, and support having 
.quiesce_counter for non-bds parents. (probably need some structure to unify 
bds and non-bds nodes of the Full graph - Generic ndoe, that's not the first 
time we are saying about that)

3. drop any recursion and automatic drain/undrain

4. user is responsible to drain all the nodes and their parents as needed to 
proceed with some block graph modification

5. user keeps the list of all nodes that was drained to undrain them in the end

6. node may be drained only when all its parents are already drained (add an 
assertion)


And of course we need helpers for the user to do 4-6. That should work similar 
to permissions update. Add a function to produce a topologically sorted list of 
Generic nodes (starting from some node and include all its parents and their 
parents and so on), and  simple functions that to drain/undrain of such list in 
a loop.



--
Best regards,
Vladimir