qcow2: More error path fixes

PATCH In review Kevin Wolf 2 participants 9 messages Apr 6, 2010 – Apr 10, 2010 Block layer

lore

Share & tools

lore

Kevin Wolf

author

2010-04-06 13:30 UTC #0 lore raw

cover qcow2: More error path fixes

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(-)

Kevin Wolf

author

2010-04-06 13:31 UTC #1 lore raw

PATCH 1/6 qcow2: Don't ignore immediate read/write failures

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

Signed-off-by Kevin Wolf <[email protected]>

---
 block/qcow2.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

block/qcow2.c +6 -2

index 5bea8f5..2f34eec 100644
@@ -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;

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-04-06 13:31 UTC #2 lore raw

PATCH 2/6 qcow2: Return 0/-errno in write_l2_entries

In reply to Kevin Wolf

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

Signed-off-by Kevin Wolf <[email protected]>

---
 block/qcow2-cluster.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

block/qcow2-cluster.c +5 -4

index 9b3d686..65b16db 100644
@@ -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;

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-04-06 13:31 UTC #3 lore raw

PATCH 3/6 qcow2: Fix error return code in qcow2_alloc_cluster_link_l2

In reply to 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 <[email protected]>

---
 block/qcow2-cluster.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

block/qcow2-cluster.c +2 -2

index 65b16db..8cc742b 100644
@@ -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;
     }

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-04-06 13:31 UTC #4 lore raw

PATCH 4/6 qcow2: Return 0/-errno in write_l1_entry

In reply to Kevin Wolf

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

Signed-off-by Kevin Wolf <[email protected]>

---
 block/qcow2-cluster.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

block/qcow2-cluster.c +5 -5

index 8cc742b..a2d3962 100644
@@ -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;

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-04-06 13:31 UTC #5 lore raw

PATCH 5/6 qcow2: Return 0/-errno in l2_allocate

In reply to 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 <[email protected]>

---
 block/qcow2-cluster.c |   40 +++++++++++++++++++++++-----------------
 1 files changed, 23 insertions(+), 17 deletions(-)

block/qcow2-cluster.c +23 ~~-17~~

index a2d3962..ace3b85 100644
@@ -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;
     }

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-04-06 13:31 UTC #6 lore raw

PATCH 6/6 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.

Signed-off-by Kevin Wolf <[email protected]>

---
 block/qcow2-cluster.c |    1 +
 block/qcow2.c         |    6 +++++-
 2 files changed, 6 insertions(+), 1 deletions(-)

block/qcow2-cluster.c +1 -0

index ace3b85..639e05e 100644
@@ -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;
     }

block/qcow2.c +5 -1

index 2f34eec..11ce8d1 100644
@@ -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);

Signature · 1 line

1.6.6.1

Aurelien Jarno

2010-04-10 01:14 UTC #7 lore raw

In reply to Kevin Wolf

Quoted 4 lines · 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)

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.

Signature · 2 lines

Aurelien Jarno                          GPG: 1024D/F1BCDB73
[email protected]                 http://www.aurel32.net

Kevin Wolf

author

2010-04-10 08:14 UTC #8 lore raw

Am 10.04.2010 02:02, schrieb Aurelien Jarno:

Quoted 11 lines · Kevin Wolf

Quoted 4 lines
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