Re: [Qemu-devel] [PATCH 00/12] nbd improvements

[Nicholas Thomas <nick@bytemark.co.uk>]

On Fri, 2011-09-09 at 12:29 +0200, Paolo Bonzini wrote:
> On 09/09/2011 11:00 AM, Kevin Wolf wrote:
> > There is anonther patch enabling AIO for NBD on the list [1], by
> > Nicholas Thomas (CCed), that lacked review so far. Can you guys please
> > review each others approach and then converge on a solution? I guess
> > Paolo's patches 1-7 can be applied in any case, probably causing minor
> > conflicts, but for the rest we need to decide which one to pick.
> 
> Stefan also pointed me to Nicholas's patches yesterday.  I would go with 
> mine, if only because his work predates coroutines (at least the older 
> versions) and are much more complex.

I'd concur here. I wrote the AIO portion of my patches merely to get the
code into a state where I could add timeout and reconnect logic - I'm
pretty sure the code I wrote is *correct* (we're using it in production,
after all), but I'm by no means invested in it. Your version includes
TRIM and flush support as well, which is nice.

> On the other hand, Nicholas's work includes timeout and reconnect.  I'm 
> not sure how complicated it is to include it in my series, but probably 
> not much.  With coroutines, preserving the list of outstanding I/O 
> requests is done implicitly by the CoMutex, so you basically have to 
> check errno for ECONNRESET and similar errors, reconnect, and retry 
> issuing the current request only.

Definitely a simpler approach than my version. Although, does your
version preserve write ordering? I was quite careful to ensure that
write requests would always be sent in the order they were presented;
although I don't know if qemu proper depends on that behaviour or not.

> Timeout can be done with a QEMUTimer that shuts down the socket 
> (shutdown(2) I mean).  This triggers an EPIPE when writing, or a 
> zero-sized read when reading.  The timeout can be set every time the 
> coroutine is (re)entered, and reset before exiting nbd_co_readv/writev.

for reconnect, I did consider using QEMUTimer, but when I tried it I ran
into linking problems with qemu-io. As far as I can tell, resolving that
is a significant code reorganisation - QEMUTimer pulls in lots of code
that isn't linked in at the moment (VM clocks, etc). I'm not sure it's
worth tackling that to avoid using timer_create(2). 

The timeout code I have at the moment is something of a bodge and
replacing it with an actual timer (of either kind) would be a massive
improvement, obviously.

/Nick