Re: [Qemu-block] [Qemu-devel] [PULL 16/71] nbd: Increase bs->in_flight during AioContext switch

[Vladimir Sementsov-Ogievskiy]

11.04.2019 19:48, Kevin Wolf wrote:
> Am 11.04.2019 um 16:48 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 11.04.2019 17:15, Kevin Wolf wrote:
>>> Am 11.04.2019 um 15:40 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> 25.02.2019 18:19, Kevin Wolf wrote:
>>>>> bdrv_drain() must not leave connection_co scheduled, so bs->in_flight
>>>>> needs to be increased while the coroutine is waiting to be scheduled
>>>>> in the new AioContext after nbd_client_attach_aio_context().
>>>>
>>>> Hi!
>>>>
>>>> I have some questions, could you explain, please?
>>>>
>>>> "bdrv_drain() must not leave connection_co scheduled" - it's because we 
>>>> want to be
>>>> sure that connection_co yielded from nbd_read_eof, yes?
>>>>
>>>> But it is guaranteed by aio_wait_bh_oneshot.. Why do we need additioinally 
>>>> inc/dec
>>>> bs->in_flight ?
>>>
>>> Without incrementing bs->in_flight, nothing would guarantee that
>>> aio_poll() is called and the BH is actually executed before bdrv_drain()
>>> returns.
>>
>> Don't follow.. Don't we want exactly this, we want BH to be executed while 
>> node is still
>> drained, as you write in comment?
> 
> Yes, exactly. But if bs->in_flight == 0, the AIO_WAIT_WHILE() condition
> in the drain code could become false, so aio_poll() would not be called
> again and drain would return even if the BH is still pending.
> 

Ah, oops, sorry my English, I read it like "nothing would prevent". Understand 
now, thanks.

>>>
>>>>>
>>>>> Signed-off-by: Kevin Wolf <address@hidden>
>>>>> ---
>>>>>     block/nbd-client.c | 20 ++++++++++++++++++--
>>>>>     1 file changed, 18 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/block/nbd-client.c b/block/nbd-client.c
>>>>> index 60f38f0320..bfbaf7ebe9 100644
>>>>> --- a/block/nbd-client.c
>>>>> +++ b/block/nbd-client.c
>>>>> @@ -977,14 +977,30 @@ void nbd_client_detach_aio_context(BlockDriverState 
>>>>> *bs)
>>>>>         qio_channel_detach_aio_context(QIO_CHANNEL(client->ioc));
>>>>>     }
>>>>>     
>>>>> +static void nbd_client_attach_aio_context_bh(void *opaque)
>>>>> +{
>>>>> +    BlockDriverState *bs = opaque;
>>>>> +    NBDClientSession *client = nbd_get_client_session(bs);
>>>>> +
>>>>> +    /* The node is still drained, so we know the coroutine has yielded in
>>>>> +     * nbd_read_eof(), the only place where bs->in_flight can reach 0, 
>>>>> or it is
>>>>> +     * entered for the first time. Both places are safe for entering the
>>>>> +     * coroutine.*/
>>>>> +    qemu_aio_coroutine_enter(bs->aio_context, client->connection_co);
>>>>> +    bdrv_dec_in_flight(bs);
>>>>> +}
>>>>> +
>>>>>     void nbd_client_attach_aio_context(BlockDriverState *bs,
>>>>>                                        AioContext *new_context)
>>>>>     {
>>>>>         NBDClientSession *client = nbd_get_client_session(bs);
>>>>>         qio_channel_attach_aio_context(QIO_CHANNEL(client->ioc), 
>>>>> new_context);
>>>>>     
>>>>> -    /* FIXME Really need a bdrv_inc_in_flight() here */
>>>>> -    aio_co_schedule(new_context, client->connection_co);
>>>>> +    bdrv_inc_in_flight(bs);
>>>>> +
>>>>> +    /* Need to wait here for the BH to run because the BH must run while 
>>>>> the
>>>>> +     * node is still drained. */
>>>>> +    aio_wait_bh_oneshot(new_context, nbd_client_attach_aio_context_bh, 
>>>>> bs);
>>>>>     }
>>>>>     
>>>>>     void nbd_client_close(BlockDriverState *bs)
>>>>>
>>>>
>>>>
>>>> -- 
>>>> Best regards,
>>>> Vladimir
>>
>>
>> -- 
>> Best regards,
>> Vladimir


-- 
Best regards,
Vladimir