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

[Denis Plotnikov]

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.

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.

Denis
>> In case if it's so, it won't work if resume postponed requests in
>> blk_root_drained_end since we can't know if external is disabled for the
>> context because the counter showing that is decreased only after roles'
>> drained callbacks are finished at bdrv_do_drained_end.
>> Please correct me if I'm wrong.
> 
> You don't need to know about the AioContext state, this is the whole
> point. blk->quiesce_counter is what tells you whether to postpone
> requests.
> 
>> Looking at the patch again, I think that it might be useful to keep the
>> requests in the structure that limits their execution and also protects
>> the access (context acquire/release) although it's indeed the layering
>> violation but at least we can store the parts related at the same place
>> and later on move somewhere else alongside the request restrictor.
> 
> You can keep everything you need in BlockBackend (and that's also where
> your code is that really postpones request).
> 
> Kevin
> 

-- 
Best,
Denis