Skip to contentSkip to conversation

Patches for 0.12.4

GIT PULL New Kevin Wolf 2 participants 12 messages Apr 9, 2010

lore
Share & tools
lore

Kevin Wolf

author
#0 lore raw

Hi Aurelien,

I went through the patches that were committed recently, either written by me or concerning block stuff (most of them are both). I came up with this list of patches that should go into 0.12.4. The first half of them is contained in current master (you'll see the cherry-pick line in the commit log for those), the rest has already been posted to the list, but is still sitting in my block queue and waiting for Anthony's return.

Kevin

The following changes since commit 804b6ab08d82adcab05d64362be91c48a7467fd4:
Paul Brook (1):
UHCI spurious interrut fix

are available in the git repository at:

git://repo.or.cz/qemu/kevin.git for-aurelien-0.12

Christoph Hellwig (1):
block: avoid creating too large iovecs in multiwrite_merge

Gerd Hoffmann (1):
scsi-disk: fix buffer overflow

Kevin Wolf (8):
json-parser: Fix segfault on malformed input
qcow2: Factor next_refcount_table_size out
qcow2: Rewrite alloc_refcount_block/grow_refcount_table
block: Fix multiwrite error handling
block: Fix error code in multiwrite for immediate failures
block: Fix multiwrite memory leak in error case
qcow2: Don't ignore immediate read/write failures
qcow2: Remove request from in-flight list after error

 block.c                |   11 +-
 block/qcow2-cluster.c  |    1 +
 block/qcow2-refcount.c |  334 ++++++++++++++++++++++++++++++++++--------------
 block/qcow2.c          |   14 ++-
 hw/scsi-disk.c         |    4 +-
 json-parser.c          |    2 +-
 qemu-common.h          |    4 +
 7 files changed, 266 insertions(+), 104 deletions(-)
author
#1 lore raw

PATCH 1/10 json-parser: Fix segfault on malformed input

If the parser fails to parse the key in parse_pair, it will access a NULL pointer. A simple way to trigger this is sending {foo} via QMP. This patch turns the segfault into a syntax error reply.

(cherry picked from commit d758d90fe1f74a46042fca665036a23b4d5fe87d)
---
 json-parser.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
json-parser.c +1 -1
index 2ab6f6c..3497cd3 100644
@@ -266,7 +266,7 @@ static int parse_pair(JSONParserContext *ctxt, QDict *dict, QList **tokens, va_l
 
     peek = qlist_peek(working);
     key = parse_value(ctxt, &working, ap);
-    if (qobject_type(key) != QTYPE_QSTRING) {
+    if (!key || qobject_type(key) != QTYPE_QSTRING) {
         parse_error(ctxt, peek, "key is not a string in object");
         goto out;
     }
Signature · 1 line
1.6.6.1
author
#2 lore raw

PATCH 2/10 block: avoid creating too large iovecs in multiwrite_merge

In reply to Kevin Wolf

From: Christoph Hellwig <[email protected]>

If we go over the maximum number of iovecs support by syscall we get back EINVAL from the kernel which translate to I/O errors for the guest.

Add a MAX_IOV defintion for platforms that don't have it. For now we use the same 1024 define that's used on Linux and various other platforms, but until the windows block backend implements some kind of vectored I/O it doesn't matter.

(cherry picked from commit e2a305fb13ff0f5cf6ff805555aaa90a5ed5954c)
---
 block.c       |    4 ++++
 qemu-common.h |    4 ++++
 2 files changed, 8 insertions(+), 0 deletions(-)
block.c +4 -0
index 97af3f5..9697dc9 100644
@@ -1669,6 +1669,10 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs,
             merge = bs->drv->bdrv_merge_requests(bs, &reqs[outidx], &reqs[i]);
         }
 
+        if (reqs[outidx].qiov->niov + reqs[i].qiov->niov + 1 > IOV_MAX) {
+            merge = 0;
+        }
+
         if (merge) {
             size_t size;
             QEMUIOVector *qiov = qemu_mallocz(sizeof(*qiov));
qemu-common.h +4 -0
index d96060a..a23afbc 100644
@@ -54,6 +54,10 @@ struct iovec {
     void *iov_base;
     size_t iov_len;
 };
+/*
+ * Use the same value as Linux for now.
+ */
+#define IOV_MAX		1024
 #else
 #include <sys/uio.h>
 #endif
Signature · 1 line
1.6.6.1
author
#3 lore raw

PATCH 3/10 qcow2: Factor next_refcount_table_size out

In reply to Kevin Wolf

When the refcount table grows, it doesn't only grow by one entry but reserves some space for future refcount blocks. The algorithm to calculate the number of entries stays the same with the fixes, so factor it out before replacing the rest.

As Juan suggested take the opportunity to simplify the code a bit.

(cherry picked from commit 05121aedc41f87e44e41e9cef55f2e49ce7ba94e)
---
 block/qcow2-refcount.c |   30 +++++++++++++++++++-----------
 1 files changed, 19 insertions(+), 11 deletions(-)
block/qcow2-refcount.c +19 -11
index c2a5c04..5dde80a 100644
@@ -123,6 +123,24 @@ static int get_refcount(BlockDriverState *bs, int64_t cluster_index)
     return be16_to_cpu(s->refcount_block_cache[block_index]);
 }
 
+/*
+ * Rounds the refcount table size up to avoid growing the table for each single
+ * refcount block that is allocated.
+ */
+static unsigned int next_refcount_table_size(BDRVQcowState *s,
+    unsigned int min_size)
+{
+    unsigned int min_clusters = (min_size >> (s->cluster_bits - 3)) + 1;
+    unsigned int refcount_table_clusters =
+        MAX(1, s->refcount_table_size >> (s->cluster_bits - 3));
+
+    while (min_clusters > refcount_table_clusters) {
+        refcount_table_clusters = (refcount_table_clusters * 3 + 1) / 2;
+    }
+
+    return refcount_table_clusters << (s->cluster_bits - 3);
+}
+
 static int grow_refcount_table(BlockDriverState *bs, int min_size)
 {
     BDRVQcowState *s = bs->opaque;
@@ -136,17 +154,7 @@ static int grow_refcount_table(BlockDriverState *bs, int min_size)
     if (min_size <= s->refcount_table_size)
         return 0;
     /* compute new table size */
-    refcount_table_clusters = s->refcount_table_size >> (s->cluster_bits - 3);
-    for(;;) {
-        if (refcount_table_clusters == 0) {
-            refcount_table_clusters = 1;
-        } else {
-            refcount_table_clusters = (refcount_table_clusters * 3 + 1) / 2;
-        }
-        new_table_size = refcount_table_clusters << (s->cluster_bits - 3);
-        if (min_size <= new_table_size)
-            break;
-    }
+    new_table_size = next_refcount_table_size(s, min_size);
 #ifdef DEBUG_ALLOC2
     printf("grow_refcount_table from %d to %d\n",
            s->refcount_table_size,
Signature · 1 line
1.6.6.1
author
#4 lore raw

PATCH 4/10 qcow2: Rewrite alloc_refcount_block/grow_refcount_table

In reply to Kevin Wolf

The current implementation of alloc_refcount_block and grow_refcount_table has fundamental problems regarding error handling. There are some places where an I/O error means that the image is going to be corrupted. I have found that the only way to fix this is to completely rewrite the thing.

In detail, the problem is that the refcount blocks itself are allocated using alloc_refcount_noref (to avoid endless recursion when updating the refcount of the new refcount block, which migh access just the same refcount block but its allocation is not yet completed...). Only at the end of the refcount allocation the refcount of the refcount block is increased. If an error happens in between, the refcount block is in use, but has a refcount of zero and will likely be overwritten later.

The new approach is explained in comments in the code. The trick is basically to let new refcount blocks describe their own refcount, so their refcount will be automatically changed when they are hooked up in the refcount table.

(cherry picked from commit 92dcb59fd4e1491afa0756ee9c2594869b487d23)
---
 block/qcow2-refcount.c |  310 ++++++++++++++++++++++++++++++++++--------------
 1 files changed, 222 insertions(+), 88 deletions(-)
block/qcow2-refcount.c +222 -88
index 5dde80a..5ebbcb6 100644
@@ -27,7 +27,7 @@
 #include "block/qcow2.h"
 
 static int64_t alloc_clusters_noref(BlockDriverState *bs, int64_t size);
-static int update_refcount(BlockDriverState *bs,
+static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
                             int64_t offset, int64_t length,
                             int addend);
 
@@ -141,114 +141,248 @@ static unsigned int next_refcount_table_size(BDRVQcowState *s,
     return refcount_table_clusters << (s->cluster_bits - 3);
 }
 
-static int grow_refcount_table(BlockDriverState *bs, int min_size)
+
+/* Checks if two offsets are described by the same refcount block */
+static int in_same_refcount_block(BDRVQcowState *s, uint64_t offset_a,
+    uint64_t offset_b)
+{
+    uint64_t block_a = offset_a >> (2 * s->cluster_bits - REFCOUNT_SHIFT);
+    uint64_t block_b = offset_b >> (2 * s->cluster_bits - REFCOUNT_SHIFT);
+
+    return (block_a == block_b);
+}
+
+/*
+ * Loads a refcount block. If it doesn't exist yet, it is allocated first
+ * (including growing the refcount table if needed).
+ *
+ * Returns the offset of the refcount block on success or -errno in error case
+ */
+static int64_t alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index)
 {
     BDRVQcowState *s = bs->opaque;
-    int new_table_size, new_table_size2, refcount_table_clusters, i, ret;
-    uint64_t *new_table;
-    int64_t table_offset;
-    uint8_t data[12];
-    int old_table_size;
-    int64_t old_table_offset;
+    unsigned int refcount_table_index;
+    int ret;
+
+    /* Find the refcount block for the given cluster */
+    refcount_table_index = cluster_index >> (s->cluster_bits - REFCOUNT_SHIFT);
+
+    if (refcount_table_index < s->refcount_table_size) {
+
+        uint64_t refcount_block_offset =
+            s->refcount_table[refcount_table_index];
+
+        /* If it's already there, we're done */
+        if (refcount_block_offset) {
+            if (refcount_block_offset != s->refcount_block_cache_offset) {
+                ret = load_refcount_block(bs, refcount_block_offset);
+                if (ret < 0) {
+                    return ret;
+                }
+            }
+            return refcount_block_offset;
+        }
+    }
+
+    /*
+     * If we came here, we need to allocate something. Something is at least
+     * a cluster for the new refcount block. It may also include a new refcount
+     * table if the old refcount table is too small.
+     *
+     * Note that allocating clusters here needs some special care:
+     *
+     * - We can't use the normal qcow2_alloc_clusters(), it would try to
+     *   increase the refcount and very likely we would end up with an endless
+     *   recursion. Instead we must place the refcount blocks in a way that
+     *   they can describe them themselves.
+     *
+     * - We need to consider that at this point we are inside update_refcounts
+     *   and doing the initial refcount increase. This means that some clusters
+     *   have already been allocated by the caller, but their refcount isn't
+     *   accurate yet. free_cluster_index tells us where this allocation ends
+     *   as long as we don't overwrite it by freeing clusters.
+     *
+     * - alloc_clusters_noref and qcow2_free_clusters may load a different
+     *   refcount block into the cache
+     */
+
+    if (cache_refcount_updates) {
+        ret = write_refcount_block(s);
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
+    /* Allocate the refcount block itself and mark it as used */
+    uint64_t new_block = alloc_clusters_noref(bs, s->cluster_size);
+    memset(s->refcount_block_cache, 0, s->cluster_size);
+    s->refcount_block_cache_offset = new_block;
 
-    if (min_size <= s->refcount_table_size)
-        return 0;
-    /* compute new table size */
-    new_table_size = next_refcount_table_size(s, min_size);
 #ifdef DEBUG_ALLOC2
-    printf("grow_refcount_table from %d to %d\n",
-           s->refcount_table_size,
-           new_table_size);
+    fprintf(stderr, "qcow2: Allocate refcount block %d for %" PRIx64
+        " at %" PRIx64 "\n",
+        refcount_table_index, cluster_index << s->cluster_bits, new_block);
 #endif
-    new_table_size2 = new_table_size * sizeof(uint64_t);
-    new_table = qemu_mallocz(new_table_size2);
+
+    if (in_same_refcount_block(s, new_block, cluster_index << s->cluster_bits)) {
+        /* The block describes itself, need to update the cache */
+        int block_index = (new_block >> s->cluster_bits) &
+            ((1 << (s->cluster_bits - REFCOUNT_SHIFT)) - 1);
+        s->refcount_block_cache[block_index] = cpu_to_be16(1);
+    } else {
+        /* Described somewhere else. This can recurse at most twice before we
+         * arrive at a block that describes itself. */
+        ret = update_refcount(bs, new_block, s->cluster_size, 1);
+        if (ret < 0) {
+            goto fail_block;
+        }
+    }
+
+    /* Now the new refcount block needs to be written to disk */
+    ret = bdrv_pwrite(s->hd, new_block, s->refcount_block_cache,
+        s->cluster_size);
+    if (ret < 0) {
+        goto fail_block;
+    }
+
+    /* If the refcount table is big enough, just hook the block up there */
+    if (refcount_table_index < s->refcount_table_size) {
+        uint64_t data64 = cpu_to_be64(new_block);
+        ret = bdrv_pwrite(s->hd,
+            s->refcount_table_offset + refcount_table_index * sizeof(uint64_t),
+            &data64, sizeof(data64));
+        if (ret < 0) {
+            goto fail_block;
+        }
+
+        s->refcount_table[refcount_table_index] = new_block;
+        return new_block;
+    }
+
+    /*
+     * If we come here, we need to grow the refcount table. Again, a new
+     * refcount table needs some space and we can't simply allocate to avoid
+     * endless recursion.
+     *
+     * Therefore let's grab new refcount blocks at the end of the image, which
+     * will describe themselves and the new refcount table. This way we can
+     * reference them only in the new table and do the switch to the new
+     * refcount table at once without producing an inconsistent state in
+     * between.
+     */
+    /* Calculate the number of refcount blocks needed so far */
+    uint64_t refcount_block_clusters = 1 << (s->cluster_bits - REFCOUNT_SHIFT);
+    uint64_t blocks_used = (s->free_cluster_index +
+        refcount_block_clusters - 1) / refcount_block_clusters;
+
+    /* And now we need at least one block more for the new metadata */
+    uint64_t table_size = next_refcount_table_size(s, blocks_used + 1);
+    uint64_t last_table_size;
+    uint64_t blocks_clusters;
+    do {
+        uint64_t table_clusters = size_to_clusters(s, table_size);
+        blocks_clusters = 1 +
+            ((table_clusters + refcount_block_clusters - 1)
+            / refcount_block_clusters);
+        uint64_t meta_clusters = table_clusters + blocks_clusters;
+
+        last_table_size = table_size;
+        table_size = next_refcount_table_size(s, blocks_used +
+            ((meta_clusters + refcount_block_clusters - 1)
+            / refcount_block_clusters));
+
+    } while (last_table_size != table_size);
+
+#ifdef DEBUG_ALLOC2
+    fprintf(stderr, "qcow2: Grow refcount table %" PRId32 " => %" PRId64 "\n",
+        s->refcount_table_size, table_size);
+#endif
+
+    /* Create the new refcount table and blocks */
+    uint64_t meta_offset = (blocks_used * refcount_block_clusters) *
+        s->cluster_size;
+    uint64_t table_offset = meta_offset + blocks_clusters * s->cluster_size;
+    uint16_t *new_blocks = qemu_mallocz(blocks_clusters * s->cluster_size);
+    uint64_t *new_table = qemu_mallocz(table_size * sizeof(uint64_t));
+
+    assert(meta_offset >= (s->free_cluster_index * s->cluster_size));
+
+    /* Fill the new refcount table */
     memcpy(new_table, s->refcount_table,
-           s->refcount_table_size * sizeof(uint64_t));
-    for(i = 0; i < s->refcount_table_size; i++)
+        s->refcount_table_size * sizeof(uint64_t));
+    new_table[refcount_table_index] = new_block;
+
+    int i;
+    for (i = 0; i < blocks_clusters; i++) {
+        new_table[blocks_used + i] = meta_offset + (i * s->cluster_size);
+    }
+
+    /* Fill the refcount blocks */
+    uint64_t table_clusters = size_to_clusters(s, table_size * sizeof(uint64_t));
+    int block = 0;
+    for (i = 0; i < table_clusters + blocks_clusters; i++) {
+        new_blocks[block++] = cpu_to_be16(1);
+    }
+
+    /* Write refcount blocks to disk */
+    ret = bdrv_pwrite(s->hd, meta_offset, new_blocks,
+        blocks_clusters * s->cluster_size);
+    qemu_free(new_blocks);
+    if (ret < 0) {
+        goto fail_table;
+    }
+
+    /* Write refcount table to disk */
+    for(i = 0; i < table_size; i++) {
         cpu_to_be64s(&new_table[i]);
-    /* Note: we cannot update the refcount now to avoid recursion */
-    table_offset = alloc_clusters_noref(bs, new_table_size2);
-    ret = bdrv_pwrite(s->hd, table_offset, new_table, new_table_size2);
-    if (ret != new_table_size2)
-        goto fail;
-    for(i = 0; i < s->refcount_table_size; i++)
-        be64_to_cpus(&new_table[i]);
+    }
+
+    ret = bdrv_pwrite(s->hd, table_offset, new_table,
+        table_size * sizeof(uint64_t));
+    if (ret < 0) {
+        goto fail_table;
+    }
 
+    for(i = 0; i < table_size; i++) {
+        cpu_to_be64s(&new_table[i]);
+    }
+
+    /* Hook up the new refcount table in the qcow2 header */
+    uint8_t data[12];
     cpu_to_be64w((uint64_t*)data, table_offset);
-    cpu_to_be32w((uint32_t*)(data + 8), refcount_table_clusters);
+    cpu_to_be32w((uint32_t*)(data + 8), table_clusters);
     ret = bdrv_pwrite(s->hd, offsetof(QCowHeader, refcount_table_offset),
-                    data, sizeof(data));
-    if (ret != sizeof(data)) {
-        goto fail;
+        data, sizeof(data));
+    if (ret < 0) {
+        goto fail_table;
     }
 
+    /* And switch it in memory */
+    uint64_t old_table_offset = s->refcount_table_offset;
+    uint64_t old_table_size = s->refcount_table_size;
+
     qemu_free(s->refcount_table);
-    old_table_offset = s->refcount_table_offset;
-    old_table_size = s->refcount_table_size;
     s->refcount_table = new_table;
-    s->refcount_table_size = new_table_size;
+    s->refcount_table_size = table_size;
     s->refcount_table_offset = table_offset;
 
-    update_refcount(bs, table_offset, new_table_size2, 1);
+    /* Free old table. Remember, we must not change free_cluster_index */
+    uint64_t old_free_cluster_index = s->free_cluster_index;
     qcow2_free_clusters(bs, old_table_offset, old_table_size * sizeof(uint64_t));
-    return 0;
- fail:
-    qemu_free(new_table);
-    return ret < 0 ? ret : -EIO;
-}
-
-
-static int64_t alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index)
-{
-    BDRVQcowState *s = bs->opaque;
-    int64_t offset, refcount_block_offset;
-    unsigned int refcount_table_index;
-    int ret;
-    uint64_t data64;
-    int cache = cache_refcount_updates;
+    s->free_cluster_index = old_free_cluster_index;
 
-    /* Find L1 index and grow refcount table if needed */
-    refcount_table_index = cluster_index >> (s->cluster_bits - REFCOUNT_SHIFT);
-    if (refcount_table_index >= s->refcount_table_size) {
-        ret = grow_refcount_table(bs, refcount_table_index + 1);
-        if (ret < 0)
-            return ret;
+    ret = load_refcount_block(bs, new_block);
+    if (ret < 0) {
+        goto fail_block;
     }
 
-    /* Load or allocate the refcount block */
-    refcount_block_offset = s->refcount_table[refcount_table_index];
-    if (!refcount_block_offset) {
-        if (cache_refcount_updates) {
-            write_refcount_block(s);
-            cache_refcount_updates = 0;
-        }
-        /* create a new refcount block */
-        /* Note: we cannot update the refcount now to avoid recursion */
-        offset = alloc_clusters_noref(bs, s->cluster_size);
-        memset(s->refcount_block_cache, 0, s->cluster_size);
-        ret = bdrv_pwrite(s->hd, offset, s->refcount_block_cache, s->cluster_size);
-        if (ret != s->cluster_size)
-            return -EINVAL;
-        s->refcount_table[refcount_table_index] = offset;
-        data64 = cpu_to_be64(offset);
-        ret = bdrv_pwrite(s->hd, s->refcount_table_offset +
-                          refcount_table_index * sizeof(uint64_t),
-                          &data64, sizeof(data64));
-        if (ret != sizeof(data64))
-            return -EINVAL;
-
-        refcount_block_offset = offset;
-        s->refcount_block_cache_offset = offset;
-        update_refcount(bs, offset, s->cluster_size, 1);
-        cache_refcount_updates = cache;
-    } else {
-        if (refcount_block_offset != s->refcount_block_cache_offset) {
-            if (load_refcount_block(bs, refcount_block_offset) < 0)
-                return -EIO;
-        }
-    }
+    return new_block;
 
-    return refcount_block_offset;
+fail_table:
+    qemu_free(new_table);
+fail_block:
+    s->refcount_block_cache_offset = 0;
+    return ret;
 }
 
 #define REFCOUNTS_PER_SECTOR (512 >> REFCOUNT_SHIFT)
Signature · 1 line
1.6.6.1
author
#5 lore raw

PATCH 5/10 scsi-disk: fix buffer overflow

In reply to Kevin Wolf

From: Gerd Hoffmann <[email protected]>

In case s->version is shorter than 4 bytes we overflow the memcpy src buffer. Fix it by clearing the target buffer, then copy only the amount of bytes we actually have.

(cherry picked from 314b1811c15f4e982e4667d9b845aee4b5a63d91)

---
 hw/scsi-disk.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
hw/scsi-disk.c +3 -1
index b34fbaa..a792012 100644
@@ -434,7 +434,9 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
         memcpy(&outbuf[16], "QEMU HARDDISK   ", 16);
     }
     memcpy(&outbuf[8], "QEMU    ", 8);
-    memcpy(&outbuf[32], s->version ? s->version : QEMU_VERSION, 4);
+    memset(&outbuf[32], 0, 4);
+    memcpy(&outbuf[32], s->version ? s->version : QEMU_VERSION,
+           MIN(4, strlen(s->version ? s->version : QEMU_VERSION)));
     /* Identify device as SCSI-3 rev 1.
        Some later commands are also implemented. */
     outbuf[2] = 3;
Signature · 1 line
1.6.6.1
author
#6 lore raw

PATCH 6/10 block: Fix multiwrite error handling

In reply to Kevin Wolf

When two requests of the same multiwrite batch fail, the callback of all requests in that batch were called twice. This could have any kind of nasty effects, in my case it lead to use after free and eventually a segfault.

---
 block.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
block.c +1 -1
index 9697dc9..06e22a6 100644
@@ -1617,7 +1617,7 @@ static void multiwrite_cb(void *opaque, int ret)
 {
     MultiwriteCB *mcb = opaque;
 
-    if (ret < 0) {
+    if (ret < 0 && !mcb->error) {
         mcb->error = ret;
         multiwrite_user_cb(mcb);
     }
Signature · 1 line
1.6.6.1
author
#7 lore raw

PATCH 7/10 block: Fix error code in multiwrite for immediate failures

In reply to Kevin Wolf
---
 block.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
block.c +2 -2
index 06e22a6..d537d10 100644
@@ -1758,10 +1758,10 @@ int bdrv_aio_multiwrite(BlockDriverState *bs, BlockRequest *reqs, int num_reqs)
             // submitted yet. Otherwise we'll wait for the submitted AIOs to
             // complete and report the error in the callback.
             if (mcb->num_requests == 0) {
-                reqs[i].error = EIO;
+                reqs[i].error = -EIO;
                 goto fail;
             } else {
-                mcb->error = EIO;
+                mcb->error = -EIO;
                 break;
             }
         } else {
Signature · 1 line
1.6.6.1
author
#8 lore raw

PATCH 8/10 block: Fix multiwrite memory leak in error case

In reply to Kevin Wolf

Previously multiwrite_user_cb was never called if a request in the multiwrite batch failed right away because it did set mcb->error immediately. Make it look more like a normal callback to fix this.

---
 block.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
block.c +2 -1
index d537d10..4f9a48b 100644
@@ -1761,7 +1761,8 @@ int bdrv_aio_multiwrite(BlockDriverState *bs, BlockRequest *reqs, int num_reqs)
                 reqs[i].error = -EIO;
                 goto fail;
             } else {
-                mcb->error = -EIO;
+                mcb->num_requests++;
+                multiwrite_cb(mcb, -EIO);
                 break;
             }
         } else {
Signature · 1 line
1.6.6.1
author
#9 lore raw

PATCH 9/10 qcow2: Don't ignore immediate read/write failures

In reply to Kevin Wolf

Returning -EIO is far from optimal, but at least it's an error code.

---
 block/qcow2.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)
block/qcow2.c +6 -2
index 4ae8f19..f6f8980 100644
@@ -467,8 +467,10 @@ static void qcow_aio_read_cb(void *opaque, int ret)
         acb->hd_aiocb = bdrv_aio_readv(s->hd,
                             (acb->cluster_offset >> 9) + index_in_cluster,
                             &acb->hd_qiov, acb->n, qcow_aio_read_cb, acb);
-        if (acb->hd_aiocb == NULL)
+        if (acb->hd_aiocb == NULL) {
+            ret = -EIO;
             goto done;
+        }
     }
 
     return;
@@ -620,8 +622,10 @@ static void qcow_aio_write_cb(void *opaque, int ret)
                                     (acb->cluster_offset >> 9) + index_in_cluster,
                                     &acb->hd_qiov, acb->n,
                                     qcow_aio_write_cb, acb);
