Re: [Qemu-block] [PATCH 4/4] block/nbd-client: use non-blocking io channel for nbd negotiation

[Vladimir Sementsov-Ogievskiy]

19.02.2019 16:18, Vladimir Sementsov-Ogievskiy wrote:
> 12.02.2019 1:02, Eric Blake wrote:
>> On 2/11/19 6:56 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Now negotiation is done in coroutine, so to take benefit of it let's
>>> use non-blocking model.
>>>
>>> Note that QIOChannel handle synchronous io calls correctly anyway, so
>>
>> s/handle/handles/
>>
>>> it's not a problem to send final NBD_CMD_DISC to non-blocking channel
>>> but using sync qio interface, even not in coroutine.
>>
>> s/not in/when not in a/
>>
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>> ---
>>>   block/nbd-client.c | 10 +++-------
>>>   1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> This fixes qemu as NBD client for use in block devices, but what about
>> qemu as NBD client in qemu-nbd?  Does that need any updates?  Then
>> again, your observation about QIO doing the right thing for both
>> blocking (qemu-nbd) and non-blocking (block layer) channels seems to
>> cover that.
> 
> In qemu-nbd client works in a separate thread, so, I think, we don't need
> nonblocking: we have a thread anyway.
> 
> On the other hand, is my code in nbd_receive_common safe, keeping in mind
> that we call it from separate thread?
> 
> Are qio_channel_attach_aio_context, qemu_aio_coroutine_enter, AIO_WAIT_WHILE
> thread-safe?

Could somebody shed a bit of light here, please?

> 
>>
>>> @@ -1072,9 +1072,6 @@ static int nbd_client_connect(BlockDriverState *bs,
>>>           object_ref(OBJECT(client->ioc));
>>>       }
>>> -    /* Now that we're connected, set the socket to be non-blocking and
>>> -     * kick the reply mechanism.  */
>>> -    qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
>>>       client->connection_co = qemu_coroutine_create(nbd_connection_entry, 
>>> client);
>>>       nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));
>>> @@ -1083,9 +1080,8 @@ static int nbd_client_connect(BlockDriverState *bs,
>>>    fail:
>>>       /*
>>> -     * We have connected, but must fail for other reasons. The
>>> -     * connection is still blocking; send NBD_CMD_DISC as a courtesy
>>> -     * to the server.
>>> +     * We have connected, but must fail for other reasons.
>>> +     * Send NBD_CMD_DISC as a courtesy to the server.
>>>        */
>>>       {
>>>           NBDRequest request = { .type = NBD_CMD_DISC };
>>>
>>
> 
> 


-- 
Best regards,
Vladimir