Re: [Qemu-devel] [PATCH v5 08/45] Return path: socket_writev_buffer: Block even on non-blocking fd's

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* David Gibson (david@gibson.dropbear.id.au) wrote:
> On Wed, Feb 25, 2015 at 04:51:31PM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > The return path uses a non-blocking fd so as not to block waiting
> > for the (possibly broken) destination to finish returning a message,
> > however we still want outbound data to behave in the same way and block.
> 
> It's not clear to me from this description exactly where the situation
> is that you need to write to the non-blocking socket.  Is it on the
> source or the destination?  If the source, why are you writing to the
> return path?  If the destination, why are you marking the outgoing
> return path as non-blocking?

My understanding is that the semantics of set_nonblock() are to
set non-blocking on all operations on the transport associated with
the fd - and that it's true even if you dup() the fd; and so if you
set non-blocking in one direction you get it in the other direction as well.

The (existing) destination side sets non-block (see process_incoming_migration
in migration.c), and so it gets non-blocking on the incoming data stream, but
that has the side effect that it's also going to be non-blocking on
the destinations writes to the reverse-path; thus we need to be able
to safely do writes from the destination reverse-path.

The text is out of date, back in v2 the source used to use non-blocking
for the return-path, but we managed to kill that off by using a thread
for the return path in the source.

How about changing the text to:
--------
The destination sets the fd to non-blocking on incoming migrations;
this also affects the return path from the destination, and thus we
need to make sure we can safely write to the return path.

Dave

> 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  migration/qemu-file-unix.c | 41 ++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 36 insertions(+), 5 deletions(-)
> > 
> > diff --git a/migration/qemu-file-unix.c b/migration/qemu-file-unix.c
> > index 50291cf..218dbd0 100644
> > --- a/migration/qemu-file-unix.c
> > +++ b/migration/qemu-file-unix.c
> > @@ -39,12 +39,43 @@ static ssize_t socket_writev_buffer(void *opaque, struct iovec *iov, int iovcnt,
> >      QEMUFileSocket *s = opaque;
> >      ssize_t len;
> >      ssize_t size = iov_size(iov, iovcnt);
> > +    ssize_t offset = 0;
> > +    int     err;
> >  
> > -    len = iov_send(s->fd, iov, iovcnt, 0, size);
> > -    if (len < size) {
> > -        len = -socket_error();
> > -    }
> > -    return len;
> > +    while (size > 0) {
> > +        len = iov_send(s->fd, iov, iovcnt, offset, size);
> > +
> > +        if (len > 0) {
> > +            size -= len;
> > +            offset += len;
> > +        }
> > +
> > +        if (size > 0) {
> > +            err = socket_error();
> > +
> > +            if (err != EAGAIN) {
> > +                error_report("socket_writev_buffer: Got err=%d for (%zd/%zd)",
> > +                             err, size, len);
> > +                /*
> > +                 * If I've already sent some but only just got the error, I
> > +                 * could return the amount validly sent so far and wait for the
> > +                 * next call to report the error, but I'd rather flag the error
> > +                 * immediately.
> > +                 */
> > +                return -err;
> > +            }
> > +
> > +            /* Emulate blocking */
> > +            GPollFD pfd;
> > +
> > +            pfd.fd = s->fd;
> > +            pfd.events = G_IO_OUT | G_IO_ERR;
> > +            pfd.revents = 0;
> > +            g_poll(&pfd, 1 /* 1 fd */, -1 /* no timeout */);
> > +        }
> > +     }
> > +
> > +    return offset;
> >  }
> >  
> >  static int socket_get_fd(void *opaque)
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK