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

[Vladimir Sementsov-Ogievskiy]

16.05.2017 12:10, Vladimir Sementsov-Ogievskiy wrote:
> 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 
> > > > > > <address@hidden>
> > > > > > ---
> > > > > >
> > > > > > 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?

Hmm, these nbd_negotiate_* functions was introduced in 1a6245a5b, when 
nbd_wr_syncv was working through qemu_co_sendv_recvv, which just yields, 
without setting any handlers. But now, nbd_wr_syncv works through 
qio_channel_yield() which sets handlers, so the code with extra watchers 
looks wrong.


--
Best regards,
Vladimir