-    if (acb->hd_aiocb == NULL)
+    if (acb->hd_aiocb == NULL) {
+        ret = -EIO;
         goto done;
+    }
 
     return;
 
Signature · 1 line
1.6.6.1
author
#10 lore raw

PATCH 10/10 qcow2: Remove request from in-flight list after error

In reply to Kevin Wolf

If we complete a request with a failure we need to remove it from the list of requests that are in flight. If we don't do it, the next time the same AIOCB is used for a cluster allocation it will create a loop in the list and qemu will hang in an endless loop.

---
 block/qcow2-cluster.c |    1 +
 block/qcow2.c         |    6 +++++-
 2 files changed, 6 insertions(+), 1 deletions(-)
block/qcow2-cluster.c +1 -0
index b13b693..c7057b1 100644
@@ -811,6 +811,7 @@ int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
 
     cluster_offset = qcow2_alloc_clusters(bs, nb_clusters * s->cluster_size);
     if (cluster_offset < 0) {
+        QLIST_REMOVE(m, next_in_flight);
         return cluster_offset;
     }
 
block/qcow2.c +5 -1
index f6f8980..5d33d6c 100644
@@ -624,11 +624,15 @@ static void qcow_aio_write_cb(void *opaque, int ret)
                                     qcow_aio_write_cb, acb);
     if (acb->hd_aiocb == NULL) {
         ret = -EIO;
-        goto done;
+        goto fail;
     }
 
     return;
 
