[Qemu-devel] [PATCH 0/6] qcow2: More error path fixes

[Qemu-devel] [PATCH 0/6] qcow2: More error path fixes

[Kevin Wolf]

This is part three of the qcow2 error path fixes. More to come...

May depend on any patches sent earlier that are in my queue (I think it does
depend at least on the blkdebug series to apply cleanly)

Kevin Wolf (6):
  qcow2: Don't ignore immediate read/write failures
  qcow2: Return 0/-errno in write_l2_entries
  qcow2: Fix error return code in qcow2_alloc_cluster_link_l2
  qcow2: Return 0/-errno in write_l1_entry
  qcow2: Return 0/-errno in l2_allocate
  qcow2: Remove request from in-flight list after error

 block/qcow2-cluster.c |   64 +++++++++++++++++++++++++++---------------------
 block/qcow2.c         |   14 ++++++++--
 2 files changed, 47 insertions(+), 31 deletions(-)

[Qemu-devel] [PATCH 1/6] qcow2: Don't ignore immediate read/write failures

[Kevin Wolf]

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

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 5bea8f5..2f34eec 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -470,8 +470,10 @@ static void qcow_aio_read_cb(void *opaque, int ret)
                             (acb->cluster_offset >> 9) + index_in_cluster,
                             &acb->hd_qiov, acb->cur_nr_sectors,
                             qcow_aio_read_cb, acb);
-        if (acb->hd_aiocb == NULL)
+        if (acb->hd_aiocb == NULL) {
+            ret = -EIO;
             goto done;
+        }
     }
 
     return;
@@ -624,8 +626,10 @@ static void qcow_aio_write_cb(void *opaque, int ret)
                                     (acb->cluster_offset >> 9) + index_in_cluster,
                                     &acb->hd_qiov, acb->cur_nr_sectors,
                                     qcow_aio_write_cb, acb);
-    if (acb->hd_aiocb == NULL)
+    if (acb->hd_aiocb == NULL) {
+        ret = -EIO;
         goto done;
+    }
 
     return;
 
-- 
1.6.6.1

[Qemu-devel] [PATCH 2/6] qcow2: Return 0/-errno in write_l2_entries

[Kevin Wolf]

Change write_l2_entries to return the real error code instead of -1.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 9b3d686..65b16db 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -627,12 +627,13 @@ static int write_l2_entries(BDRVQcowState *s, uint64_t *l2_table,
     int start_offset = (8 * l2_index) & ~511;
     int end_offset = (8 * (l2_index + num) + 511) & ~511;
     size_t len = end_offset - start_offset;
+    int ret;
 
     BLKDBG_EVENT(s->hd, BLKDBG_L2_UPDATE);
-    if (bdrv_pwrite(s->hd, l2_offset + start_offset, &l2_table[l2_start_index],
-        len) != len)
-    {
-        return -1;
+    ret = bdrv_pwrite(s->hd, l2_offset + start_offset,
+        &l2_table[l2_start_index], len);
+    if (ret < 0) {
+        return ret;
     }
 
     return 0;
-- 
1.6.6.1

[Qemu-devel] [PATCH 3/6] qcow2: Fix error return code in qcow2_alloc_cluster_link_l2

[Kevin Wolf]

Fix qcow2_alloc_cluster_link_l2 to return the real error code like it does in
all other error cases.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 65b16db..8cc742b 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -687,8 +687,8 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
                     (i << s->cluster_bits)) | QCOW_OFLAG_COPIED);
      }
 
