Re: [Qemu-devel] NBD block device backend - 'improvements'

[Kevin Wolf]

Am 14.02.2011 21:32, schrieb Stefan Hajnoczi:
> On Mon, Feb 14, 2011 at 7:40 PM, Nicholas Thomas <address@hidden> 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