From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=43613 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PpIkc-0006xl-2z for qemu-devel@nongnu.org; Tue, 15 Feb 2011 06:07:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PpIkZ-0006pe-98 for qemu-devel@nongnu.org; Tue, 15 Feb 2011 06:07:21 -0500 Received: from mx1.redhat.com ([209.132.183.28]:17088) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PpIkY-0006pY-UU for qemu-devel@nongnu.org; Tue, 15 Feb 2011 06:07:19 -0500 Message-ID: <4D5A5ECD.7060701@redhat.com> Date: Tue, 15 Feb 2011 12:09:01 +0100 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] NBD block device backend - 'improvements' References: <1297712422.12551.2.camel@den> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: nick@lupine.me.uk, qemu-devel@nongnu.org, Laurent Vivier Am 14.02.2011 21:32, schrieb Stefan Hajnoczi: > On Mon, Feb 14, 2011 at 7:40 PM, Nicholas Thomas wrote: >> I've written a patch that changes the behaviour - instead of exiting at >> startup, we wait for the NBD connection to be established, and we hang >> on reads and writes until the connection is re-established. > > Hi Nick, > I think reconnect is a useful feature. For more info on submitting > patches to QEMU, please see > http://wiki.qemu.org/Contribute/SubmitAPatch. It contains a few > points like sending patches inline instead of as an attachment (makes > review easier), using Signed-off-by:, and references to QEMU coding > style. > >> I'm interested in getting the changes merged upstream, so I thought I'd >> get in early and ask if you'd be interested in the patch, in principle; >> whether the old behaviour would need to be preserved, making the new >> behaviour accessible via a config option ("-drive >> file=nbd:127.0.0.1:5000:retry=forever,..." ?); and whether I'm going >> about the changes in a sane way (I've attached the current version of >> the patch). > > block/nbd.c needs to be made asynchronous in order for this change to > work. And even then it's not free of problem: For example qemu_aio_flush() will hang. We're having all kinds of fun with NFS servers that go away and let requests hang indefinitely. So maybe what we should add is a timeout option which defaults to 0 (fail immediately, like today) > Otherwise the only thing you can do is to block QEMU and the > VM, and even that shouldn't be done using sleep(2) because that could > stop the I/O thread which processes the QEMU monitor and other > external interfaces. See below for more info. Unconditionally stopping the VM from a block driver sounds wrong to me. If you want to have this behaviour, the block driver should return an error and you should use werror=stop. >> Another thing I've noticed is that the nbd library ( /nbd.c ) is not >> IPv6-compatible ( "-drive file=nbd:\:\:1:5000", for instance ) - I don't >> have a patch for that yet, but I'm going to need to write one :) - >> presumably you'd like that merging upstream too (and I should make the >> library use the functions provided in qemu_socket.h) ? > > IPv6 would be nice and if you can consolidate that in qemu_socket.h, > then that's a win for non-nbd socket users too. Agreed. Kevin