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

[Kevin Wolf]

Am 26.07.2019 um 11:52 hat Max Reitz geschrieben:
> 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.

(5.5) Remove mirror_top_bs from the target chain

> (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.

The problem is that (5) happens before (5.5), so we can start requests
on a node that we're about to remove (without draining it again before).

> (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.

Do you really think having a third drained section makes things easier
to understand? Draining both source and target while we're modifying the
graph seems pretty intuitive to me - which is also why I moved the
bdrv_drained_begin() to the very start instead of looking for the first
operation that actually strictly needs it.

> >      /* 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?

I think to_replace is required to be in the subtree of src, no?

Though maybe it could have another parent, so you might be right.

Kevin

signature.asc
Description: PGP signature