From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=57503 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pp4HY-000800-R7 for qemu-devel@nongnu.org; Mon, 14 Feb 2011 14:40:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pp4HW-0001DE-SX for qemu-devel@nongnu.org; Mon, 14 Feb 2011 14:40:24 -0500 Received: from comms.lupine.me.uk ([89.16.189.169]:39586 helo=lupine.me.uk) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pp4HW-0001D2-JQ for qemu-devel@nongnu.org; Mon, 14 Feb 2011 14:40:22 -0500 Received: from den.lupine.me.uk ([2001:8b0:174:1::2]) by lupine.me.uk with esmtpsa (SSL3.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.69) (envelope-from ) id 1Pp4HU-0002CB-Mk for qemu-devel@nongnu.org; Mon, 14 Feb 2011 19:40:20 +0000 From: Nicholas Thomas Content-Type: multipart/mixed; boundary="=-Y+2dyPVIUxqz9fJBH2Xu" Date: Mon, 14 Feb 2011 19:40:22 +0000 Message-ID: <1297712422.12551.2.camel@den> Mime-Version: 1.0 Subject: [Qemu-devel] NBD block device backend - 'improvements' Reply-To: nick@lupine.me.uk List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org --=-Y+2dyPVIUxqz9fJBH2Xu Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit [Apologies for the cross-post - I originally sent this to the KVM ML - obviously, it's far more appropriate here] Hi, I've been doing some work with /block/nbd.c with the aim of improving its behaviour when the NBD server is inaccessible or goes away. Current behaviour is to exit on startup if connecting to the NBD server fails for any reason. If the connection dies once KVM is started, the VM stays up but reads and writes fail. No attempt to re-establish the connection is made. 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. 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). 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) ? Regards, -- Nick Thomas Bytemark Computing --=-Y+2dyPVIUxqz9fJBH2Xu Content-Disposition: attachment; filename="01-make-nbd-retry.patch" Content-Type: text/x-patch; name="01-make-nbd-retry.patch"; charset="UTF-8" Content-Transfer-Encoding: 7bit diff --git a/block/nbd.c b/block/nbd.c index c8dc763..87da07e 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -39,9 +39,11 @@ typedef struct BDRVNBDState { int sock; off_t size; size_t blocksize; + char *filename; } BDRVNBDState; -static int nbd_open(BlockDriverState *bs, const char* filename, int flags) + +static int nbd_open(BlockDriverState *bs) { BDRVNBDState *s = bs->opaque; uint32_t nbdflags; @@ -56,7 +58,7 @@ static int nbd_open(BlockDriverState *bs, const char* filename, int flags) int ret; int err = -EINVAL; - file = qemu_strdup(filename); + file = qemu_strdup(s->filename); name = strstr(file, EN_OPTSTR); if (name) { @@ -121,32 +123,62 @@ out: return err; } +// Puts the filename into the driver state and calls nbd_open - hanging until +// it is successful. +static int nbd_setup(BlockDriverState *bs, const char* filename, int flags) +{ + BDRVNBDState *s = bs->opaque; + int err = 1; + + s->filename = qemu_strdup(filename); + while (err != 0) + { + err = nbd_open(bs); + // Avoid tight loops + if (err != 0) + sleep(1); + } + + return err; +} + static int nbd_read(BlockDriverState *bs, int64_t sector_num, uint8_t *buf, int nb_sectors) { BDRVNBDState *s = bs->opaque; struct nbd_request request; struct nbd_reply reply; + bool success = false; request.type = NBD_CMD_READ; request.handle = (uint64_t)(intptr_t)bs; - request.from = sector_num * 512;; + request.from = sector_num * 512; request.len = nb_sectors * 512; - if (nbd_send_request(s->sock, &request) == -1) - return -errno; + while (success == false) + { + if ( (nbd_send_request(s->sock, &request) == -1) || + (nbd_receive_reply(s->sock, &reply) == -1) ) + { + // We hang here until the TCP session is established + close(s->sock); + while(nbd_open(bs) != 0) + sleep(1); + continue; + } - if (nbd_receive_reply(s->sock, &reply) == -1) - return -errno; + if (reply.error !=0) + return -reply.error; - if (reply.error !=0) - return -reply.error; + if (reply.handle != request.handle) + return -EIO; - if (reply.handle != request.handle) - return -EIO; + // If reading the actual data fails, we retry the whole request + if (nbd_wr_sync(s->sock, buf, request.len, 1) != request.len) + continue; - if (nbd_wr_sync(s->sock, buf, request.len, 1) != request.len) - return -EIO; + success = true; + } return 0; } @@ -157,26 +189,39 @@ static int nbd_write(BlockDriverState *bs, int64_t sector_num, BDRVNBDState *s = bs->opaque; struct nbd_request request; struct nbd_reply reply; + bool success = false; request.type = NBD_CMD_WRITE; request.handle = (uint64_t)(intptr_t)bs; request.from = sector_num * 512;; request.len = nb_sectors * 512; - if (nbd_send_request(s->sock, &request) == -1) - return -errno; + while (success == false) + { + if ( (nbd_send_request(s->sock, &request) == -1) || + (nbd_wr_sync(s->sock, (uint8_t*)buf, request.len, 0) != request.len) ) + { + // We hang here until the TCP session is established + close(s->sock); + while(nbd_open(bs) != 0) + sleep(1); + continue; + } + + // We didn't get a reply from the write, so try again + if (nbd_receive_reply(s->sock, &reply) == -1) + continue; - if (nbd_wr_sync(s->sock, (uint8_t*)buf, request.len, 0) != request.len) - return -EIO; + // Problem with the response itself + if (reply.error !=0) + return -reply.error; - if (nbd_receive_reply(s->sock, &reply) == -1) - return -errno; + if (reply.handle != request.handle) + return -EIO; - if (reply.error !=0) - return -reply.error; + success = true; + } - if (reply.handle != request.handle) - return -EIO; return 0; } @@ -191,6 +236,7 @@ static void nbd_close(BlockDriverState *bs) request.from = 0; request.len = 0; nbd_send_request(s->sock, &request); + qemu_free(s->filename); close(s->sock); } @@ -205,7 +251,7 @@ static int64_t nbd_getlength(BlockDriverState *bs) static BlockDriver bdrv_nbd = { .format_name = "nbd", .instance_size = sizeof(BDRVNBDState), - .bdrv_file_open = nbd_open, + .bdrv_file_open = nbd_setup, .bdrv_read = nbd_read, .bdrv_write = nbd_write, .bdrv_close = nbd_close, --=-Y+2dyPVIUxqz9fJBH2Xu--