Re: [Qemu-devel] [PATCH v2 1/1] qemu-img: wait for convert coroutines to complete

[Peter Lieven]

Am 21.04.2017 um 12:04 schrieb Anton Nefedov:
> On error path (like i/o error in one of the coroutines), it's required to
>   - wait for coroutines completion before cleaning the common structures
>   - reenter dependent coroutines so they ever finish
>
> Introduced in 2d9187bc65.
>
> Signed-off-by: Anton Nefedov <address@hidden>
> ---
>  qemu-img.c | 44 +++++++++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index b220cf7..24950c1 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1735,6 +1735,27 @@ static int coroutine_fn 
> convert_co_write(ImgConvertState *s, int64_t sector_num,
>      return 0;
>  }
>  
> +static void coroutine_fn convert_reenter_waiting(ImgConvertState *s,
> +                                                 uint64_t wr_offs)
> +{
> +    int i;
> +    if (s->wr_in_order) {
> +        /* reenter the coroutine that might have waited
> +         * for the write to complete */
> +        for (i = 0; i < s->num_coroutines; i++) {
> +            if (s->co[i] && s->wait_sector_num[i] == wr_offs) {
> +                /*
> +                 * A -> B -> A cannot occur because A has
> +                 * s->wait_sector_num[i] == -1 during A -> B.  Therefore
> +                 * B will never enter A during this time window.
> +                 */
> +                qemu_coroutine_enter(s->co[i]);
> +                break;
> +            }
> +        }
> +    }
> +}
> +
>  static void coroutine_fn convert_co_do_copy(void *opaque)
>  {
>      ImgConvertState *s = opaque;
> @@ -1792,6 +1813,7 @@ static void coroutine_fn convert_co_do_copy(void 
> *opaque)
>                  error_report("error while reading sector %" PRId64
>                               ": %s", sector_num, strerror(-ret));
>                  s->ret = ret;
> +                convert_reenter_waiting(s, sector_num + n);
>                  goto out;
>              }
>          } else if (!s->min_sparse && status == BLK_ZERO) {
> @@ -1803,6 +1825,7 @@ static void coroutine_fn convert_co_do_copy(void 
> *opaque)
>              /* keep writes in order */
>              while (s->wr_offs != sector_num) {
>                  if (s->ret != -EINPROGRESS) {
> +                    convert_reenter_waiting(s, sector_num + n);
>                      goto out;
>                  }
>                  s->wait_sector_num[index] = sector_num;
> @@ -1816,25 +1839,12 @@ static void coroutine_fn convert_co_do_copy(void 
> *opaque)
>              error_report("error while writing sector %" PRId64
>                           ": %s", sector_num, strerror(-ret));
>              s->ret = ret;
> +            convert_reenter_waiting(s, sector_num + n);
>              goto out;
>          }
>  
> -        if (s->wr_in_order) {
> -            /* reenter the coroutine that might have waited
> -             * for this write to complete */
> -            s->wr_offs = sector_num + n;
> -            for (i = 0; i < s->num_coroutines; i++) {
> -                if (s->co[i] && s->wait_sector_num[i] == s->wr_offs) {
> -                    /*
> -                     * A -> B -> A cannot occur because A has
> -                     * s->wait_sector_num[i] == -1 during A -> B.  Therefore
> -                     * B will never enter A during this time window.
> -                     */
> -                    qemu_coroutine_enter(s->co[i]);
> -                    break;
> -                }
> -            }
> -        }
> +        s->wr_offs = sector_num + n;
> +        convert_reenter_waiting(s, s->wr_offs);
>      }
>  
>  out:
> @@ -1899,7 +1909,7 @@ static int convert_do_copy(ImgConvertState *s)
>          qemu_coroutine_enter(s->co[i]);
>      }
>  
> -    while (s->ret == -EINPROGRESS) {
> +    while (s->running_coroutines) {
>          main_loop_wait(false);
>      }
>  


And what if we error out in the read path? Wouldn't be something like this 
easier?


diff --git a/qemu-img.c b/qemu-img.c
index 22f559a..4ff1085 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1903,6 +1903,16 @@ static int convert_do_copy(ImgConvertState *s)
         main_loop_wait(false);
     }
 
+    /* on error path we need to enter all coroutines that are still
+     * running before cleaning up common structures */
+    if (s->ret) {
+        for (i = 0; i < s->num_coroutines; i++) {
+             if (s->co[i]) {
+                 qemu_coroutine_enter(s->co[i]);
+             }
+        }
+    }
+
     if (s->compressed && !s->ret) {
         /* signal EOF to align */
         ret = blk_pwrite_compressed(s->target, 0, NULL, 0);


Peter