Re: [Qemu-devel] [PATCH] nbd: strict nbd_wr_syncv

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

15.05.2017 12:43, Vladimir Sementsov-Ogievskiy wrote:
> 12.05.2017 19:30, Paolo Bonzini wrote:
>>
>> On 12/05/2017 17:57, Vladimir Sementsov-Ogievskiy wrote:
>>> 12.05.2017 18:46, Paolo Bonzini wrote:
>>>> On 12/05/2017 16:17, Vladimir Sementsov-Ogievskiy wrote:
>>>>> nbd_wr_syncv is called either from coroutine or from client 
>>>>> negotiation
>>>>> code, when socket is in blocking mode. So, -EAGAIN is impossible.
>>>>>
>>>>> Furthermore, EAGAIN is confusing, as, what to read/write again? With
>>>>> EAGAIN as a return code we don't know how much data is already
>>>>> read or written by the function, so in case of EAGAIN the whole
>>>>> communication is broken.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>>>> <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>
>>>>> Hi all!
>>>>>
>>>>> If I understand right, nbd_wr_syncv is called either from coroutine
>>>>> or from client negotiation code, when socket is in blocking mode.
>>>>> So, some thoughts
>>>>>
>>>>> 1. let's establish this with an assert, because returning EAGAIN is
>>>>>      confusing (see above)
>>>> Yes, this seems like a good idea.
>>>>
>>>>> 2. should we in case of non-coroutine context start this coroutine in
>>>>>      nbd_wr_syncv, like in bdrv_prwv_co, and use non-blocking mode?
>>>>> 3. is there any problems or disadvantages of moving client 
>>>>> negotiation
>>>>>      to coroutine too?
>>>> When you move code to coroutines you need to be aware of what code can
>>>> now run concurrently, for example the monitor.  I'm not sure that it's
>>>> possible to do this.
>>> Hmm, can you please give some example of a problem? qcow2_open starts
>>> coroutines to read it's header, why nbd_open can't start
>>> coroutine/coroutines to read/write some negotiation data?
>> Ah, it's not a problem if you use synchronous I/O (aio_poll) within the
>> coroutines.  But then it's still blocking I/O in every way (except
>> you've fcntl-ed the descriptor to make it non-blocking); it's simply
>> hidden underneath coroutines and aio_poll.
>
> I mean, make negotiation behave like normal nbd communication, 
> non-blocking socket + yield.. So, some other coroutines may do their 
> work, while nbd-negotiation coroutine waits for io..
>
>>
>> Paolo
>
>

Also, one more question here: in nbd_negotiate_write(), why do we need 
qio_channel_add_watch? write_sync will yield with qio_channel_yield() 
until io complete, why to use 2 mechanisms to wake up a coroutine?


-- 
Best regards,
Vladimir