Re: [PATCH for-5.0 v2 13/23] mirror: Double-check immediately before replacing

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

11.11.2019 19:02, Max Reitz wrote:
> There is no guarantee that we can still replace the node we want to
> replace at the end of the mirror job.  Double-check by calling
> bdrv_recurse_can_replace().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/mirror.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index f0f2d9dff1..68a4404666 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -695,7 +695,19 @@ static int mirror_exit_common(Job *job)
>            * drain potential other users of the BDS before changing the graph. */
>           assert(s->in_drain);
>           bdrv_drained_begin(target_bs);
> -        bdrv_replace_node(to_replace, target_bs, &local_err);
> +        /*
> +         * Cannot use check_to_replace_node() here, because that would
> +         * check for an op blocker on @to_replace, and we have our own
> +         * there.
> +         */

interesting, that check_to_replace_node would acquire aio context of src..

Here we acquire aio context only if s->to_replace set (above this hunk).. Isn't it a bug?

If it is, it's preexisting, and not directly related to the patch, so here:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> +        if (bdrv_recurse_can_replace(src, to_replace)) {
> +            bdrv_replace_node(to_replace, target_bs, &local_err);
> +        } else {
> +            error_setg(&local_err, "Can no longer replace '%s' by '%s', "
> +                       "because it can no longer be guaranteed that doing so "
> +                       "would not lead to an abrupt change of visible data",
> +                       to_replace->node_name, target_bs->node_name);
> +        }
>           bdrv_drained_end(target_bs);
>           if (local_err) {
>               error_report_err(local_err);
> 


-- 
Best regards,
Vladimir