Re: [Qemu-devel] [PATCH 3/4] mirror: Keep target drained until graph changes are done

[Max Reitz]

On 25.07.19 18:27, Kevin Wolf wrote:
> Calling bdrv_drained_end() for target_bs can restarts requests too
> early, so that they would execute on mirror_top_bs, which however has
> already dropped all permissions.
> 
> Keep the target node drained until all graph changes have completed.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block/mirror.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 8cb75fb409..7483051f8d 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -644,6 +644,11 @@ static int mirror_exit_common(Job *job)
>      bdrv_ref(mirror_top_bs);
>      bdrv_ref(target_bs);
>  
> +    /* The mirror job has no requests in flight any more, but we need to
> +     * drain potential other users of the BDS before changing the graph. */
> +    assert(s->in_drain);
> +    bdrv_drained_begin(target_bs);
> +

In contrast to what Eric said, I think it is a problem that this is just
code motion.

The comment doesn’t tell the reason why the target needs to be drained
here.  Other users of the BDS have their own BdrvChild and thus their
own permissions, their requests do not go through mirror.

So in addition to why the target needs to be drained around
bdrv_replace_node(), the comment should tell why we need to drain it
here, like the commit message does.

Now, the thing is, I don’t quite understand the connection between the
target and mirror_top_bs that the commit message wants to establish.

I see the following problem:
(1) We drain src (at the end of mirror_run()).
(2) This implicitly drains mirror_top_bs.
(3) We drain target.
(4) bdrv_replace_node() replaces src by target, thus replacing the drain
    on mirror_top_bs from src by the one from target.
(5) We undrain target, thus also undraining mirror_top_bs.
(6) After all is done, we undrain src, which has no effect on
    mirror_top_bs, because they haven’t been connected since (4).

I suppose (5) is the problem.  This patch moves it down to (6), so
mirror_top_bs is drained as long as src is drained.

(If to_replace is not src, then src will stay attached, which keeps
mirror_top_bs drained, too.)

This makes it seem to me like the actually important thing is to drain
mirror_top_bs, not target.  If so, it would seem more obvious to me to
just add a drain on mirror_top_bs than to move the existing target drain.

>      /* Remove target parent that still uses BLK_PERM_WRITE/RESIZE before
>       * inserting target_bs at s->to_replace, where we might not be able to 
> get
>       * these permissions.
> @@ -684,12 +689,7 @@ static int mirror_exit_common(Job *job)
>              bdrv_reopen_set_read_only(target_bs, ro, NULL);
>          }
>  
> -        /* The mirror job has no requests in flight any more, but we need to
> -         * drain potential other users of the BDS before changing the graph. 
> */
> -        assert(s->in_drain);
> -        bdrv_drained_begin(target_bs);

By the way, don’t we need to drain to_replace also?  In case it isn’t src?

Max

>          bdrv_replace_node(to_replace, target_bs, &local_err);
> -        bdrv_drained_end(target_bs);
>          if (local_err) {
>              error_report_err(local_err);
>              ret = -EPERM;
> @@ -704,7 +704,6 @@ static int mirror_exit_common(Job *job)
>          aio_context_release(replace_aio_context);
>      }
>      g_free(s->replaces);
> -    bdrv_unref(target_bs);
>  
>      /*
>       * Remove the mirror filter driver from the graph. Before this, get rid 
> of
> @@ -724,9 +723,12 @@ static int mirror_exit_common(Job *job)
>      bs_opaque->job = NULL;
>  
>      bdrv_drained_end(src);
> +    bdrv_drained_end(target_bs);
> +
>      s->in_drain = false;
>      bdrv_unref(mirror_top_bs);
>      bdrv_unref(src);
> +    bdrv_unref(target_bs);
>  
>      return ret;
>  }
>

signature.asc
Description: OpenPGP digital signature