[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 00/12] nbd improvements
From: |
Nicholas Thomas |
Subject: |
Re: [Qemu-devel] [PATCH 00/12] nbd improvements |
Date: |
Fri, 09 Sep 2011 11:50:22 +0100 |
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
- [Qemu-devel] [PATCH 10/12] block: add bdrv_co_flush support, (continued)
- [Qemu-devel] [PATCH 10/12] block: add bdrv_co_flush support, Paolo Bonzini, 2011/09/08
- [Qemu-devel] [PATCH 04/12] nbd: add support for NBD_CMD_FLUSH, Paolo Bonzini, 2011/09/08
- [Qemu-devel] [PATCH 07/12] sheepdog: add coroutine_fn markers, Paolo Bonzini, 2011/09/08
- [Qemu-devel] [PATCH 05/12] nbd: add support for NBD_CMD_FLAG_FUA, Paolo Bonzini, 2011/09/08
- Re: [Qemu-devel] [PATCH 00/12] nbd improvements, Kevin Wolf, 2011/09/09
Re: [Qemu-devel] [PATCH 00/12] nbd improvements, Kevin Wolf, 2011/09/14