-    if (write_l2_entries(s, l2_table, l2_offset, l2_index, m->nb_clusters) < 0) {
-        ret = -1;
+    ret = write_l2_entries(s, l2_table, l2_offset, l2_index, m->nb_clusters);
+    if (ret < 0) {
         goto err;
     }
 
-- 
1.6.6.1

[Qemu-devel] [PATCH 4/6] qcow2: Return 0/-errno in write_l1_entry

[Kevin Wolf]

Change write_l1_entry to return the real error code instead of -1.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 8cc742b..a2d3962 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -193,7 +193,7 @@ static int write_l1_entry(BDRVQcowState *s, int l1_index)
 {
     uint64_t buf[L1_ENTRIES_PER_SECTOR];
     int l1_start_index;
-    int i;
+    int i, ret;
 
     l1_start_index = l1_index & ~(L1_ENTRIES_PER_SECTOR - 1);
     for (i = 0; i < L1_ENTRIES_PER_SECTOR; i++) {
@@ -201,10 +201,10 @@ static int write_l1_entry(BDRVQcowState *s, int l1_index)
     }
 
     BLKDBG_EVENT(s->hd, BLKDBG_L1_UPDATE);
-    if (bdrv_pwrite(s->hd, s->l1_table_offset + 8 * l1_start_index,
-        buf, sizeof(buf)) != sizeof(buf))
-    {
-        return -1;
+    ret = bdrv_pwrite(s->hd, s->l1_table_offset + 8 * l1_start_index,
+        buf, sizeof(buf));
+    if (ret < 0) {
+        return ret;
     }
 
     return 0;
-- 
1.6.6.1

[Qemu-devel] [PATCH 5/6] qcow2: Return 0/-errno in l2_allocate

[Kevin Wolf]

Returning NULL on error doesn't allow distinguishing between different errors.
Change the interface to return an integer for -errno.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c |   40 +++++++++++++++++++++++-----------------
 1 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index a2d3962..ace3b85 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -220,13 +220,14 @@ static int write_l1_entry(BDRVQcowState *s, int l1_index)
  *
  */
 
-static uint64_t *l2_allocate(BlockDriverState *bs, int l1_index)
+static int l2_allocate(BlockDriverState *bs, int l1_index, uint64_t **table)
 {
     BDRVQcowState *s = bs->opaque;
     int min_index;
     uint64_t old_l2_offset;
     uint64_t *l2_table;
     int64_t l2_offset;
+    int ret;
 
     old_l2_offset = s->l1_table[l1_index];
 
@@ -234,14 +235,15 @@ static uint64_t *l2_allocate(BlockDriverState *bs, int l1_index)
 
     l2_offset = qcow2_alloc_clusters(bs, s->l2_size * sizeof(uint64_t));
     if (l2_offset < 0) {
-        return NULL;
+        return l2_offset;
     }
 
     /* update the L1 entry */
 
     s->l1_table[l1_index] = l2_offset | QCOW_OFLAG_COPIED;
-    if (write_l1_entry(s, l1_index) < 0) {
-        return NULL;
+    ret = write_l1_entry(s, l1_index);
+    if (ret < 0) {
+        return ret;
     }
 
     /* allocate a new entry in the l2 cache */
@@ -255,24 +257,27 @@ static uint64_t *l2_allocate(BlockDriverState *bs, int l1_index)
     } else {
         /* if there was an old l2 table, read it from the disk */
         BLKDBG_EVENT(s->hd, BLKDBG_L2_ALLOC_COW_READ);
-        if (bdrv_pread(s->hd, old_l2_offset,
-                       l2_table, s->l2_size * sizeof(uint64_t)) !=
-            s->l2_size * sizeof(uint64_t))
-            return NULL;
+        ret = bdrv_pread(s->hd, old_l2_offset, l2_table,
+            s->l2_size * sizeof(uint64_t));
+        if (ret < 0) {
+            return ret;
+        }
     }
     /* write the l2 table to the file */
     BLKDBG_EVENT(s->hd, BLKDBG_L2_ALLOC_WRITE);
-    if (bdrv_pwrite(s->hd, l2_offset,
-                    l2_table, s->l2_size * sizeof(uint64_t)) !=
-        s->l2_size * sizeof(uint64_t))
-        return NULL;
+    ret = bdrv_pwrite(s->hd, l2_offset, l2_table,
+        s->l2_size * sizeof(uint64_t));
+    if (ret < 0) {
+        return ret;
+    }
 
     /* update the l2 cache entry */
 
     s->l2_cache_offsets[min_index] = l2_offset;
     s->l2_cache_counts[min_index] = 1;
 
-    return l2_table;
+    *table = l2_table;
+    return 0;
 }
 
 static int count_contiguous_clusters(uint64_t nb_clusters, int cluster_size,
@@ -510,7 +515,8 @@ static int get_cluster_table(BlockDriverState *bs, uint64_t offset,
 {
     BDRVQcowState *s = bs->opaque;
     unsigned int l1_index, l2_index;
-    uint64_t l2_offset, *l2_table;
+    uint64_t l2_offset;
+    uint64_t *l2_table = NULL;
     int ret;
 
     /* seek the the l2 offset in the l1 table */
@@ -536,9 +542,9 @@ static int get_cluster_table(BlockDriverState *bs, uint64_t offset,
     } else {
         if (l2_offset)
             qcow2_free_clusters(bs, l2_offset, s->l2_size * sizeof(uint64_t));
-        l2_table = l2_allocate(bs, l1_index);
-        if (l2_table == NULL) {
-            return -EIO;
+        ret = l2_allocate(bs, l1_index, &l2_table);
+        if (ret < 0) {
+            return ret;
         }
         l2_offset = s->l1_table[l1_index] & ~QCOW_OFLAG_COPIED;
     }
-- 
1.6.6.1

[Qemu-devel] [PATCH 6/6] qcow2: Remove request from in-flight list after error

[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.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c |    1 +
 block/qcow2.c         |    6 +++++-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index ace3b85..639e05e 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -832,6 +832,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;
     }
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 2f34eec..11ce8d1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -628,11 +628,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);
-- 
1.6.6.1

Re: [Qemu-devel] [PATCH 0/6] qcow2: More error path fixes

[Aurelien Jarno]

On Tue, Apr 06, 2010 at 03:30:08PM +0200, Kevin Wolf wrote:
> This is part three of the qcow2 error path fixes. More to come...
> 
> May depend on any patches sent earlier that are in my queue (I think it does
> depend at least on the blkdebug series to apply cleanly)

All those patches looks fine to me, but as you explained, they depend
on the blkdebug series, that I am not fully comfortable to review.

I have therefore applied patches 1 & 6 only, to be able to cherry-pick
them to the stable branch.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 0/6] qcow2: More error path fixes

[Kevin Wolf]

Am 10.04.2010 02:02, schrieb Aurelien Jarno:
> On Tue, Apr 06, 2010 at 03:30:08PM +0200, Kevin Wolf wrote:
>> This is part three of the qcow2 error path fixes. More to come...
>>
>> May depend on any patches sent earlier that are in my queue (I think it does
>> depend at least on the blkdebug series to apply cleanly)
> 
> All those patches looks fine to me, but as you explained, they depend
> on the blkdebug series, that I am not fully comfortable to review.
> 
> I have therefore applied patches 1 & 6 only, to be able to cherry-pick
> them to the stable branch.

Okay, makes sense. The patches you left out are nice to have, but not as
critical as 1 & 6, so that's fine. Block work can be done based on my
tree, so it doesn't hurt too much if they sit on the list until Anthony
returns.

Kevin