From 5a8a30db4771675480829d7d3bf35a138e9c35f1 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 12 Mar 2014 15:59:16 +0100 Subject: [PATCH 1/8] block: Add error handling to bdrv_invalidate_cache() If it returns an error, the migrated VM will not be started, but qemu exits with an error message. Signed-off-by: Kevin Wolf Reviewed-by: Juan Quintela Reviewed-by: Eric Blake Reviewed-by: Benoit Canet --- block.c | 28 ++++++++++++++++++++++------ block/qcow2.c | 22 +++++++++++++++++++--- block/qed.c | 21 ++++++++++++++++++--- block/quorum.c | 9 +++++++-- include/block/block.h | 4 ++-- include/block/block_int.h | 2 +- migration.c | 8 +++++++- 7 files changed, 76 insertions(+), 18 deletions(-) diff --git a/block.c b/block.c index 53f5b44fbb..acb70fde3d 100644 --- a/block.c +++ b/block.c @@ -4781,27 +4781,43 @@ flush_parent: return bdrv_co_flush(bs->file); } -void bdrv_invalidate_cache(BlockDriverState *bs) +void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) { + Error *local_err = NULL; + int ret; + if (!bs->drv) { return; } if (bs->drv->bdrv_invalidate_cache) { - bs->drv->bdrv_invalidate_cache(bs); + bs->drv->bdrv_invalidate_cache(bs, &local_err); } else if (bs->file) { - bdrv_invalidate_cache(bs->file); + bdrv_invalidate_cache(bs->file, &local_err); + } + if (local_err) { + error_propagate(errp, local_err); + return; } - refresh_total_sectors(bs, bs->total_sectors); + ret = refresh_total_sectors(bs, bs->total_sectors); + if (ret < 0) { + error_setg_errno(errp, -ret, "Could not refresh total sector count"); + return; + } } -void bdrv_invalidate_cache_all(void) +void bdrv_invalidate_cache_all(Error **errp) { BlockDriverState *bs; + Error *local_err = NULL; QTAILQ_FOREACH(bs, &bdrv_states, device_list) { - bdrv_invalidate_cache(bs); + bdrv_invalidate_cache(bs, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } } } diff --git a/block/qcow2.c b/block/qcow2.c index 945c9d6334..b9dc960bd1 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1156,7 +1156,7 @@ static void qcow2_close(BlockDriverState *bs) qcow2_free_snapshots(bs); } -static void qcow2_invalidate_cache(BlockDriverState *bs) +static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp) { BDRVQcowState *s = bs->opaque; int flags = s->flags; @@ -1164,6 +1164,8 @@ static void qcow2_invalidate_cache(BlockDriverState *bs) AES_KEY aes_decrypt_key; uint32_t crypt_method = 0; QDict *options; + Error *local_err = NULL; + int ret; /* * Backing files are read-only which makes all of their metadata immutable, @@ -1178,11 +1180,25 @@ static void qcow2_invalidate_cache(BlockDriverState *bs) qcow2_close(bs); - bdrv_invalidate_cache(bs->file); + bdrv_invalidate_cache(bs->file, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } memset(s, 0, sizeof(BDRVQcowState)); options = qdict_clone_shallow(bs->options); - qcow2_open(bs, options, flags, NULL); + + ret = qcow2_open(bs, options, flags, &local_err); + if (local_err) { + error_setg(errp, "Could not reopen qcow2 layer: %s", + error_get_pretty(local_err)); + error_free(local_err); + return; + } else if (ret < 0) { + error_setg_errno(errp, -ret, "Could not reopen qcow2 layer"); + return; + } QDECREF(options); diff --git a/block/qed.c b/block/qed.c index 837accd39b..3bd9db9c85 100644 --- a/block/qed.c +++ b/block/qed.c @@ -1558,16 +1558,31 @@ static int bdrv_qed_change_backing_file(BlockDriverState *bs, return ret; } -static void bdrv_qed_invalidate_cache(BlockDriverState *bs) +static void bdrv_qed_invalidate_cache(BlockDriverState *bs, Error **errp) { BDRVQEDState *s = bs->opaque; + Error *local_err = NULL; + int ret; bdrv_qed_close(bs); - bdrv_invalidate_cache(bs->file); + bdrv_invalidate_cache(bs->file, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } memset(s, 0, sizeof(BDRVQEDState)); - bdrv_qed_open(bs, NULL, bs->open_flags, NULL); + ret = bdrv_qed_open(bs, NULL, bs->open_flags, &local_err); + if (local_err) { + error_setg(errp, "Could not reopen qed layer: %s", + error_get_pretty(local_err)); + error_free(local_err); + return; + } else if (ret < 0) { + error_setg_errno(errp, -ret, "Could not reopen qed layer"); + return; + } } static int bdrv_qed_check(BlockDriverState *bs, BdrvCheckResult *result, diff --git a/block/quorum.c b/block/quorum.c index 33bf2ae6a7..7f580a83b5 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -625,13 +625,18 @@ static int64_t quorum_getlength(BlockDriverState *bs) return result; } -static void quorum_invalidate_cache(BlockDriverState *bs) +static void quorum_invalidate_cache(BlockDriverState *bs, Error **errp) { BDRVQuorumState *s = bs->opaque; + Error *local_err = NULL; int i; for (i = 0; i < s->num_children; i++) { - bdrv_invalidate_cache(s->bs[i]); + bdrv_invalidate_cache(s->bs[i], &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } } } diff --git a/include/block/block.h b/include/block/block.h index bd34d14109..1ed55d839a 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -329,8 +329,8 @@ BlockDriverAIOCB *bdrv_aio_ioctl(BlockDriverState *bs, BlockDriverCompletionFunc *cb, void *opaque); /* Invalidate any cached metadata used by image formats */ -void bdrv_invalidate_cache(BlockDriverState *bs); -void bdrv_invalidate_cache_all(void); +void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp); +void bdrv_invalidate_cache_all(Error **errp); void bdrv_clear_incoming_migration_all(void); diff --git a/include/block/block_int.h b/include/block/block_int.h index 4fc5ea8a65..cd5bc7308a 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -153,7 +153,7 @@ struct BlockDriver { /* * Invalidate any cached meta-data. */ - void (*bdrv_invalidate_cache)(BlockDriverState *bs); + void (*bdrv_invalidate_cache)(BlockDriverState *bs, Error **errp); /* * Flushes all data that was already written to the OS all the way down to diff --git a/migration.c b/migration.c index 00f465ea46..e0e24d42c7 100644 --- a/migration.c +++ b/migration.c @@ -101,6 +101,7 @@ void qemu_start_incoming_migration(const char *uri, Error **errp) static void process_incoming_migration_co(void *opaque) { QEMUFile *f = opaque; + Error *local_err = NULL; int ret; ret = qemu_loadvm_state(f); @@ -115,7 +116,12 @@ static void process_incoming_migration_co(void *opaque) bdrv_clear_incoming_migration_all(); /* Make sure all file formats flush their mutable metadata */ - bdrv_invalidate_cache_all(); + bdrv_invalidate_cache_all(&local_err); + if (local_err) { + qerror_report_err(local_err); + error_free(local_err); + exit(EXIT_FAILURE); + } if (autostart) { vm_start(); From 6e6507c06beb28e0551f68b13aa843ed1303f0f0 Mon Sep 17 00:00:00 2001 From: Maria Kustova Date: Tue, 18 Mar 2014 09:59:17 +0400 Subject: [PATCH 2/8] qemu-io-cmds: Fixed typo in example for writev. Signed-off-by: Maria Kustova Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- qemu-io-cmds.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index fb1db53c6b..60c1cebffc 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -1087,7 +1087,7 @@ writev_help(void) " writes a range of bytes from the given offset source from multiple buffers\n" "\n" " Example:\n" -" 'write 512 1k 1k' - writes 2 kilobytes at 512 bytes into the open file\n" +" 'writev 512 1k 1k' - writes 2 kilobytes at 512 bytes into the open file\n" "\n" " Writes into a segment of the currently open file, using a buffer\n" " filled with a set pattern (0xcdcdcdcd).\n" From d208cc353a2860dc9614ba651439713e4ecdf9d3 Mon Sep 17 00:00:00 2001 From: Maria Kustova Date: Tue, 18 Mar 2014 09:59:19 +0400 Subject: [PATCH 3/8] qemu-io: Extended "--cmd" description in usage text It's not clear from the usage description that "--cmd" option accepts its argument as a string, so any special symbols have to be quoted from the shell. Updates in usage text: - Specified parameter format for "--cmd" option. - Added an instruction how to get help for "--cmd" option. Signed-off-by: Maria Kustova Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- qemu-io.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/qemu-io.c b/qemu-io.c index 2d119c28a6..5d7b53f756 100644 --- a/qemu-io.c +++ b/qemu-io.c @@ -193,10 +193,11 @@ static const cmdinfo_t quit_cmd = { static void usage(const char *name) { printf( -"Usage: %s [-h] [-V] [-rsnm] [-c cmd] ... [file]\n" +"Usage: %s [-h] [-V] [-rsnm] [-c STRING] ... [file]\n" "QEMU Disk exerciser\n" "\n" -" -c, --cmd command to execute\n" +" -c, --cmd STRING execute command with its arguments\n" +" from the given string\n" " -r, --read-only export read-only\n" " -s, --snapshot use snapshot file\n" " -n, --nocache disable host cache\n" @@ -207,8 +208,10 @@ static void usage(const char *name) " -T, --trace FILE enable trace events listed in the given file\n" " -h, --help display this help and exit\n" " -V, --version output version information and exit\n" +"\n" +"See '%s -c help' for information on available commands." "\n", - name); + name, name); } static char *get_prompt(void) From 8a15b813e6034856d4177c6ab242791795434c15 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Mon, 17 Mar 2014 23:04:51 +0100 Subject: [PATCH 4/8] qcow2: Correct comment for realloc_refcount_block() Contrary to the comment describing this function's behavior, it does not return 0 on success, but rather the offset of the newly allocated cluster. This patch adjusts the comment accordingly to reflect the actual behavior. Signed-off-by: Max Reitz Reviewed-by: Laszlo Ersek Signed-off-by: Kevin Wolf --- block/qcow2-refcount.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 6151148507..3f2ed08466 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -1383,7 +1383,7 @@ static int write_reftable_entry(BlockDriverState *bs, int rt_index) * does _not_ decrement the reference count for the currently occupied cluster. * * This function prints an informative message to stderr on error (and returns - * -errno); on success, 0 is returned. + * -errno); on success, the offset of the newly allocated cluster is returned. */ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index, uint64_t offset) From a134d90f50806597c5da4fd191352fe62d40f71a Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Mon, 17 Mar 2014 23:04:52 +0100 Subject: [PATCH 5/8] qcow2: Fix fail path in realloc_refcount_block() If qcow2_alloc_clusters() fails, new_offset and ret will both be negative after the fail label, thus passing the first if condition and subsequently resulting in a call of qcow2_free_clusters() with an invalid (negative) offset parameter. Fix this by introducing a new label "fail_free_cluster" which is only invoked if new_offset is indeed pointing to a newly allocated cluster that should be cleaned up by freeing it. While we're at it, clean up the whole fail path. qcow2_cache_put() should (and actually can) never fail, hence the return value can safely be ignored (aside from asserting that it indeed did not fail). Furthermore, there is no reason to give QCOW2_DISCARD_ALWAYS to qcow2_free_clusters(), a mere QCOW2_DISCARD_OTHER will suffice. Ultimately, rename the "fail" label to "done", as it is invoked both on failure and success. Suggested-by: Laszlo Ersek Signed-off-by: Max Reitz Reviewed-by: Laszlo Ersek Signed-off-by: Kevin Wolf --- block/qcow2-refcount.c | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 3f2ed08466..4a2df5fb99 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -1399,14 +1399,14 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index, fprintf(stderr, "Could not allocate new cluster: %s\n", strerror(-new_offset)); ret = new_offset; - goto fail; + goto done; } /* fetch current refcount block content */ ret = qcow2_cache_get(bs, s->refcount_block_cache, offset, &refcount_block); if (ret < 0) { fprintf(stderr, "Could not fetch refcount block: %s\n", strerror(-ret)); - goto fail; + goto fail_free_cluster; } /* new block has not yet been entered into refcount table, therefore it is @@ -1417,8 +1417,7 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index, "check failed: %s\n", strerror(-ret)); /* the image will be marked corrupt, so don't even attempt on freeing * the cluster */ - new_offset = 0; - goto fail; + goto done; } /* write to new block */ @@ -1426,7 +1425,7 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index, s->cluster_sectors); if (ret < 0) { fprintf(stderr, "Could not write refcount block: %s\n", strerror(-ret)); - goto fail; + goto fail_free_cluster; } /* update refcount table */ @@ -1436,24 +1435,27 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index, if (ret < 0) { fprintf(stderr, "Could not update refcount table: %s\n", strerror(-ret)); - goto fail; + goto fail_free_cluster; } -fail: - if (new_offset && (ret < 0)) { - qcow2_free_clusters(bs, new_offset, s->cluster_size, - QCOW2_DISCARD_ALWAYS); - } + goto done; + +fail_free_cluster: + qcow2_free_clusters(bs, new_offset, s->cluster_size, QCOW2_DISCARD_OTHER); + +done: if (refcount_block) { - if (ret < 0) { - qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block); - } else { - ret = qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block); - } + /* This should never fail, as it would only do so if the given refcount + * block cannot be found in the cache. As this is impossible as long as + * there are no bugs, assert the success. */ + int tmp = qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block); + assert(tmp == 0); } + if (ret < 0) { return ret; } + return new_offset; } From b7d769c93214bd6e58d16009f47e61ccb541025c Mon Sep 17 00:00:00 2001 From: Peter Lieven Date: Mon, 17 Mar 2014 09:37:33 +0100 Subject: [PATCH 6/8] block/nfs: bump libnfs requirement to 1.9.3 libnfs prior to 1.9.3 contains a bug that will report wrong transfer sizes if the file offset grows beyond 4GB and RPC responses are received out of order. this error is not detectable and fixable in qemu. additionally 1.9.3 introduces support for handling short read/writes in general and takes care of the necessary retransmissions internally. Signed-off-by: Peter Lieven Signed-off-by: Kevin Wolf --- configure | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure b/configure index 2bc6b770e2..aae617e765 100755 --- a/configure +++ b/configure @@ -3868,7 +3868,7 @@ fi ########################################## # Do we have libnfs if test "$libnfs" != "no" ; then - if $pkg_config --atleast-version=1.9.2 libnfs; then + if $pkg_config --atleast-version=1.9.3 libnfs; then libnfs="yes" libnfs_libs=$($pkg_config --libs libnfs) LIBS="$LIBS $libnfs_libs" From 20fccb187c54105177a7859360e3cb7166c8f22f Mon Sep 17 00:00:00 2001 From: Peter Lieven Date: Mon, 17 Mar 2014 09:37:21 +0100 Subject: [PATCH 7/8] block/nfs: report errors from libnfs if an NFS operation fails we should report what libnfs knows about the failure. It is likely more than just an error code. Signed-off-by: Peter Lieven Signed-off-by: Kevin Wolf --- block/nfs.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/block/nfs.c b/block/nfs.c index ef731f04e3..98aa363e48 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -112,6 +112,9 @@ nfs_co_generic_cb(int ret, struct nfs_context *nfs, void *data, if (task->ret == 0 && task->st) { memcpy(task->st, data, sizeof(struct stat)); } + if (task->ret < 0) { + error_report("NFS Error: %s", nfs_get_error(nfs)); + } if (task->co) { task->bh = qemu_bh_new(nfs_co_generic_bh_cb, task); qemu_bh_schedule(task->bh); From 198fd05c357afff22f0b0e02639937519ed49b1f Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 18 Mar 2014 17:50:52 +0100 Subject: [PATCH 8/8] dataplane: fix implicit IOThread refcount When creating an IOThread implicitly (the user did not specify x-iothread=) remember that iothread_find() does not return the object with an incremented refcount. Signed-off-by: Stefan Hajnoczi Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- hw/block/dataplane/virtio-blk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index a5afc217c0..f558b45a60 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -393,7 +393,6 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk, if (blk->iothread) { s->internal_iothread = false; s->iothread = blk->iothread; - object_ref(OBJECT(s->iothread)); } else { /* Create per-device IOThread if none specified */ Error *local_err = NULL; @@ -408,6 +407,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk, s->iothread = iothread_find(vdev->name); assert(s->iothread); } + object_ref(OBJECT(s->iothread)); s->ctx = iothread_get_aio_context(s->iothread); /* Prevent block operations that conflict with data plane thread */