[Qemu-devel] [PATCH] qemu-img: fix in-flight count for qemu-img bench

[Qemu-devel] [PATCH] qemu-img: fix in-flight count for qemu-img bench

[Paolo Bonzini]

With aio=native (qemu-img bench -n) one or more requests can be completed
when a new request is submitted.  This in turn can cause bench_cb to
recurse before b->in_flight is updated.  The blk_aio_pwritev coroutines
are never freed, and qemu-img aborts.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-img.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 6949b73..607dbe5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3559,6 +3559,9 @@ static void bench_cb(void *opaque, int ret)
     }
 
     while (b->n > b->in_flight && b->in_flight < b->nrreq) {
+        b->in_flight++;
+        b->offset += b->step;
+        b->offset %= b->image_size;
         if (b->write) {
             acb = blk_aio_pwritev(b->blk, b->offset, b->qiov, 0,
                                   bench_cb, b);
@@ -3570,9 +3573,6 @@ static void bench_cb(void *opaque, int ret)
             error_report("Failed to issue request");
             exit(EXIT_FAILURE);
         }
-        b->in_flight++;
-        b->offset += b->step;
-        b->offset %= b->image_size;
     }
 }
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH] qemu-img: fix in-flight count for qemu-img bench

[Kevin Wolf]

Am 06.12.2016 um 12:08 hat Paolo Bonzini geschrieben:
> With aio=native (qemu-img bench -n) one or more requests can be completed
> when a new request is submitted.  This in turn can cause bench_cb to
> recurse before b->in_flight is updated.  The blk_aio_pwritev coroutines
> are never freed, and qemu-img aborts.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qemu-img.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 6949b73..607dbe5 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3559,6 +3559,9 @@ static void bench_cb(void *opaque, int ret)
>      }
>  
>      while (b->n > b->in_flight && b->in_flight < b->nrreq) {
> +        b->in_flight++;
> +        b->offset += b->step;
> +        b->offset %= b->image_size;
>          if (b->write) {
>              acb = blk_aio_pwritev(b->blk, b->offset, b->qiov, 0,
>                                    bench_cb, b);

This implicitly adds b->step to the initial offset because the write
request now uses the already updated offset. We should probably save the
old value and use that for the request.

Also, maybe add a short comment to the code (rather than just to the
commit message) that explains why the update has to be first?

Kevin