Re: [Qemu-devel] [PATCH] blk: postpone request execution on a context protected with "drained section"

[Kevin Wolf <kwolf@redhat.com>]

Am 02.04.2019 um 10:35 hat Denis Plotnikov geschrieben:
> 
> 
> On 13.03.2019 19:04, Kevin Wolf wrote:
> > Am 14.12.2018 um 12:54 hat Denis Plotnikov geschrieben:
> >> On 13.12.2018 15:20, Kevin Wolf wrote:
> >>> Am 13.12.2018 um 12:07 hat Denis Plotnikov geschrieben:
> >>>> Sounds it should be so, but it doesn't work that way and that's why:
> >>>> when doing mirror we may resume postponed coroutines too early when the
> >>>> underlying bs is protected from writing at and thus we encounter the
> >>>> assert on a write request execution at bdrv_co_write_req_prepare when
> >>>> resuming the postponed coroutines.
> >>>>
> >>>> The thing is that the bs is protected for writing before execution of
> >>>> bdrv_replace_node at mirror_exit_common and bdrv_replace_node calls
> >>>> bdrv_replace_child_noperm which, in turn, calls child->role->drained_end
> >>>> where one of the callbacks is blk_root_drained_end which check
> >>>> if(--blk->quiesce_counter == 0) and runs the postponed requests
> >>>> (coroutines) if the coundition is true.
> >>>
> >>> Hm, so something is messed up with the drain sections in the mirror
> >>> driver. We have:
> >>>
> >>>       bdrv_drained_begin(target_bs);
> >>>       bdrv_replace_node(to_replace, target_bs, &local_err);
> >>>       bdrv_drained_end(target_bs);
> >>>
> >>> Obviously, the intention was to keep the BlockBackend drained during
> >>> bdrv_replace_node(). So how could blk->quiesce_counter ever get to 0
> >>> inside bdrv_replace_node() when target_bs is drained?
> >>>
> >>> Looking at bdrv_replace_child_noperm(), it seems that the function has
> >>> a bug: Even if old_bs and new_bs are both drained, the quiesce_counter
> >>> for the parent reaches 0 for a moment because we call .drained_end for
> >>> the old child first and .drained_begin for the new one later.
> >>>
> >>> So it seems the fix would be to reverse the order and first call
> >>> .drained_begin for the new child and then .drained_end for the old
> >>> child. Sounds like a good new testcase for tests/test-bdrv-drain.c, too.
> >> Yes, it's true, but it's not enough...
> > 
> > Did you ever implement the changes suggested so far, so that we could
> > continue from there? Or should I try and come up with something myself?
> 
> Sorry for the late reply...
> Yes, I did ...

If there are more question or problems, can you post the patches in
their current shape (as an RFC) or a git URL so I can play with it a
bit? If you could include a failing test case, too, that would be ideal.

> >> In mirror_exit_common() we actively manipulate with block driver states.
> >> When we replaced a node in the snippet you showed we can't allow the
> >> postponed coroutines to run because the block tree isn't ready to
> >> receive the requests yet.
> >> To be ready, we need to insert a proper block driver state to the block
> >> backend which is done here
> >>
> >>       blk_remove_bs(bjob->blk);
> >>       blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);
> >>       blk_insert_bs(bjob->blk, mirror_top_bs, &error_abort); << << << <<
> >>
> >>       bs_opaque->job = NULL;
> >>
> >>       bdrv_drained_end(src);
> > 
> > Did you actually encounter a bug here or is this just theory? bjob->blk
> > is the BlockBackend of the job and isn't in use at this point any more.
> > We only insert the old node in it again because block_job_free() must
> > set bs->job = NULL, and it gets bs with blk_bs(bjob->blk).
> > 
> > So if there is an actual bug here, I don't understand it yet.
> And did encounter the bug that I described above.
> When a postponed coroutine resumes it fails on assert:
> 
> bdrv_co_write_req_prepare: Assertion `child->perm & BLK_PERM_WRITE' failed
> 
> That's why it happens: we have the mirror filter bds in blk root which 
> receives all the requests. On mirror completion we call 
> mirror_exit_common to finish mirroring. To finish mirroring we need to 
> remove the mirror filter from the graph and set mirror file blk root.
> We call block_job_complete. Assume the ide request has came after the 
> completion calling and has been postponed because blk->quiesce_counter 
> is not 0.

Just to avoid confusion, there are two BlockBackends here. Lets call the
one attached to the IDE device the ide_blk, and the one owned by the
block job job_blk.

> block_job_complete does mirror_exit_common which drops the permissions.
> 
>      /* We don't access the source any more. Dropping any WRITE/RESIZE is
>       * required before it could become a backing file of target_bs. */
>      bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
>                              &error_abort);

Hm... This permission change is questionable (it temporarily drops the
file locks of the backing chain), but as long as we don't get new
requests to mirror_top_bs, it should be okay. As the attached parents
are drained, no requests should be made to mirror_top_bs until it is
removed from the graph.

> then, it replaces the source with the target
> 
>          /* 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. */
> 
>          // here, target_bs has no parents and doesn't begin to draing
> 
>          bdrv_drained_begin(target_bs);
> 
>          // after execution of the function below
>          // target bs has mirror_top_bs->backing as a parent
> 
>          bdrv_replace_node(to_replace, target_bs, &local_err);

Right, and both ide_blk and job_blk point to the subtree with target_bs
in it. They are both still drained because target_bs is drained.

src is also still drained, but it doesn't have parents any more. It is
only kept alive because of the explicit bdrv_ref() at the start of the
function.

>          // now target_bs has source's blk as a parent
>          // the following call sets blk->quiesce_counter to 0
>          // and executes the postponed coroutine on blk with
>          // mirror filter set which eventually does writing
>          // on mirror_top_bs->backing child which has no writing
>          // (and reading) permissions
> 
>          bdrv_drained_end(target_bs);
> 
> 
> Does it make thing more clear?

Yes, I see the problem now. So we accidentally end the drained section
before mirror_top_bs is actually removed from the chain.

How about the following then? It should make sure that requests can't
run before the whole graph change is completed.

Kevin

diff --git a/block/mirror.c b/block/mirror.c
index ff15cfb197..8e197779ec 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -643,6 +643,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);
+
     /* 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.
@@ -682,12 +687,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);
         bdrv_replace_node(to_replace, target_bs, &local_err);
-        bdrv_drained_end(target_bs);
         if (local_err) {
             error_report_err(local_err);
             ret = -EPERM;
@@ -722,6 +722,7 @@ static int mirror_exit_common(Job *job)

     bs_opaque->job = NULL;

+    bdrv_drained_end(target_bs);
     bdrv_drained_end(src);
     s->in_drain = false;
     bdrv_unref(mirror_top_bs);