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

[Kevin Wolf <kwolf@redhat.com>]

Am 17.01.2019 um 13:57 hat Denis Plotnikov geschrieben:
> Kevin,
> 
> could you please take a look at my last comments?

I read it, and what it told me is essentially that I need to work on it
myself to fully understand the problem and possible acceptable solutions
because you can't seem to find one yourself. I will, but I can't
guarantee when I can find the time for it.

Kevin

> On 15.01.2019 10:22, Denis Plotnikov wrote:
> > ping ping ping ping!!!!
> > 
> > On 09.01.2019 11:18, Denis Plotnikov wrote:
> >> ping ping!!!
> >>
> >> On 18.12.2018 11:53, Denis Plotnikov wrote:
> >>> ping ping
> >>>
> >>> On 14.12.2018 14:54, Denis Plotnikov wrote:
> >>>>
> >>>>
> >>>> On 13.12.2018 15:20, Kevin Wolf wrote:
> >>>>> Am 13.12.2018 um 12:07 hat Denis Plotnikov geschrieben:
> >>>>>> On 12.12.2018 15:24, Kevin Wolf wrote:
> >>>>>>> Am 11.12.2018 um 17:55 hat Denis Plotnikov geschrieben:
> >>>>>>>>> Why involve the AioContext at all? This could all be kept at the
> >>>>>>>>> BlockBackend level without extending the layering violation that
> >>>>>>>>> aio_disable_external() is.
> >>>>>>>>>
> >>>>>>>>> BlockBackends get notified when their root node is drained, so 
> >>>>>>>>> hooking
> >>>>>>>>> things up there should be as easy, if not even easier than in
> >>>>>>>>> AioContext.
> >>>>>>>>
> >>>>>>>> Just want to make sure that I understood correctly what you 
> >>>>>>>> meant by
> >>>>>>>> "BlockBackends get notified". Did you mean that bdrv_drain_end 
> >>>>>>>> calls
> >>>>>>>> child's role callback blk_root_drained_end by calling
> >>>>>>>> bdrv_parent_drained_end?
> >>>>>>>
> >>>>>>> Yes, blk_root_drained_begin/end calls are all you need. 
> >>>>>>> Specifically,
> >>>>>>> their adjustments to blk->quiesce_counter that are already there, 
> >>>>>>> and in
> >>>>>>> the 'if (--blk->quiesce_counter == 0)' block of 
> >>>>>>> blk_root_drained_end()
> >>>>>>> we can resume the queued requests.
> >>>>>> 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...
> >>>> 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);
> >>>>
> >>>> If the tree isn't ready and we resume the coroutines, we'll end up 
> >>>> with the request landed in a wrong block driver state.
> >>>>
> >>>> So, we explicitly should stop all activities on all the driver states
> >>>> and its parents and allow the activities when everything is ready to 
> >>>> go.
> >>>>
> >>>> Why explicitly, because the block driver states may belong to 
> >>>> different block backends at the moment of the manipulation beginning.
> >>>>
> >>>> So, it seems we need to disable all their contexts until the 
> >>>> manipulation ends.
> >>>>
> >>>> Please, correct me if I'm wrong.
> >>>>
> >>>>>
> >>>>>> In seems that if the external requests disabled on the context we 
> >>>>>> can't
> >>>>>> rely on anything or should check where the underlying bs and its
> >>>>>> underlying nodes are ready to receive requests which sounds quite
> >>>>>> complicated.
> >>>>>> Please correct me if still don't understand something in that 
> >>>>>> routine.
> >>>>>
> >>>>> I think the reason why reyling on aio_disable_external() works is 
> >>>>> simply
> >>>>> because src is also drained, which keeps external events in the
> >>>>> AioContext disabled despite the bug in draining the target node.
> >>>>>
> >>>>> The bug would become apparent even with aio_disable_external() if we
> >>>>> didn't drain src, or even if we just supported src and target being in
> >>>>> different AioContexts.
> >>>>
> >>>> Why don't we disable all those contexts involved until the end of 
> >>>> the block device tree reconstruction?
> >>>>
> >>>> Thanks!
> >>>>
> >>>> Denis
> >>>>>
> >>>>> Kevin
> >>>>>
> >>>>
> >>>
> >>
> > 
> 
> -- 
> Best,
> Denis