Re: [Qemu-devel] Re: [PATCH 2/3] NBD library: add aio-compatible read/write function

[Nicholas Thomas]

On Mon, 2011-02-21 at 20:10 +0000, Stefan Hajnoczi wrote:
> On Mon, Feb 21, 2011 at 12:37 PM, Kevin Wolf <address@hidden> wrote:
> > Am 18.02.2011 13:55, schrieb Nick Thomas:
> >> +retry:
> >> +    if (do_read) {
> >> +        ret = recvmsg(sockfd, &msg, 0);
> >> +    } else {
> >> +        ret = sendmsg(sockfd, &msg, 0);
> >> +    }
> >> +
> >> +    /* recoverable error */
> >> +    if (ret == -1 && (errno == EAGAIN || errno == EINTR)) {
> >> +        goto retry;
> >> +    }
> >
> > Make this a do...while loop, no reason for goto here.
> 
> This is not asynchronous.
> 
> If we're writing and the in-kernel socket buffer is full then we'll spin
> retrying - this burns CPU for no reason.  Instead we should get a
> callback when the socket becomes writable again so we can fill it with
> more data.
> 
> If we're reading and enough data hasn't arrived yet we will spin
> retrying.  Again, we should set up a callback to be notified when the
> socket becomes readable again.
> 
> It seems sheepdog suffers from the same problem.

Here, I'm a little concerned by the possibility of getting reads and
writes from different requests interleaved - so, writing half of the
data for one write request, returning, then getting another write
request through and starting that before the other one is completed. 

Obviously, I 'just' need to write clever code that avoids that ;)

Since sheepdog has the same issue, it might be worth me looking at this
as a separate issue in both code bases, starting from a patch that gets
nbd to the same kind of state as sheepdog in this area? The intermediate
stage is definitely better than the starting point, after all.

v3 of patches incoming, with this in mind.

/Nick