Re: [Qemu-block] [PATCH v3] block/gluster: Handle changed glfs_ftruncate signature

[Jeff Cody]

On Tue, Jul 31, 2018 at 11:18:02AM +0200, Niels de Vos wrote:
> On Mon, Jul 30, 2018 at 03:27:29PM -0400, Jeff Cody wrote:
> > On Mon, Jul 30, 2018 at 10:07:27AM -0500, Eric Blake wrote:
> > > On 07/28/2018 02:50 AM, Niels de Vos wrote:
> > > >>
> > > >>Part of me wishes that libgfapi had just created a new function
> > > >>'glfs_ftruncate2', so that existing users don't need to handle the api
> > > >>change.  But I guess in the grand scheme, not a huge deal either way.
> > > >
> > > >Gluster uses versioned symbols, so older binaries will keep working with
> > > >new libraries. It is (hopefully) rare that existing symbols get updated.
> > > >We try to send patches for these kind of changes to the projects we know
> > > >well in advance, reducing the number of surprises.
> > > 
> > > >>I can go ahead and add that to the comment in my branch after applying, 
> > > >>if
> > > >>Niels can let me know what that version is/will be (if known).
> > > >
> > > >The new glfs_ftruncate() will be part of glusterfs-5 (planned for
> > > >October). We're changing the numbering scheme, it was expected to come
> > > >in glusterfs-4.2, but that is a version that never will be released.
> > > >
> > > 
> > > Wait - so you're saying gluster has not yet released the incompatible
> > > change? Now would be the right time to get rid of the API breakage, before
> > > you bake it in, rather than relying solely on the versioned symbols to 
> > > avoid
> > > an ABI breakage but forcing all clients to compensate to the API breakage.
> > > 
> > 
> > If this is not yet in a released version of Gluster, I'm not real eager to
> > pollute the QEMU driver codebase with #ifdef's, especially if there is a
> > possibility the API change may not actually materialize.
> > 
> > Is there any reason that this change is being approached in a way that
> > breaks API usage, and is it too late in the Gluster development pipeline to
> > change that?
> 
> There recently have been a few changes like this in libgfapi. These have
> been introduced to improve performance in common use-cases where an
> updated 'struct stat' is needed after an operation. Some functions have
> been adapted in previous releases, glfs_ftruncate() landed too late for
> that. I hope we'll get a complete coherent API with glusterfs-5 again.
> 

Am I correct in assuming the API changes that happened in previous libgfapi
release are for APIs that QEMU does not use (i.e. no action needed from
us?)

> For QEMU this means additional changes will come related to
> glfs_fallocate(), glfs_zerofill() and probably more. The current
> glusterfs-4.1 version will be maintained for one year, after which the
> detection/#ifdef can be removed as the than maintained versions should
> all have the updated API. I'm sorry for the inconvenience that this
> causes.
> 

Understood.  I don't want to make a huge deal out of it.  I guess my
recommendation/request is that API enhancements happen in a way that don't
break existing APIs (e.g. use parallel APIs), because QEMU version usage and
supported glusterfs usage may not always follow supported versions in the
wild.  I know versioned symbols helps with this, but it still complicates
the code (and couples QEMU's code to gluster support windows).


> If you prefer, I can wait with sending patches for QEMU with future
> Gluster releases until additional changes have landed in libgfapi.
>

I think generally, we don't want to start introducing #ifdef's and new APi
usage for unreleased versions of libgfapi if possible (as unreleased APIs,
it could technically change, and then QEMU releases would have 'dead' API
support in it).

If glusterfs-5 releases in October, that may line up with 3.1 or 3.2.


-Jeff

Re: [Qemu-block] qemu NBD client behaviour when device size is not a multiple of 512

[Eric Blake]

On 07/31/2018 01:00 PM, Richard W.M. Jones wrote:


Hi Eric.  Is this a bug?

   $ nbdkit -fv random size=1023

(You can choose any size which is not a multiple of 512.)

   $ qemu-img convert nbd:localhost:10809 /var/tmp/test
   qemu-img: error while reading sector 0: Invalid argument


Or more directly:

$ qemu-io -r -f raw --trace=nbd_\* nbd://localhost:10809
...
422833@1533060368.492178:nbd_receive_negotiate_size_flags Size is 1023, 
export flags 0x3

qemu-io> r -v 0 1023
422833@1533060426.523422:nbd_send_request Sending request to server: { 
.from = 0, .len = 1024, .handle = 94749973992480, .flags = 0x0, .type = 
0 (read) }
422833@1533060426.523632:nbd_receive_simple_reply Got simple reply: { 
.error = 22 (EINVAL), handle = 94749973992480 }

read failed: Invalid argument

Yep. Looks likes the client code is trying to enforce 512-byte 
alignment, and qemu rounds ALL disk sizes up to the next sector boundary 
(even when the actual disk is unaligned).  I'll have to patch things to 
not round past image end on the final partial sector.


Note that with qemu as server:

$ truncate --size=1023 file
$ qemu-nbd -f raw -x '' file

the client now sees:

423068@1533060754.229106:nbd_opt_go_info_block_size Block sizes are 
0x200, 0x1000, 0x200

...
423068@1533060754.229156:nbd_receive_negotiate_size_flags Size is 1024, 
export flags 0x1ed


where qemu as server is 0-padding the source file in all read requests; 
and that probably explains why I haven't noticed the problem with 
unaligned images in the client before (since qemu as NBD server doesn't 
advertise unaligned images).  What's more, a write request to the tail 
of the partial sector (when not using a read-only connection) succeeds, 
and resizes the file in the process (at least, on protocols that support 
live resizing); but I wouldn't count on it behaving sanely for all 
protocols.


What SHOULD happen is that if qemu as server advertises a block size, it 
should truncate the advertised image size DOWN (not round up).  I'm also 
not sure why qemu as server is advertising 512-byte blocks when backed 
by a file - it should be able to advertise 1-byte blocks.  Likewise, 
qemu as client should pay attention to the size advertised by the 
server: if it is not sector-aligned and the server did not advertise 
block size, then assume that byte alignment works; if the server DID 
advertise block size (greater than 1) then qemu should round the image 
size down (as obeying the advertised block size implies that the partial 
sector at the end is inaccessible without violating NBD protocol - 
although the NBD protocol also states that servers SHOULD advertise a 
size that is a multiple of the block size).


At any rate, you've given me plenty to work with for fixing things, 
although it probably won't make it into 3.0-rc3 and I don't know if 
we'll have an -rc4.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[Qemu-block] qemu NBD client behaviour when device size is not a multiple of 512

[Richard W.M. Jones]

Hi Eric.  Is this a bug?

  $ nbdkit -fv random size=1023

(You can choose any size which is not a multiple of 512.)

  $ qemu-img convert nbd:localhost:10809 /var/tmp/test
  qemu-img: error while reading sector 0: Invalid argument

In the nbdkit debugging output:

  nbdkit: random.1: error: invalid request: offset and count are out of range: 
offset=0 count=1024

qemu requested bytes beyond the end of the size of the device.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

[Qemu-block] [PATCH v4 08/10] block/nbd: add cmdline and qapi parameter reconnect-delay

[Vladimir Sementsov-Ogievskiy]

Reconnect will be implemented in the following commit, so for now,
in semantics below, disconnect itself is a "serious error".

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 qapi/block-core.json | 12 +++-
 block/nbd-client.h   |  1 +
 block/nbd-client.c   |  1 +
 block/nbd.c  | 16 +++-
 4 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5b9084a394..cf03402ec5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3511,13 +3511,23 @@
 #  traditional "base:allocation" block status (see
 #  NBD_OPT_LIST_META_CONTEXT in the NBD protocol) (since 3.0)
 #
+# @reconnect-delay: Reconnect delay. On disconnect, nbd client tries to connect
+#   again, until success or serious error. During first
+#   @reconnect-delay seconds of reconnecting loop all requests
+#   are paused and have a chance to rerun, if successful
+#   connect occures during this time. After @reconnect-delay
+#   seconds all delayed requests are failed and all following
+#   requests will be failed to (until successfull reconnect).
+#   Default 300 seconds (Since 3.1)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsNbd',
   'data': { 'server': 'SocketAddress',
 '*export': 'str',
 '*tls-creds': 'str',
-'*x-dirty-bitmap': 'str' } }
+'*x-dirty-bitmap': 'str',
+'*reconnect-delay': 'uint32' } }
 
 ##
 # @BlockdevOptionsRaw:
