From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:58139) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R1yfE-00080p-FD for qemu-devel@nongnu.org; Fri, 09 Sep 2011 06:50:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R1yfD-0005NX-HQ for qemu-devel@nongnu.org; Fri, 09 Sep 2011 06:50:28 -0400 Received: from nog.sh.bytemark.co.uk ([212.110.161.168]:53400) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R1yfD-0005NQ-Cy for qemu-devel@nongnu.org; Fri, 09 Sep 2011 06:50:27 -0400 From: Nicholas Thomas In-Reply-To: <4E69EA89.6090706@redhat.com> References: <1315495505-28906-1-git-send-email-pbonzini@redhat.com> <4E69D595.8090000@redhat.com> <4E69EA89.6090706@redhat.com> Content-Type: text/plain; charset="UTF-8" Date: Fri, 09 Sep 2011 11:50:22 +0100 Message-ID: <1315565422.30081.14.camel@desk4.office.bytemark.co.uk> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 00/12] nbd improvements List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Kevin Wolf , qemu-devel@nongnu.org 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