Re: [Qemu-devel] [PATCH v2 04/10] nbd-server: refactor simple reply sending

[Daniel P. Berrange]

On Mon, Oct 09, 2017 at 02:18:10PM -0500, Eric Blake wrote:
> [adding Dan for a qio question - search for your name below]
> 
> On 10/09/2017 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
> > Get rid of calculating structure fields offsets by hand and set_cork,
> > rename nbd_co_send_reply to nbd_co_send_simple_reply. Do not use
> > NBDReply which will be upgraded in future patches to handle both simple
> > and structured replies and will be used only in the client.
> > 
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >  include/block/nbd.h |  6 ++++
> >  nbd/nbd-internal.h  |  9 +++++
> >  nbd/server.c        | 97 
> > ++++++++++++++++++++++-------------------------------
> >  3 files changed, 56 insertions(+), 56 deletions(-)

> > -static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len,
> > -                             Error **errp)
> > +static int coroutine_fn nbd_co_send_iov(NBDClient *client, struct iovec 
> > *iov,
> > +                                        unsigned niov, Error **errp)
> >  {
> > -    NBDClient *client = req->client;
> >      int ret;
> >  
> >      g_assert(qemu_in_coroutine());
> > -
> > -    trace_nbd_co_send_reply(reply->handle, reply->error, len);
> > -
> >      qemu_co_mutex_lock(&client->send_lock);
> >      client->send_coroutine = qemu_coroutine_self();
> >  
> > -    if (!len) {
> > -        ret = nbd_send_reply(client->ioc, reply, errp);
> > -    } else {
> > -        qio_channel_set_cork(client->ioc, true);
> > -        ret = nbd_send_reply(client->ioc, reply, errp);
> > -        if (ret == 0) {
> > -            ret = nbd_write(client->ioc, req->data, len, errp);
> > -            if (ret < 0) {
> > -                ret = -EIO;
> > -            }
> > -        }
> > -        qio_channel_set_cork(client->ioc, false);
> > -    }
> > +    ret = nbd_writev(client->ioc, iov, niov, errp);
> 
> ...the transmission is now done via iov so that you can send two buffers
> at once without having to play with the cork,...
> 
> Dan - are we guaranteed that qio_channel_writev_all() with a multi-iov
> argument called by one thread will send all its data in order, without
> being interleaved with any other parallel coroutine also calling
> qio_channel_writev_all()?  In other words, it looks like the NBD code
> was previously using manual cork changes to ensure that two halves of a
> message were sent without interleave, but Vladimir is now relying on the
> qio code to do that under the hood.

The I/O channels code does not make guarantees wrt concurrent usage of
threads or coroutines. It is the callers responsibility to avoid any
concurrent usage for all APIs. With coroutines you are at least avoiding
the danger of corrupting memory state, but you still have risk of unexpected
data ordering.

IOW, if you have 2 coroutines each doing a series writes on the same QIOChannel
object, and one does a yield, I/O can certainly be interleaved between the two.
This is true whether doing multiple qio_channel_writev() calls directly, or
whether using qio_channel_writev_all().

Unless I'm misunderstanding though, cork did not offer you protection in
this scenario either. cork just prevents the data being transmitted onto
the wire immediately - ie it ensures that if you do 2 writes, they get
sent in 1 TCP packet instead of many TCP packets.

If you have multiple coroutines writing to the channel at the same time
and one yielded in its series of writes, the I/O from multiple coroutines
will get merged into that 1 single TCP packet when uncorked.

So if this concurrent usage is a problem NBD was already broken AFAICT.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|