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