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

[Denis Plotnikov]

On 17.01.2019 17:23, Kevin Wolf wrote:
> 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
ok. Thanks!

Denis
> 
>> 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

-- 
Best,
Denis