[Qemu-devel] [PATCH 0/3] qcow2/coroutine fixes

[Qemu-devel] [PATCH 0/3] qcow2/coroutine fixes

[Kevin Wolf]

This fixes a couple of issues with the coroutine-based block layer regarding
error handling.

Luiz, can you please try if this fixes your bug?

Kevin Wolf (3):
  qcow2: Properly initialise QcowL2Meta
  qcow2: Fix error cases to run depedent requests
  async: Allow nested qemu_bh_poll calls

 async.c       |   24 ++++++++++++++++--------
 block/qcow2.c |   12 +++++++-----
 2 files changed, 23 insertions(+), 13 deletions(-)

-- 
1.7.6

[Qemu-devel] [PATCH 1/3] qcow2: Properly initialise QcowL2Meta

[Kevin Wolf]

Dependency list pointers filled with random garbage from the stack aren't a
good idea.

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

diff --git a/block/qcow2.c b/block/qcow2.c
index b725d68..f26f7b6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -526,13 +526,14 @@ static int qcow2_co_writev(BlockDriverState *bs,
     int n_end;
     int ret;
     int cur_nr_sectors; /* number of sectors in current iteration */
-    QCowL2Meta l2meta;
     uint64_t cluster_offset;
     QEMUIOVector hd_qiov;
     uint64_t bytes_done = 0;
     uint8_t *cluster_data = NULL;
+    QCowL2Meta l2meta = {
+        .nb_clusters = 0,
+    };
 
-    l2meta.nb_clusters = 0;
     qemu_co_queue_init(&l2meta.dependent_requests);
 
     qemu_iovec_init(&hd_qiov, qiov->niov);
-- 
1.7.6

[Qemu-devel] [PATCH 2/3] qcow2: Fix error cases to run depedent requests

[Kevin Wolf]

Requests depending on a failed request would end up waiting forever. This fixes
the error path to continue dependent requests even when the request has failed.

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

diff --git a/block/qcow2.c b/block/qcow2.c
index f26f7b6..8aed310 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -593,13 +593,12 @@ static int qcow2_co_writev(BlockDriverState *bs,
         }
 
         ret = qcow2_alloc_cluster_link_l2(bs, &l2meta);
-
-        run_dependent_requests(s, &l2meta);
-
         if (ret < 0) {
             goto fail;
         }
 
+        run_dependent_requests(s, &l2meta);
+
         remaining_sectors -= cur_nr_sectors;
         sector_num += cur_nr_sectors;
         bytes_done += cur_nr_sectors * 512;
@@ -607,6 +606,8 @@ static int qcow2_co_writev(BlockDriverState *bs,
     ret = 0;
 
 fail:
+    run_dependent_requests(s, &l2meta);
+
     qemu_co_mutex_unlock(&s->lock);
 
     qemu_iovec_destroy(&hd_qiov);
-- 
1.7.6

[Qemu-devel] [PATCH 3/3] async: Allow nested qemu_bh_poll calls

[Kevin Wolf]

qemu may segfault when a BH handler first deletes a BH and then (possibly
indirectly) calls a nested qemu_bh_poll(). This is because the inner instance
frees the BH and deletes it from the list that the outer one processes.

This patch deletes BHs only in the outermost qemu_bh_poll instance.

Commit 7887f620 already tried to achieve the same, but it assumed that the BH
handler would only delete its own BH. With a nested qemu_bh_poll(), this isn't
guaranteed, so that commit wasn't enough. Hope this one fixes it for real.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 async.c |   24 ++++++++++++++++--------
 1 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/async.c b/async.c
index 9d4e960..ca13962 100644
--- a/async.c
+++ b/async.c
@@ -55,6 +55,9 @@ int qemu_bh_poll(void)
 {
     QEMUBH *bh, **bhp, *next;
     int ret;
+    static int nesting = 0;
+
+    nesting++;
 
     ret = 0;
     for (bh = first_bh; bh; bh = next) {
@@ -68,15 +71,20 @@ int qemu_bh_poll(void)
         }
     }
 
+    nesting--;
+
     /* remove deleted bhs */
-    bhp = &first_bh;
-    while (*bhp) {
-        bh = *bhp;
-        if (bh->deleted) {
-            *bhp = bh->next;
-            g_free(bh);
-        } else
-            bhp = &bh->next;
+    if (!nesting) {
+        bhp = &first_bh;
+        while (*bhp) {
+            bh = *bhp;
+            if (bh->deleted) {
+                *bhp = bh->next;
+                g_free(bh);
+            } else {
+                bhp = &bh->next;
+            }
+        }
     }
 
     return ret;
-- 
1.7.6

Re: [Qemu-devel] [PATCH 0/3] qcow2/coroutine fixes

[Luiz Capitulino]

On Thu,  1 Sep 2011 16:31:08 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> This fixes a couple of issues with the coroutine-based block layer regarding
> error handling.
> 
> Luiz, can you please try if this fixes your bug?

It fixes the reported bug, but it seems that there's another issue.

I'll copy & paste the test case here for our convenience:

 1. Create a 100MB logical volume and create a 200MB qcow2 image on it
 2. Run qemu with the following command-line:

   # qemu -drive file=disks/test.img,if=virtio,cache=writeback,aio=native \
          -drive file=/dev/vg_doriath/kvmtest,if=virtio -enable-kvm -m 1G \
          -monitor stdio -netdev type=tap,id=guest0,script=qemu-ifup-switch \
          -device virtio-net-pci,netdev=guest0 -cpu host

 3. Log into the guest and run dd to write 150MB on /dev/vdb
 4. The VM will stop
 5. Assign extra space to the logical volume, say 200MB
 6. Type 'cont' in the monitor

If I assign chunks of 50MB each time the VM stop in steps 4 and 5,
I have to do it several times (totalizing a logical volume size of 400MB),
so that dd is able to finish.

Testing with commit b96e92470 works as expected, I extend the lv twice
with 50MB (totalizing 200MB) and dd finishes normally.

Small note: I'm writing 157MB in step 3.

Re: [Qemu-devel] [PATCH 3/3] async: Allow nested qemu_bh_poll calls

[Stefan Hajnoczi]

On Thu, Sep 1, 2011 at 3:31 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> qemu may segfault when a BH handler first deletes a BH and then (possibly
> indirectly) calls a nested qemu_bh_poll(). This is because the inner instance
> frees the BH and deletes it from the list that the outer one processes.
>
> This patch deletes BHs only in the outermost qemu_bh_poll instance.
>
> Commit 7887f620 already tried to achieve the same, but it assumed that the BH
> handler would only delete its own BH. With a nested qemu_bh_poll(), this isn't
> guaranteed, so that commit wasn't enough. Hope this one fixes it for real.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  async.c |   24 ++++++++++++++++--------
>  1 files changed, 16 insertions(+), 8 deletions(-)

Seems okay as a fix.

Stefan