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

[Kevin Wolf]

Am 09.09.2011 12:50, schrieb Nicholas Thomas:
> 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.

Good to see agreement here. Do you think that Paolo's patches need to be
changed or can we do everything else on top?

>> 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.

This is stricter semantics than is required. Write order is only
guaranteed if request B is issued after request A has completed. If A
has been submitted, but hasn't completed yet, and you submit B, then the
order is undefined.

>> 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.

We do have some timer stubs in qemu-tool.c since May. Of course, they
are just stubs and never really trigger the timer, but I assume that the
timeouts are really meant for qemu proper anyway, rather than qemu-img
or qemu-io.

Kevin