+fail:
+    if (acb->l2meta.nb_clusters != 0) {
+        QLIST_REMOVE(&acb->l2meta, next_in_flight);
+    }
 done:
     if (acb->qiov->niov > 1)
         qemu_vfree(acb->orig_buf);
Signature · 1 line
1.6.6.1
#11 lore raw
In reply to Kevin Wolf
Quoted 8 lines · Kevin Wolf

Hi Aurelien,

I went through the patches that were committed recently, either written by me or concerning block stuff (most of them are both). I came up with this list of patches that should go into 0.12.4. The first half of them is contained in current master (you'll see the cherry-pick line in the commit log for those), the rest has already been posted to the list, but is still sitting in my block queue and waiting for Anthony's return.

Thanks for those patches. I have already pulled all the cherry-picked ones, I'll look at the others a bit later as I need a bit more time to look at them in details.

Quoted 38 lines

Kevin

The following changes since commit 804b6ab08d82adcab05d64362be91c48a7467fd4:
Paul Brook (1):
UHCI spurious interrut fix

are available in the git repository at:

git://repo.or.cz/qemu/kevin.git for-aurelien-0.12

Christoph Hellwig (1):
block: avoid creating too large iovecs in multiwrite_merge

Gerd Hoffmann (1):
scsi-disk: fix buffer overflow

Kevin Wolf (8):
json-parser: Fix segfault on malformed input
qcow2: Factor next_refcount_table_size out
qcow2: Rewrite alloc_refcount_block/grow_refcount_table
block: Fix multiwrite error handling
block: Fix error code in multiwrite for immediate failures
block: Fix multiwrite memory leak in error case
qcow2: Don't ignore immediate read/write failures
qcow2: Remove request from in-flight list after error

 block.c                |   11 +-
 block/qcow2-cluster.c  |    1 +
 block/qcow2-refcount.c |  334 ++++++++++++++++++++++++++++++++++--------------
 block/qcow2.c          |   14 ++-
 hw/scsi-disk.c         |    4 +-
 json-parser.c          |    2 +-
 qemu-common.h          |    4 +
 7 files changed, 266 insertions(+), 104 deletions(-)
Signature · 2 lines
Aurelien Jarno                          GPG: 1024D/F1BCDB73
[email protected]                 http://www.aurel32.net