diff --git a/block/nbd-client.h b/block/nbd-client.h
index e7bda4d3c4..ef8a6a9239 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -53,6 +53,7 @@ int nbd_client_init(BlockDriverState *bs,
 QCryptoTLSCreds *tlscreds,
 const char *hostname,
 const char *x_dirty_bitmap,
+uint32_t reconnect_delay,
 Error **errp);
 void nbd_client_close(BlockDriverState *bs);
 
diff --git a/block/nbd-client.c b/block/nbd-client.c
index c184a9f0e9..41e6e6e702 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -1079,6 +1079,7 @@ int nbd_client_init(BlockDriverState *bs,
 QCryptoTLSCreds *tlscreds,
 const char *hostname,
 const char *x_dirty_bitmap,
+uint32_t reconnect_delay,
 Error **errp)
 {
 NBDClientSession *client = nbd_get_client_session(bs);
diff --git a/block/nbd.c b/block/nbd.c
index 9db5eded89..b40fb5a977 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -360,6 +360,18 @@ static QemuOptsList nbd_runtime_opts = {
 .help = "experimental: expose named dirty bitmap in place of "
 "block status",
 },
+{
+.name = "reconnect-delay",
+.type = QEMU_OPT_NUMBER,
+.help = "Reconnect delay. On disconnect, nbd client tries to"
+"connect again, until success or serious error. During"
+"first @reconnect-delay seconds of reconnecting loop all"
+"requests are paused and have a chance to rerun, if"
+"successful connect occures during this time. After"
+"@reconnect-delay seconds all delayed requests are failed"
+"and all following requests will be failed to (until"
+"successfull reconnect). Default 300 seconds",
+},
 { /* end of list */ }
 },
 };
@@ -411,7 +423,9 @@ static int nbd_open(BlockDriverState *bs, QDict *options, 
int flags,
 
 /* NBD handshake */
 ret = nbd_client_init(bs, s->saddr, s->export, tlscreds, hostname,
-  qemu_opt_get(opts, "x-dirty-bitmap"), errp);
+  qemu_opt_get(opts, "x-dirty-bitmap"),
+  qemu_opt_get_number(opts, "reconnect-delay", 300),
+  errp);
 
  error:
 if (tlscreds) {
-- 
2.11.1

[Qemu-block] [PATCH v4 07/10] block/nbd-client: rename read_reply_co to connection_co

[Vladimir Sementsov-Ogievskiy]

This coroutine will serve nbd reconnects, so, rename it to be something
more generic.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/nbd-client.h |  4 ++--
 block/nbd-client.c | 24 
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index 5367425774..e7bda4d3c4 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -20,7 +20,7 @@
 typedef struct {
 Coroutine *coroutine;
 uint64_t offset;/* original offset of the request */
-bool receiving; /* waiting for read_reply_co? */
+bool receiving; /* waiting for connection_co? */
 } NBDClientRequest;
 
 typedef enum NBDClientState {
@@ -37,7 +37,7 @@ typedef struct NBDClientSession {
 
 CoMutex send_mutex;
 CoQueue free_sema;
-Coroutine *read_reply_co;
+Coroutine *connection_co;
 int in_flight;
 NBDClientState state;
 
diff --git a/block/nbd-client.c b/block/nbd-client.c
index a91fd3ea3e..c184a9f0e9 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -63,7 +63,7 @@ static void nbd_teardown_connection(BlockDriverState *bs)
 qio_channel_shutdown(client->ioc,
  QIO_CHANNEL_SHUTDOWN_BOTH,
  NULL);
-BDRV_POLL_WHILE(bs, client->read_reply_co);
+BDRV_POLL_WHILE(bs, client->connection_co);
 
 nbd_client_detach_aio_context(bs);
 object_unref(OBJECT(client->sioc));
@@ -72,7 +72,7 @@ static void nbd_teardown_connection(BlockDriverState *bs)
 client->ioc = NULL;
 }
 
-static coroutine_fn void nbd_read_reply_entry(void *opaque)
+static coroutine_fn void nbd_connection_entry(void *opaque)
 {
 NBDClientSession *s = opaque;
 uint64_t i;
@@ -105,14 +105,14 @@ static coroutine_fn void nbd_read_reply_entry(void 
*opaque)
 }
 
 /* We're woken up again by the request itself.  Note that there
- * is no race between yielding and reentering read_reply_co.  This
+ * is no race between yielding and reentering connection_co.  This
  * is because:
  *
  * - if the request runs on the same AioContext, it is only
  *   entered after we yield
  *
  * - if the request runs on a different AioContext, reentering
- *   read_reply_co happens through a bottom half, which can only
+ *   connection_co happens through a bottom half, which can only
  *   run after we yield.
  */
 aio_co_wake(s->requests[i].coroutine);
@@ -120,7 +120,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
 }
 
 nbd_recv_coroutines_wake_all(s);
-s->read_reply_co = NULL;
+s->connection_co = NULL;
 }
 
 static int nbd_co_send_request(BlockDriverState *bs,
@@ -428,7 +428,7 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
 }
 *request_ret = 0;
 
-/* Wait until we're woken up by nbd_read_reply_entry.  */
+/* Wait until we're woken up by nbd_connection_entry.  */
 s->requests[i].receiving = true;
 qemu_coroutine_yield();
 s->requests[i].receiving = false;
