From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:41897) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R1ypR-0001V9-SR for qemu-devel@nongnu.org; Fri, 09 Sep 2011 07:01:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R1ypQ-0008Em-6d for qemu-devel@nongnu.org; Fri, 09 Sep 2011 07:01:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:13277) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R1ypP-0008EL-Tj for qemu-devel@nongnu.org; Fri, 09 Sep 2011 07:01:00 -0400 Message-ID: <4E69F1E0.6050900@redhat.com> Date: Fri, 09 Sep 2011 13:00:48 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1315495505-28906-1-git-send-email-pbonzini@redhat.com> <4E69D595.8090000@redhat.com> <4E69EA89.6090706@redhat.com> <1315565422.30081.14.camel@desk4.office.bytemark.co.uk> In-Reply-To: <1315565422.30081.14.camel@desk4.office.bytemark.co.uk> Content-Type: text/plain; charset=UTF-8; format=flowed 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: Nicholas Thomas Cc: Kevin Wolf , qemu-devel@nongnu.org On 09/09/2011 12:50 PM, Nicholas Thomas wrote: >> > 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. Yes, the current operation does not drop the CoMutex until the error goes away. Ordering is preserved because in turn no request can start until the first leaves the coroutine (with the CoMutex locked). Anyway I think the guests do not depend on it, there is no ordering guarantee in the AIO thread pool. If any of the more advanced file formats do, that would be a bug. >> > 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). I agree. I think it's worth it because it would help Anthony's work with unit testing too; but it's also a significant amount of work. But in this sense, keeping each feature in a separate patch helps a lot. If something is done in a hacky way, it's much easier to redo it later if "git show" gives a good overview of how it was done. I saw earlier versions of your patch had problems with the upstream nbd server. It works for me, but you'd be welcome trying out my series. Paolo