@@ -503,7 +503,7 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
 }
 
 /* nbd_co_receive_one_chunk
- * Read reply, wake up read_reply_co and set s->quit if needed.
+ * Read reply, wake up connection_co and set s->quit if needed.
  * Return value is a fatal error code or normal nbd reply error code
  */
 static coroutine_fn int nbd_co_receive_one_chunk(
@@ -517,15 +517,15 @@ static coroutine_fn int nbd_co_receive_one_chunk(
 if (ret < 0) {
 nbd_channel_error(s, ret);
 } else {
-/* For assert at loop start in nbd_read_reply_entry */
+/* For assert at loop start in nbd_connection_entry */
 if (reply) {
 *reply = s->reply;
 }
 s->reply.handle = 0;
 }
 
-if (s->read_reply_co) {
-aio_co_wake(s->read_reply_co);
+if (s->connection_co) {
+aio_co_wake(s->connection_co);
 }
 
 return ret;
@@ -966,7 +966,7 @@ void nbd_client_attach_aio_context(BlockDriverState *bs,
 {
 NBDClientSession *client = nbd_get_client_session(bs);
 qio_channel_attach_aio_context(QIO_CHANNEL(client->ioc), new_context);
-aio_co_schedule(new_context, client->read_reply_co);
+aio_co_schedule(new_context, client->connection_co);
 }
 
 void nbd_client_close(BlockDriverState *bs)
@@ -1066,7 +1066,7 @@ static int nbd_client_connect(BlockDriverState *bs,
 /* Now that we're connected, set the socket to be non-blocking and
  * kick the reply mechanism.  */
 qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
-client->read_reply_co = qemu_coroutine_create(nbd_read_reply_entry, 
client);
+client->connection_co = qemu_coroutine_create(nbd_connection_entry, 
client);
 nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));
 
 logout("Established connection with NBD server\n");
-- 
2.11.1

[Qemu-block] [PATCH v4 06/10] block/nbd-client: move from quit to state

[Vladimir Sementsov-Ogievskiy]

To implement reconnect we need several states for the client:
CONNECTED, QUIT and two CONNECTING states. CONNECTING states will
be realized in the following patches. This patch implements CONNECTED
and QUIT.

QUIT means, that we should close the connection and fail all current
and further requests (like old quit = true).

CONNECTED means that connection is ok, we can send requests (like old
quit = false).

For receiving loop we use a comparison of the current state with QUIT,
because reconnect will be in the same loop, so it should be looping
until the end.

Opposite, for requests we use a comparison of the current state with
CONNECTED, as we don't want to send requests in CONNECTING states (
which are unreachable now, but will be reachable after the following
commits)

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/nbd-client.h |  9 -
 block/nbd-client.c | 55 --
 2 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index 2f047ba614..5367425774 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -23,6 +23,13 @@ typedef struct {
 bool receiving; /* waiting for read_reply_co? */
 } NBDClientRequest;
 
+typedef enum NBDClientState {
+NBD_CLIENT_CONNECTING_WAIT,
+NBD_CLIENT_CONNECTING_NOWAIT,
+NBD_CLIENT_CONNECTED,
+NBD_CLIENT_QUIT
+} NBDClientState;
+
 typedef struct NBDClientSession {
 QIOChannelSocket *sioc; /* The master data channel */
 QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
@@ -32,10 +39,10 @@ typedef struct NBDClientSession {
 CoQueue free_sema;
 Coroutine *read_reply_co;
 int in_flight;
+NBDClientState state;
 
 NBDClientRequest requests[MAX_NBD_REQUESTS];
 NBDReply reply;
-bool quit;
 } NBDClientSession;
 
 NBDClientSession *nbd_get_client_session(BlockDriverState *bs);
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 7eaf0149f0..a91fd3ea3e 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -34,6 +34,12 @@
 #define HANDLE_TO_INDEX(bs, handle) ((handle) ^ (uint64_t)(intptr_t)(bs))
 #define INDEX_TO_HANDLE(bs, index)  ((index)  ^ (uint64_t)(intptr_t)(bs))
 
+/* @ret would be used for reconnect in future */
+static void nbd_channel_error(NBDClientSession *s, int ret)
+{
+s->state = NBD_CLIENT_QUIT;
+}
+
 static void nbd_recv_coroutines_wake_all(NBDClientSession *s)
 {
 int i;
@@ -73,14 +79,15 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
 int ret = 0;
 Error *local_err = NULL;
 
-while (!s->quit) {
+while (s->state != NBD_CLIENT_QUIT) {
 assert(s->reply.handle == 0);
 ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);
 if (local_err) {
 error_report_err(local_err);
 }
 if (ret <= 0) {
-break;
+nbd_channel_error(s, ret ? ret : -EIO);
+continue;
 }
 
 /* There's no need for a mutex on the receive side, because the
@@ -93,7 +100,8 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
 !s->requests[i].receiving ||
 (nbd_reply_is_structured(&s->reply) && !s->info.structured_reply))
 {
-break;
+nbd_channel_error(s, -EINVAL);
+continue;
 }
 
 /* We're woken up again by the request itself.  Note that there
@@ -111,7 +119,6 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
 qemu_coroutine_yield();
 }
 
-s->quit = true;
 nbd_recv_coroutines_wake_all(s);
 s->read_reply_co = NULL;
 }
@@ -121,12 +128,18 @@ static int nbd_co_send_request(BlockDriverState *bs,
QEMUIOVector *qiov)
 {
 NBDClientSession *s = nbd_get_client_session(bs);
-int rc, i;
+int rc, i = -1;
 
 qemu_co_mutex_lock(&s->send_mutex);
 while (s->in_flight == MAX_NBD_REQUESTS) {
 qemu_co_queue_wait(&s->free_sema, &s->send_mutex);
 }
+
+if (s->state != NBD_CLIENT_CONNECTED) {
+rc = -EIO;
+goto err;
+}
+
 s->in_flight++;
 
 for (i = 0; i < MAX_NBD_REQUESTS; i++) {
@@ -144,16 +157,12 @@ static int nbd_co_send_request(BlockDriverState *bs,
 
 request->handle = INDEX_TO_HANDLE(s, i);
 
-if (s->quit) {
-rc = -EIO;
-goto err;
-}
 assert(s->ioc);
 
 if (qiov) {
 qio_channel_set_cork(s->ioc, true);
 rc = nbd_send_request(s->ioc, request);
-if (rc >= 0 && !s->quit) {
+if (rc >= 0 && s->state == NBD_CLIENT_CONNECTED) {
 if (qio_channel_writev_all(s->ioc, qiov->iov, qiov->niov,
NULL) < 0) {
 rc = -EIO;
@@ -168,9 +177,11 @@ static int nbd_co_send_request(BlockDriverState *bs,
 
 err:
 if (rc < 0) {
-s->quit = true;
-s->requests[i].coroutine = NULL;
-s->in_flight--;
+nbd_channel_error(s, rc

[Qemu-block] [PATCH v4 00/10] NBD reconnect

[Vladimir Sementsov-Ogievskiy]

Hi all.

Here is NBD reconnect. Previously, if connection failed all current
and future requests will fail. After the series, nbd-client driver
will try to reconnect unlimited times. During first @reconnect-delay
seconds of reconnecting all requests will wait for the connection,
and if it is established requests will be resent. After
@reconnect-delay period all requests will be failed (until successful
reconnect).

v4: - add Eric's r-b to 01.
- drop CONNECTING_INIT mode, don't reconnect on _open.
- new api: only one parameter @reconnect-delay
- new interval scheme between reconnect attempts
(1 - 2 - 4 - 8 - 16 - 16 ... seconds)
- fixes and refactorings in main patch (09), including merge with
  old 08 patch


v3:
06: fix build error in function 'nbd_co_send_request':
 error: 'i' may be used uninitialized in this function

v2 notes:
Here is v2 of NBD reconnect, but it is very very different from v1, so,
forget about v1.
The series includes my "NBD reconnect: preliminary refactoring", with
changes in 05: leave asserts (Eric).

Vladimir Sementsov-Ogievskiy (10):
  block/nbd-client: split channel errors from export errors
  block/nbd: move connection code from block/nbd to block/nbd-client
  block/nbd-client: split connection from initialization
  block/nbd-client: fix nbd_reply_chunk_iter_receive
  block/nbd-client: don't check ioc
  block/nbd-client: move from quit to state
  block/nbd-client: rename read_reply_co to connection_co
  block/nbd: add cmdline and qapi parameter reconnect-delay
  block/nbd-client: nbd reconnect
  iotests: test nbd reconnect

 qapi/block-core.json  |  12 +-
 block/nbd-client.h|  20 +-
 block/nbd-client.c| 515 +++---
 block/nbd.c   |  56 ++---
 tests/qemu-iotests/220|  67 ++
 tests/qemu-iotests/220.out|   7 +
 tests/qemu-iotests/group  |   1 +
 tests/qemu-iotests/iotests.py |   4 +
 8 files changed, 512 insertions(+), 170 deletions(-)
 create mode 100755 tests/qemu-iotests/220
 create mode 100644 tests/qemu-iotests/220.out

-- 
2.11.1

[Qemu-block] [PATCH v4 03/10] block/nbd-client: split connection from initialization

[Vladimir Sementsov-Ogievskiy]

Split connection code to reuse it for reconnect.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/nbd-client.c | 33 -
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index a0d8f2523e..a1813cbfe1 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -999,13 +999,13 @@ static QIOChannelSocket 
*nbd_establish_connection(SocketAddress *saddr,
 return sioc;
 }
 
-int nbd_client_init(BlockDriverState *bs,
-SocketAddress *saddr,
-const char *export,
-QCryptoTLSCreds *tlscreds,
-const char *hostname,
-const char *x_dirty_bitmap,
-Error **errp)
+static int nbd_client_connect(BlockDriverState *bs,
+  SocketAddress *saddr,
+  const char *export,
+  QCryptoTLSCreds *tlscreds,
+  const char *hostname,
+  const char *x_dirty_bitmap,
+  Error **errp)
 {
 NBDClientSession *client = nbd_get_client_session(bs);
 int ret;
@@ -1051,8 +1051,6 @@ int nbd_client_init(BlockDriverState *bs,
 bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP;
 }
 
-qemu_co_mutex_init(&client->send_mutex);
-qemu_co_queue_init(&client->free_sema);
 client->sioc = sioc;
 
 if (!client->ioc) {
@@ -1069,3 +1067,20 @@ int nbd_client_init(BlockDriverState *bs,
 logout("Established connection with NBD server\n");
 return 0;
 }
+
+int nbd_client_init(BlockDriverState *bs,
+SocketAddress *saddr,
+const char *export,
+QCryptoTLSCreds *tlscreds,
+const char *hostname,
+const char *x_dirty_bitmap,
+Error **errp)
+{
+NBDClientSession *client = nbd_get_client_session(bs);
+
+qemu_co_mutex_init(&client->send_mutex);
+qemu_co_queue_init(&client->free_sema);
+
+return nbd_client_connect(bs, saddr, export, tlscreds, hostname,
+  x_dirty_bitmap, errp);
+}
-- 
2.11.1

[Qemu-block] [PATCH v4 09/10] block/nbd-client: nbd reconnect

[Vladimir Sementsov-Ogievskiy]

Implement reconnect. To achieve this:

1. add new modes:
   connecting-wait: means, that reconnecting is in progress, and there
 were small number of reconnect attempts, so all requests are
 waiting for the connection.
   connecting-nowait: reconnecting is in progress, there were a lot of
 attempts of reconnect, all requests will return errors.

   two old modes are used too:
   connected: normal state
   quit: exiting after fatal error or on close

Possible transitions are:

   * -> quit
   connecting-* -> connected
   connecting-wait -> connecting-nowait (transition is done after
  reconnect-delay seconds in connecting-wait mode)
   connected -> connecting-wait

2. Implement reconnect in connection_co. So, in connecting-* mode,
connection_co, tries to reconnect unlimited times.

3. Retry nbd queries on channel error, if we are in connecting-wait
state.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/nbd-client.h |   4 +
 block/nbd-client.c | 304 +++--
 2 files changed, 255 insertions(+), 53 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index ef8a6a9239..52e4ec66be 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -40,6 +40,10 @@ typedef struct NBDClientSession {
 Coroutine *connection_co;
 int in_flight;
 NBDClientState state;
+bool receiving;
+int connect_status;
+Error *connect_err;
+bool wait_in_flight;
 
 NBDClientRequest requests[MAX_NBD_REQUESTS];
 NBDReply reply;
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 41e6e6e702..b09907096d 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -34,10 +34,26 @@
 #define HANDLE_TO_INDEX(bs, handle) ((handle) ^ (uint64_t)(intptr_t)(bs))
 #define INDEX_TO_HANDLE(bs, index)  ((index)  ^ (uint64_t)(intptr_t)(bs))
 
-/* @ret would be used for reconnect in future */
+static int nbd_client_connect(BlockDriverState *bs,
+  SocketAddress *saddr,
+  const char *export,
+  QCryptoTLSCreds *tlscreds,
+  const char *hostname,
+  const char *x_dirty_bitmap,
+  Error **errp);
+
 static void nbd_channel_error(NBDClientSession *s, int ret)
 {
-s->state = NBD_CLIENT_QUIT;
+if (ret == -EIO) {
+if (s->state == NBD_CLIENT_CONNECTED) {
+s->state = NBD_CLIENT_CONNECTING_WAIT;
+}
+} else {
+if (s->state == NBD_CLIENT_CONNECTED) {
+qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+}
+s->state = NBD_CLIENT_QUIT;
+}
 }
 
 static void nbd_recv_coroutines_wake_all(NBDClientSession *s)
@@ -57,33 +73,151 @@ static void nbd_teardown_connection(BlockDriverState *bs)
 {
 NBDClientSession *client = nbd_get_client_session(bs);
 
-assert(client->ioc);
-
-/* finish any pending coroutines */
-qio_channel_shutdown(client->ioc,
- QIO_CHANNEL_SHUTDOWN_BOTH,
- NULL);
+if (client->state == NBD_CLIENT_CONNECTED) {
+/* finish any pending coroutines */
+assert(client->ioc);
+qio_channel_shutdown(client->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+}
+client->state = NBD_CLIENT_QUIT;
 BDRV_POLL_WHILE(bs, client->connection_co);
+}
+
+typedef struct NBDConnection {
+BlockDriverState *bs;
+SocketAddress *saddr;
+const char *export;
+QCryptoTLSCreds *tlscreds;
+const char *hostname;
+const char *x_dirty_bitmap;
+uint32_t reconnect_delay;
+} NBDConnection;
+
+static bool nbd_client_connecting(NBDClientSession *client)
+{
+return client->state == NBD_CLIENT_CONNECTING_WAIT ||
+   client->state == NBD_CLIENT_CONNECTING_NOWAIT;
+}
+
+static bool nbd_client_connecting_wait(NBDClientSession *client)
+{
+return client->state == NBD_CLIENT_CONNECTING_WAIT;
+}
+
+static coroutine_fn void nbd_reconnect_attempt(NBDConnection *con)
+{
+NBDClientSession *s = nbd_get_client_session(con->bs);
+Error *local_err = NULL;
+
+assert(nbd_client_connecting(s));
+
+/* Wait completion of all in-flight requests */
+
+qemu_co_mutex_lock(&s->send_mutex);
+
+while (s->in_flight > 0) {
+qemu_co_mutex_unlock(&s->send_mutex);
+nbd_recv_coroutines_wake_all(s);
+s->wait_in_flight = true;
+qemu_coroutine_yield();
+s->wait_in_flight = false;
+qemu_co_mutex_lock(&s->send_mutex);
+}
+
+qemu_co_mutex_unlock(&s->send_mutex);
+
+/* Now we are sure, that nobody accessing the channel now and nobody
+ * will try to access the channel, until we set state to CONNECTED
+ */
+
+/* Finalize previous connection if any */
+if (s->ioc) {
+nbd_client_detach_aio_context(con->bs);
+object_unref(OBJECT(s->sioc));
+s->sioc = NULL;
+object_unref(OBJECT(s->ioc

[Qemu-block] [PATCH v4 02/10] block/nbd: move connection code from block/nbd to block/nbd-client

[Vladimir Sementsov-Ogievskiy]

Keep all connection code in one file, to be able to implement reconnect
in further patches.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/nbd-client.h |  2 +-
 block/nbd-client.c | 37 +++--
 block/nbd.c| 40 ++--
 3 files changed, 38 insertions(+), 41 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index cfc90550b9..2f047ba614 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -41,7 +41,7 @@ typedef struct NBDClientSession {
 NBDClientSession *nbd_get_client_session(BlockDriverState *bs);
 
 int nbd_client_init(BlockDriverState *bs,
-QIOChannelSocket *sock,
+SocketAddress *saddr,
 const char *export_name,
 QCryptoTLSCreds *tlscreds,
 const char *hostname,
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 1efc3d36cc..a0d8f2523e 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -976,8 +976,31 @@ void nbd_client_close(BlockDriverState *bs)
 nbd_teardown_connection(bs);
 }
 
+static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr,
+  Error **errp)
+{
+QIOChannelSocket *sioc;
+Error *local_err = NULL;
+
+sioc = qio_channel_socket_new();
+qio_channel_set_name(QIO_CHANNEL(sioc), "nbd-client");
+
+qio_channel_socket_connect_sync(sioc,
+saddr,
+&local_err);
+if (local_err) {
+object_unref(OBJECT(sioc));
+error_propagate(errp, local_err);
+return NULL;
+}
+
+qio_channel_set_delay(QIO_CHANNEL(sioc), false);
+
+return sioc;
+}
+
 int nbd_client_init(BlockDriverState *bs,
-QIOChannelSocket *sioc,
+SocketAddress *saddr,
 const char *export,
 QCryptoTLSCreds *tlscreds,
 const char *hostname,
@@ -987,6 +1010,15 @@ int nbd_client_init(BlockDriverState *bs,
 NBDClientSession *client = nbd_get_client_session(bs);
 int ret;
 
+/* establish TCP connection, return error if it fails
+ * TODO: Configurable retry-until-timeout behaviour.
+ */
+QIOChannelSocket *sioc = nbd_establish_connection(saddr, errp);
+
+if (!sioc) {
+return -ECONNREFUSED;
+}
+
 /* NBD handshake */
 logout("session init %s\n", export);
 qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
@@ -1001,12 +1033,14 @@ int nbd_client_init(BlockDriverState *bs,
 g_free(client->info.x_dirty_bitmap);
 if (ret < 0) {
 logout("Failed to negotiate with the NBD server\n");
+object_unref(OBJECT(sioc));
 return ret;
 }
 if (client->info.flags & NBD_FLAG_READ_ONLY &&
 !bdrv_is_read_only(bs)) {
 error_setg(errp,
"request for write access conflicts with read-only export");
+object_unref(OBJECT(sioc));
 return -EACCES;
 }
 if (client->info.flags & NBD_FLAG_SEND_FUA) {
@@ -1020,7 +1054,6 @@ int nbd_client_init(BlockDriverState *bs,
 qemu_co_mutex_init(&client->send_mutex);
 qemu_co_queue_init(&client->free_sema);
 client->sioc = sioc;
-object_ref(OBJECT(client->sioc));
 
 if (!client->ioc) {
 client->ioc = QIO_CHANNEL(sioc);
diff --git a/block/nbd.c b/block/nbd.c
index e87699fb73..9db5eded89 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -295,30 +295,6 @@ NBDClientSession *nbd_get_client_session(BlockDriverState 
*bs)
 return &s->client;
 }
 
-static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr,
-  Error **errp)
-{
-QIOChannelSocket *sioc;
-Error *local_err = NULL;
-
-sioc = qio_channel_socket_new();
-qio_channel_set_name(QIO_CHANNEL(sioc), "nbd-client");
-
-qio_channel_socket_connect_sync(sioc,
-saddr,
-&local_err);
-if (local_err) {
-object_unref(OBJECT(sioc));
-error_propagate(errp, local_err);
-return NULL;
-}
-
-qio_channel_set_delay(QIO_CHANNEL(sioc), false);
-
-return sioc;
-}
-
-
 static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)
 {
 Object *obj;
@@ -394,7 +370,6 @@ static int nbd_open(BlockDriverState *bs, QDict *options, 
int flags,
 BDRVNBDState *s = bs->opaque;
 QemuOpts *opts = NULL;
 Error *local_err = NULL;
-QIOChannelSocket *sioc = NULL;
 QCryptoTLSCreds *tlscreds = NULL;
 const char *hostname = NULL;
 int ret = -EINVAL;
@@ -434,22 +409,11 @@ static int nbd_open(BlockDriverState *bs, QDict *options, 
int flags,
 hostname = s->saddr->u.inet.host;
 }
 
-/* establish TCP connection, return error if it fails
- * TODO: Configurable retry-until-timeout behaviour.
- */
-sioc = nbd_establis

[Qemu-block] [PATCH v4 01/10] block/nbd-client: split channel errors from export errors

[Vladimir Sementsov-Ogievskiy]

To implement nbd reconnect in further patches, we need to distinguish
error codes, returned by nbd server, from channel errors, to reconnect
only in the latter case.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
---
 block/nbd-client.c | 83 +++---
 1 file changed, 47 insertions(+), 36 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 9686ecbd5e..1efc3d36cc 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -501,11 +501,11 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
  */
 static coroutine_fn int nbd_co_receive_one_chunk(
 NBDClientSession *s, uint64_t handle, bool only_structured,
-QEMUIOVector *qiov, NBDReply *reply, void **payload, Error **errp)
+int *request_ret, QEMUIOVector *qiov, NBDReply *reply, void **payload,
+Error **errp)
 {
-int request_ret;
 int ret = nbd_co_do_receive_one_chunk(s, handle, only_structured,
-  &request_ret, qiov, payload, errp);
+  request_ret, qiov, payload, errp);
 
 if (ret < 0) {
 s->quit = true;
@@ -515,7 +515,6 @@ static coroutine_fn int nbd_co_receive_one_chunk(
 *reply = s->reply;
 }
 s->reply.handle = 0;
-ret = request_ret;
 }
 
 if (s->read_reply_co) {
@@ -527,22 +526,17 @@ static coroutine_fn int nbd_co_receive_one_chunk(
 
 typedef struct NBDReplyChunkIter {
 int ret;
-bool fatal;
+int request_ret;
 Error *err;
 bool done, only_structured;
 } NBDReplyChunkIter;
 
-static void nbd_iter_error(NBDReplyChunkIter *iter, bool fatal,
-   int ret, Error **local_err)
+static void nbd_iter_channel_error(NBDReplyChunkIter *iter,
+   int ret, Error **local_err)
 {
 assert(ret < 0);
 
-if ((fatal && !iter->fatal) || iter->ret == 0) {
-if (iter->ret != 0) {
-error_free(iter->err);
-iter->err = NULL;
-}
-iter->fatal = fatal;
+if (!iter->ret) {
 iter->ret = ret;
 error_propagate(&iter->err, *local_err);
 } else {
@@ -552,6 +546,15 @@ static void nbd_iter_error(NBDReplyChunkIter *iter, bool 
fatal,
 *local_err = NULL;
 }
 
+static void nbd_iter_request_error(NBDReplyChunkIter *iter, int ret)
+{
+assert(ret < 0);
+
+if (!iter->request_ret) {
+iter->request_ret = ret;
+}
+}
+
 /* NBD_FOREACH_REPLY_CHUNK
  */
 #define NBD_FOREACH_REPLY_CHUNK(s, iter, handle, structured, \
@@ -567,13 +570,13 @@ static bool nbd_reply_chunk_iter_receive(NBDClientSession 
*s,
  QEMUIOVector *qiov, NBDReply *reply,
  void **payload)
 {
-int ret;
+int ret, request_ret;
 NBDReply local_reply;
 NBDStructuredReplyChunk *chunk;
 Error *local_err = NULL;
 if (s->quit) {
 error_setg(&local_err, "Connection closed");
-nbd_iter_error(iter, true, -EIO, &local_err);
+nbd_iter_channel_error(iter, -EIO, &local_err);
 goto break_loop;
 }
 
@@ -587,10 +590,12 @@ static bool nbd_reply_chunk_iter_receive(NBDClientSession 
*s,
 }
 
 ret = nbd_co_receive_one_chunk(s, handle, iter->only_structured,
-   qiov, reply, payload, &local_err);
+   &request_ret, qiov, reply, payload,
+   &local_err);
 if (ret < 0) {
-/* If it is a fatal error s->quit is set by nbd_co_receive_one_chunk */
-nbd_iter_error(iter, s->quit, ret, &local_err);
+nbd_iter_channel_error(iter, ret, &local_err);
+} else if (request_ret < 0) {
+nbd_iter_request_error(iter, request_ret);
 }
 
 /* Do not execute the body of NBD_FOREACH_REPLY_CHUNK for simple reply. */
@@ -627,7 +632,7 @@ break_loop:
 }
 
 static int nbd_co_receive_return_code(NBDClientSession *s, uint64_t handle,
-  Error **errp)
+  int *request_ret, Error **errp)
 {
 NBDReplyChunkIter iter;
 
@@ -636,12 +641,13 @@ static int nbd_co_receive_return_code(NBDClientSession 
*s, uint64_t handle,
 }
 
 error_propagate(errp, iter.err);
+*request_ret = iter.request_ret;
 return iter.ret;
 }
 
 static int nbd_co_receive_cmdread_reply(NBDClientSession *s, uint64_t handle,
 uint64_t offset, QEMUIOVector *qiov,
-Error **errp)
+int *request_ret, Error **errp)
 {
 NBDReplyChunkIter iter;
 NBDReply reply;
@@ -666,7 +672,7 @@ static int nbd_co_receive_cmdread_reply(NBDClientSession 
*s, uint64_t handle,
 offset, qiov, &local_err);
 if (ret < 0) {
 s->quit = true;
-

[Qemu-block] [PATCH v4 05/10] block/nbd-client: don't check ioc

[Vladimir Sementsov-Ogievskiy]

We have several paranoiac checks for ioc != NULL. But ioc may become
NULL only on close, which should not happen during requests handling.
Also, we check ioc only sometimes, not after each yield, which is
inconsistent. Let's drop these checks. However, for safety, lets leave
asserts instead.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/nbd-client.c | 16 +---
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 263d1721f9..7eaf0149f0 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -51,9 +51,7 @@ static void nbd_teardown_connection(BlockDriverState *bs)
 {
 NBDClientSession *client = nbd_get_client_session(bs);
 
-if (!client->ioc) { /* Already closed */
-return;
-}
+assert(client->ioc);
 
 /* finish any pending coroutines */
 qio_channel_shutdown(client->ioc,
@@ -150,10 +148,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
 rc = -EIO;
 goto err;
 }
-if (!s->ioc) {
-rc = -EPIPE;
-goto err;
-}
+assert(s->ioc);
 
 if (qiov) {
 qio_channel_set_cork(s->ioc, true);
@@ -426,10 +421,11 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
 s->requests[i].receiving = true;
 qemu_coroutine_yield();
 s->requests[i].receiving = false;
-if (!s->ioc || s->quit) {
+if (s->quit) {
 error_setg(errp, "Connection closed");
 return -EIO;
 }
+assert(s->ioc);
 
 assert(s->reply.handle == handle);
 
@@ -967,9 +963,7 @@ void nbd_client_close(BlockDriverState *bs)
 NBDClientSession *client = nbd_get_client_session(bs);
 NBDRequest request = { .type = NBD_CMD_DISC };
 
-if (client->ioc == NULL) {
-return;
-}
+assert(client->ioc);
 
 nbd_send_request(client->ioc, &request);
 
-- 
2.11.1

[Qemu-block] [PATCH v4 10/10] iotests: test nbd reconnect

[Vladimir Sementsov-Ogievskiy]

Add test, which starts backup to nbd target and restarts nbd server
during backup.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/220| 67 +++
 tests/qemu-iotests/220.out|  7 +
 tests/qemu-iotests/group  |  1 +
 tests/qemu-iotests/iotests.py |  4 +++
 4 files changed, 79 insertions(+)
 create mode 100755 tests/qemu-iotests/220
 create mode 100644 tests/qemu-iotests/220.out

diff --git a/tests/qemu-iotests/220 b/tests/qemu-iotests/220
new file mode 100755
index 00..c9702a7dad
--- /dev/null
+++ b/tests/qemu-iotests/220
@@ -0,0 +1,67 @@
+#!/usr/bin/env python
+#
+# Test nbd reconnect
+#
+# Copyright (c) 2018 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import time
+
+import iotests
+from iotests import qemu_img_create, file_path, qemu_nbd_popen
+
+disk_a, disk_b, nbd_sock = file_path('disk_a', 'disk_b', 'nbd-sock')
+nbd_uri = 'nbd+unix:///exp?socket=' + nbd_sock
+
+qemu_img_create('-f', iotests.imgfmt, disk_a, '5M')
+qemu_img_create('-f', iotests.imgfmt, disk_b, '5M')
+srv = qemu_nbd_popen('-k', nbd_sock, '-x', 'exp', '-f', iotests.imgfmt, disk_b)
+time.sleep(1)
+
+vm = iotests.VM().add_drive(disk_a)
+vm.launch()
+vm.hmp_qemu_io('drive0', 'write 0 5M')
+
+print 'blockdev-add:', vm.qmp('blockdev-add', node_name='backup0', 
driver='raw',
+  file={'driver':'nbd',
+'export': 'exp',
+'server': {'type': 'unix',
+   'path': nbd_sock}})
+print 'blockdev-backup:', vm.qmp('blockdev-backup', device='drive0',
+ sync='full', target='backup0')
+
+time.sleep(1)
+print 'Kill NBD server'
+srv.kill()
+
+jobs = vm.qmp('query-block-jobs')['return']
+if jobs and jobs[0]['offset'] < jobs[0]['len']:
+print 'Backup job is still in progress'
+
+time.sleep(1)
+
+print 'Start NBD server'
+srv = qemu_nbd_popen('-k', nbd_sock, '-x', 'exp', '-f', iotests.imgfmt, disk_b)
+
+try:
+e = vm.event_wait('BLOCK_JOB_COMPLETED')
+print e['event'], ':', e['data']
+except:
+pass
+
+print 'blockdev-del:', vm.qmp('blockdev-del', node_name='backup0')
+srv.kill()
+vm.shutdown()
diff --git a/tests/qemu-iotests/220.out b/tests/qemu-iotests/220.out
new file mode 100644
index 00..dae1a49d9f
--- /dev/null
+++ b/tests/qemu-iotests/220.out
@@ -0,0 +1,7 @@
+blockdev-add: {u'return': {}}
+blockdev-backup: {u'return': {}}
+Kill NBD server
+Backup job is still in progress
+Start NBD server
+BLOCK_JOB_COMPLETED : {u'device': u'drive0', u'type': u'backup', u'speed': 0, 
u'len': 5242880, u'offset': 5242880}
+blockdev-del: {u'return': {}}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b973dc842d..ee2473c6a3 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -219,6 +219,7 @@
 217 rw auto quick
 218 rw auto quick
 219 rw auto
+220 rw auto quick
 221 rw auto quick
 222 rw auto quick
 223 rw auto quick
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 4e67fbbe96..17bc8c8e32 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -185,6 +185,10 @@ def qemu_nbd(*args):
 '''Run qemu-nbd in daemon mode and return the parent's exit code'''
 return subprocess.call(qemu_nbd_args + ['--fork'] + list(args))
 
+def qemu_nbd_popen(*args):
+'''Run qemu-nbd in daemon mode and return the parent's exit code'''
+return subprocess.Popen(qemu_nbd_args + ['--persistent'] + list(args))
+
 def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
 '''Return True if two image files are identical'''
 return qemu_img('compare', '-f', fmt1,
-- 
2.11.1

[Qemu-block] [PATCH v4 04/10] block/nbd-client: fix nbd_reply_chunk_iter_receive

[Vladimir Sementsov-Ogievskiy]

Use exported report, not the variable to be reused (should not really
matter).

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/nbd-client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index a1813cbfe1..263d1721f9 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -599,7 +599,7 @@ static bool nbd_reply_chunk_iter_receive(NBDClientSession 
*s,
 }
 
 /* Do not execute the body of NBD_FOREACH_REPLY_CHUNK for simple reply. */
-if (nbd_reply_is_simple(&s->reply) || s->quit) {
+if (nbd_reply_is_simple(reply) || s->quit) {
 goto break_loop;
 }
 
-- 
2.11.1

Re: [Qemu-block] [PATCH] throttle-groups: fix hang when group member leaves

[Alberto Garcia]

On Wed 04 Jul 2018 04:54:10 PM CEST, Stefan Hajnoczi wrote:
> Throttle groups consist of members sharing one throttling state
> (including bps/iops limits).  Round-robin scheduling is used to ensure
> fairness.  If a group member already has a timer pending then other
> groups members do not schedule their own timers.  The next group
> member will have its turn when the existing timer expires.
>
> A hang may occur when a group member leaves while it had a timer
> scheduled.

Ok, I can reproduce this if I run fio with iodepth=1.

We're draining the BDS before removing it from a throttle group, and
therefore there cannot be any pending requests.

So the problem seems to be that when throttle_co_drain_begin() runs the
pending requests from a member using throttle_group_co_restart_queue(),
it simply uses qemu_co_queue_next() and doesn't touch the timer at all.

So it can happen that there's a request in the queue waiting for a
timer, and after that call the request is gone but the timer remains.

The current patch is perhaps not worth touching at this point (we're
about to release QEMU 3.0), but I think that a better solution would be
to either

a) cancel the existing timer and reset tg->any_timer_armed on the given
   tgm after throttle_group_co_restart_queue() and before
   schedule_next_request() if the queue is empty.

b) force the existing timer to run immediately instead of calling
   throttle_group_co_restart_queue(). Seems cleaner, but I haven't tried
   this one yet.

I'll explore them a bit and send a patch.

Berto

Re: [Qemu-block] Python NBD client library

[Richard W.M. Jones]

On Tue, Jul 31, 2018 at 05:48:50PM +0300, Nir Soffer wrote:
> Thanks everyone for the good input on
> http://lists.nongnu.org/archive/html/qemu-block/2018-07/msg00168.html
> 
> I'm convinced that the best direction for oVirt is having NBD client instead
> of using the kernel nbd client. I think a C library would be best, but a
> python
> version is faster to implement and may be good enough for oVirt, so I'm
> starting in this direction.

I think clients in all languages are welcome, though I agree that
eventually we're going to need a C client (and not the Curl track
which I was headed down, which was the wrong direction).

> If you like to check the prototype, see:
> https://gerrit.ovirt.org/c/93384/
> 
> qemu's nbd/client.c was very helpful so far, since NBD doc
> https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md
> is not very clear.

I agree the protocol spec has evolved by accretion so that it no
longer has a coherent narrative.  Anyway some general comments on your
client in no particular order:

- Be nice if it was a separate Python library.

- It should support TCP sockets too.  In fact those are probably more
  important than Unix sockets in the bigger picture.  Is there not a
  Python "socket client" class you can subclass from which does all
  the socket machinery?

- It looks as if you're only handling fixed new-style.  Good!  Don't
  bother implementing anything else.

I would test against ‘qemu-nbd -x’ primarily, then secondarily against
nbd-server and nbdkit.  Once it interoperates with all 3 then you're
good to go :-)

> Not sure that imageio is the best place for this. Do you have a better idea
> about a better home for this library?

A separate Python library in PyPi or wherever would be my preference.
I don't know what the policy is in imageio about dependencies though.

[...]

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

[Qemu-block] Python NBD client library

[Nir Soffer]

Thanks everyone for the good input on
http://lists.nongnu.org/archive/html/qemu-block/2018-07/msg00168.html

I'm convinced that the best direction for oVirt is having NBD client instead
of using the kernel nbd client. I think a C library would be best, but a
python
version is faster to implement and may be good enough for oVirt, so I'm
starting in this direction.

If you like to check the prototype, see:
https://gerrit.ovirt.org/c/93384/

qemu's nbd/client.c was very helpful so far, since NBD doc
https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md
is not very clear.

Not sure that imageio is the best place for this. Do you have a better idea
about a better home for this library?

Here is what we can do now:

$ qemu-img create -f qcow2 test.qcow2 1g
Formatting 'test.qcow2', fmt=qcow2 size=1073741824 cluster_size=65536
lazy_refcounts=off refcount_bits=16

$ qemu-nbd --fork -k /tmp/nbd.sock -t -f qcow2 test.qcow2 -x export -n

$ python3
...
>>> from ovirt_imageio_common import nbd
>>> c = nbd.Client("/tmp/nbd.sock", "export")
>>> c.export_size
1073741824
>>> c.minimum_block_size
1
>>> c.preferred_bloc_ksize
4096
>>> c.maximum_bloc_ksize
33554432

Nir

Re: [Qemu-block] [PATCH] throttle-groups: fix hang when group member leaves

[Alberto Garcia]

On Wed 04 Jul 2018 04:54:10 PM CEST, Stefan Hajnoczi wrote:
> Throttle groups consist of members sharing one throttling state
> (including bps/iops limits).  Round-robin scheduling is used to ensure
> fairness.  If a group member already has a timer pending then other
> groups members do not schedule their own timers.  The next group
> member will have its turn when the existing timer expires.
>
> A hang may occur when a group member leaves while it had a timer
> scheduled.

I haven't been able to reproduce this. When a member is removed from the
group the pending request queue must already be empty, so does this mean
that there's still a timer when the queue is already empty?

Berto

Re: [Qemu-block] [PATCH] Add interactive mode to qemu-img command

[Programmingkid]

> On Jul 31, 2018, at 6:48 AM, Kevin Wolf  wrote:
> 
> Am 30.07.2018 um 22:27 hat Programmingkid geschrieben:
>> On Jul 30, 2018, at 3:39 PM, Max Reitz  wrote:
>>> On 2018-07-30 21:14, John Arbuckle wrote:
>>> What comes to my mind is this: Why don't you write a front-end for
>>> qemu-img?  It'd be trivial to write a script that performs the
>>> interactive mode, offers nice features such as tab completion (because
>>> you can write it in a scripting language, which makes such things easy),
>>> and then feeds the result to qemu-img.
>> 
>> Interesting idea. A cross platform solution could be made using something
>> like Tkinter. 
>> 
>>> The drawback of course is the following: It wouldn't be in qemu-img.  So
>>> you'd need "advertising" for it.  Putting it in the qemu source tree
>>> would be a bit out of the question, because it'd basically be a
>>> management tool, so it should be separate.  And when we're talking about
>>> management tools...  Well, there are already management tools that allow
>>> you image creation with bells and whistles, so, well.
> 
> We do have things like the QMP shell in scripts/, so I don't think
> having this script in the QEMU git tree is completely out of question,
> but I agree that it's not obvious that we should do it.
> 
>> I got the idea of the interactive mode from bochs. It has a very extensive
>> set of options that it walks the user thru. I really liked how complete
>> it was. A management tool would be helpful to the user, but it wouldn't
>> be included with qemu-img. That was my goal.
> 
> That's my biggest problem with your approach: It's not complete at all.
> It only provides the most basic options without which image creation
> would fail.

The three questions are enough to make an image file. I made a image file 
of each of the formats that are listed in the patch.

> But the real advantage in an interactive user interface would be to make
> advanced options more discoverable. All of them. As long as you don't
> provide that, it's useless.

I won't say useless. There are plenty of people who use it to create image
files. But it is very limited in what it can do interactively. Having the
user be able to select any one of the commands available to qemu-img 
would make this patch more useful.

> Everybody knows how to do the basic
> 'qemu-img create -f qcow2 test.qcow2 8G' (and if they don't, they'll
> know after reading the help text or searching the internet for 10
> seconds).

I want to make qemu-img easy enough to use that googling how to use it
becomes a thing of the past.

> Also, it's completely unclear to me why 'qemu-img' without any arguments
> should mean creating an image and not one of the other operations.

I agree now. This patch should provide the user with the full list of 
commands available.

> 
> Kevin

Thank you for reviewing my patch.

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-img-cmds.hx: Add example usage for create command

[Programmingkid]

> On Jul 31, 2018, at 7:57 AM, Eric Blake  wrote:
> 
> On 07/30/2018 09:52 PM, John Arbuckle wrote:
>> Add an example on how to use the create command. I believe this will make 
>> qemu-img easier to use.
>> Signed-off-by: John Arbuckle 
>> ---
>>  qemu-img-cmds.hx | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
>> index 69758fb6e8..92f7437944 100644
>> --- a/qemu-img-cmds.hx
>> +++ b/qemu-img-cmds.hx
>> @@ -50,7 +50,7 @@ STEXI
>>  ETEXI
>>DEF("create", img_create,
>> -"create [--object objectdef] [-q] [-f fmt] [-b backing_file] [-F 
>> backing_fmt] [-u] [-o options] filename [size]")
>> +"create [--object objectdef] [-q] [-f fmt] [-b backing_file] [-F 
>> backing_fmt] [-u] [-o options] filename [size]\nExample: qemu-img create -f 
>> qcow2 WindowsXP.qcow2 10G")
> 
> Making a long line longer. It would be worth using C string concatenation and 
> splitting this over two lines, at the \n.

Sounds like a good idea.

> Using the name WindowsXP.qcow2 as the guest is somewhat misleading (that OS 
> is proprietary, and quickly reaching the point of obsolescence from its 
> vendor - furthermore, qemu-img doesn't actually install an OS, but rather 
> creates a blank image for a later install process to utilize); better would 
> be a generic name that won't go out of date, such 'image.qcow2'.

I always felt a concrete example was easier to understand rather than a generic 
example. What about this: 

Example: qemu-img create -f qcow2 .qcow2 10G

> 
>>  STEXI
>>  @item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-b 
>> @var{backing_file}] [-F @var{backing_fmt}] [-u] [-o @var{options}] 
>> @var{filename} [@var{size}]
>>  ETEXI
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-img-cmds.hx: Add example usage for create command

[Programmingkid]

> On Jul 31, 2018, at 8:35 AM, Kevin Wolf  wrote:
> 
> Am 31.07.2018 um 13:57 hat Eric Blake geschrieben:
>> On 07/30/2018 09:52 PM, John Arbuckle wrote:
>>> Add an example on how to use the create command. I believe this will make 
>>> qemu-img easier to use.
>>> 
>>> Signed-off-by: John Arbuckle 
>>> ---
>>>  qemu-img-cmds.hx | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
>>> index 69758fb6e8..92f7437944 100644
>>> --- a/qemu-img-cmds.hx
>>> +++ b/qemu-img-cmds.hx
>>> @@ -50,7 +50,7 @@ STEXI
>>>  ETEXI
>>>  DEF("create", img_create,
>>> -"create [--object objectdef] [-q] [-f fmt] [-b backing_file] [-F 
>>> backing_fmt] [-u] [-o options] filename [size]")
>>> +"create [--object objectdef] [-q] [-f fmt] [-b backing_file] [-F 
>>> backing_fmt] [-u] [-o options] filename [size]\nExample: qemu-img create -f 
>>> qcow2 WindowsXP.qcow2 10G")
>> 
>> Making a long line longer. It would be worth using C string concatenation
>> and splitting this over two lines, at the \n.
>> 
>> Using the name WindowsXP.qcow2 as the guest is somewhat misleading (that OS
>> is proprietary, and quickly reaching the point of obsolescence from its
>> vendor - furthermore, qemu-img doesn't actually install an OS, but rather
>> creates a blank image for a later install process to utilize); better would
>> be a generic name that won't go out of date, such 'image.qcow2'.
> 
> Also, while I like the idea of adding examples to the man page, I don't
> think adding it here would give the right result. The example would
> appear in the middle of the subcommand syntax lines.

As it reads now I don't feel its easy for the user to decipher. Having an
example near by would help the user understand how to use it.

> I'd rather add a whole new section "EXAMPLES" in the man page.

That is a good idea. I would like to have examples in both documents. 

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-img-cmds.hx: Add example usage for create command

[Kevin Wolf]

Am 31.07.2018 um 13:57 hat Eric Blake geschrieben:
> On 07/30/2018 09:52 PM, John Arbuckle wrote:
> > Add an example on how to use the create command. I believe this will make 
> > qemu-img easier to use.
> > 
> > Signed-off-by: John Arbuckle 
> > ---
> >   qemu-img-cmds.hx | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> > index 69758fb6e8..92f7437944 100644
> > --- a/qemu-img-cmds.hx
> > +++ b/qemu-img-cmds.hx
> > @@ -50,7 +50,7 @@ STEXI
> >   ETEXI
> >   DEF("create", img_create,
> > -"create [--object objectdef] [-q] [-f fmt] [-b backing_file] [-F 
> > backing_fmt] [-u] [-o options] filename [size]")
> > +"create [--object objectdef] [-q] [-f fmt] [-b backing_file] [-F 
> > backing_fmt] [-u] [-o options] filename [size]\nExample: qemu-img create -f 
> > qcow2 WindowsXP.qcow2 10G")
> 
> Making a long line longer. It would be worth using C string concatenation
> and splitting this over two lines, at the \n.
> 
> Using the name WindowsXP.qcow2 as the guest is somewhat misleading (that OS
> is proprietary, and quickly reaching the point of obsolescence from its
> vendor - furthermore, qemu-img doesn't actually install an OS, but rather
> creates a blank image for a later install process to utilize); better would
> be a generic name that won't go out of date, such 'image.qcow2'.

Also, while I like the idea of adding examples to the man page, I don't
think adding it here would give the right result. The example would
appear in the middle of the subcommand syntax lines.

I'd rather add a whole new section "EXAMPLES" in the man page.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-img-cmds.hx: Add example usage for create command

[Eric Blake]

On 07/30/2018 09:52 PM, John Arbuckle wrote:

Add an example on how to use the create command. I believe this will make 
qemu-img easier to use.

Signed-off-by: John Arbuckle 
---
  qemu-img-cmds.hx | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 69758fb6e8..92f7437944 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -50,7 +50,7 @@ STEXI
  ETEXI
  
  DEF("create", img_create,

-"create [--object objectdef] [-q] [-f fmt] [-b backing_file] [-F backing_fmt] 
[-u] [-o options] filename [size]")
+"create [--object objectdef] [-q] [-f fmt] [-b backing_file] [-F backing_fmt] 
[-u] [-o options] filename [size]\nExample: qemu-img create -f qcow2 WindowsXP.qcow2 
10G")


Making a long line longer. It would be worth using C string 
concatenation and splitting this over two lines, at the \n.


Using the name WindowsXP.qcow2 as the guest is somewhat misleading (that 
OS is proprietary, and quickly reaching the point of obsolescence from 
its vendor - furthermore, qemu-img doesn't actually install an OS, but 
rather creates a blank image for a later install process to utilize); 
better would be a generic name that won't go out of date, such 
'image.qcow2'.




  STEXI
  @item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-b 
@var{backing_file}] [-F @var{backing_fmt}] [-u] [-o @var{options}] 
@var{filename} [@var{size}]
  ETEXI



--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH] Add interactive mode to qemu-img command

[Kevin Wolf]

Am 30.07.2018 um 22:27 hat Programmingkid geschrieben:
> On Jul 30, 2018, at 3:39 PM, Max Reitz  wrote:
> > On 2018-07-30 21:14, John Arbuckle wrote:
> > What comes to my mind is this: Why don't you write a front-end for
> > qemu-img?  It'd be trivial to write a script that performs the
> > interactive mode, offers nice features such as tab completion (because
> > you can write it in a scripting language, which makes such things easy),
> > and then feeds the result to qemu-img.
> 
> Interesting idea. A cross platform solution could be made using something
> like Tkinter. 
> 
> > The drawback of course is the following: It wouldn't be in qemu-img.  So
> > you'd need "advertising" for it.  Putting it in the qemu source tree
> > would be a bit out of the question, because it'd basically be a
> > management tool, so it should be separate.  And when we're talking about
> > management tools...  Well, there are already management tools that allow
> > you image creation with bells and whistles, so, well.

We do have things like the QMP shell in scripts/, so I don't think
having this script in the QEMU git tree is completely out of question,
but I agree that it's not obvious that we should do it.

> I got the idea of the interactive mode from bochs. It has a very extensive
> set of options that it walks the user thru. I really liked how complete
> it was. A management tool would be helpful to the user, but it wouldn't
> be included with qemu-img. That was my goal.

That's my biggest problem with your approach: It's not complete at all.
It only provides the most basic options without which image creation
would fail.

But the real advantage in an interactive user interface would be to make
advanced options more discoverable. All of them. As long as you don't
provide that, it's useless. Everybody knows how to do the basic
'qemu-img create -f qcow2 test.qcow2 8G' (and if they don't, they'll
know after reading the help text or searching the internet for 10
seconds).

Also, it's completely unclear to me why 'qemu-img' without any arguments
should mean creating an image and not one of the other operations.

Kevin

Re: [Qemu-block] [PATCH v3] block/gluster: Handle changed glfs_ftruncate signature

[Niels de Vos]

On Mon, Jul 30, 2018 at 03:27:29PM -0400, Jeff Cody wrote:
> On Mon, Jul 30, 2018 at 10:07:27AM -0500, Eric Blake wrote:
> > On 07/28/2018 02:50 AM, Niels de Vos wrote:
> > >>
> > >>Part of me wishes that libgfapi had just created a new function
> > >>'glfs_ftruncate2', so that existing users don't need to handle the api
> > >>change.  But I guess in the grand scheme, not a huge deal either way.
> > >
> > >Gluster uses versioned symbols, so older binaries will keep working with
> > >new libraries. It is (hopefully) rare that existing symbols get updated.
> > >We try to send patches for these kind of changes to the projects we know
> > >well in advance, reducing the number of surprises.
> > 
> > >>I can go ahead and add that to the comment in my branch after applying, if
> > >>Niels can let me know what that version is/will be (if known).
> > >
> > >The new glfs_ftruncate() will be part of glusterfs-5 (planned for
> > >October). We're changing the numbering scheme, it was expected to come
> > >in glusterfs-4.2, but that is a version that never will be released.
> > >
> > 
> > Wait - so you're saying gluster has not yet released the incompatible
> > change? Now would be the right time to get rid of the API breakage, before
> > you bake it in, rather than relying solely on the versioned symbols to avoid
> > an ABI breakage but forcing all clients to compensate to the API breakage.
> > 
> 
> If this is not yet in a released version of Gluster, I'm not real eager to
> pollute the QEMU driver codebase with #ifdef's, especially if there is a
> possibility the API change may not actually materialize.
> 
> Is there any reason that this change is being approached in a way that
> breaks API usage, and is it too late in the Gluster development pipeline to
> change that?

There recently have been a few changes like this in libgfapi. These have
been introduced to improve performance in common use-cases where an
updated 'struct stat' is needed after an operation. Some functions have
been adapted in previous releases, glfs_ftruncate() landed too late for
that. I hope we'll get a complete coherent API with glusterfs-5 again.

For QEMU this means additional changes will come related to
glfs_fallocate(), glfs_zerofill() and probably more. The current
glusterfs-4.1 version will be maintained for one year, after which the
detection/#ifdef can be removed as the than maintained versions should
all have the updated API. I'm sorry for the inconvenience that this
causes.

If you prefer, I can wait with sending patches for QEMU with future
Gluster releases until additional changes have landed in libgfapi.

Thanks,
Niels

Re: [Qemu-block] [PULL 00/13] Block layer patches

[Peter Maydell]

On 30 July 2018 at 16:09, Kevin Wolf  wrote:
> The following changes since commit 6d9dd5fb9d0e9f4a174f53a0e20a39fbe809c71e:
>
>   Merge remote-tracking branch 
> 'remotes/armbru/tags/pull-qobject-2018-07-27-v2' into staging (2018-07-30 
> 09:55:47 +0100)
>
> are available in the git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 1239ac241fe170bb9fcf0be74bfff04f6f1c2560:
>
>   qemu-iotests: Test query-blockstats with -drive and -blockdev (2018-07-30 
> 15:35:37 +0200)
>
> 
> Block layer patches:
>
> - qemu-img convert -C is now required to enable copy offloading
> - file-posix: Fix write_zeroes with unmap on block devices (would fall
>   back to explicit writes on recent kernels)
> - Fix query-blockstats interface for use with -blockdev
> - Minor fixes and documentation updates
>
> 

Applied, thanks.

-- PMM