* [Qemu-devel] [PATCH v2 1/1] qemu-img: wait for convert coroutines to complete @ 2017-04-21 10:04 Anton Nefedov 2017-04-21 10:44 ` Peter Lieven 0 siblings, 1 reply; 10+ messages in thread From: Anton Nefedov @ 2017-04-21 10:04 UTC (permalink / raw) To: qemu-devel; +Cc: den, qemu-block, qemu-stable, kwolf, mreitz, pl, 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 <anton.nefedov@virtuozzo.com> --- 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); } -- 2.7.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] qemu-img: wait for convert coroutines to complete 2017-04-21 10:04 [Qemu-devel] [PATCH v2 1/1] qemu-img: wait for convert coroutines to complete Anton Nefedov @ 2017-04-21 10:44 ` Peter Lieven 2017-04-21 12:19 ` Anton Nefedov 0 siblings, 1 reply; 10+ messages in thread From: Peter Lieven @ 2017-04-21 10:44 UTC (permalink / raw) To: Anton Nefedov, qemu-devel; +Cc: den, qemu-block, qemu-stable, kwolf, mreitz 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 <anton.nefedov@virtuozzo.com> > --- > 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] qemu-img: wait for convert coroutines to complete 2017-04-21 10:44 ` Peter Lieven @ 2017-04-21 12:19 ` Anton Nefedov 2017-04-21 12:37 ` Peter Lieven 0 siblings, 1 reply; 10+ messages in thread From: Anton Nefedov @ 2017-04-21 12:19 UTC (permalink / raw) To: Peter Lieven, qemu-devel; +Cc: den, qemu-block, qemu-stable, kwolf, mreitz On 04/21/2017 01:44 PM, Peter Lieven wrote: > 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 <anton.nefedov@virtuozzo.com> >> --- >> [..] >> > > > 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 > seemed a bit too daring to me to re-enter every coroutine potentially including the ones that yielded waiting for I/O completion. If that's ok - that is for sure easier :) Or maybe some intermediate variant, as you suggest but only enter the "wait_sector" coroutines (and main_loop_wait for the rest)? Or do not even re-enter them, like just - while (s->ret == -EINPROGRESS) { + while (s->running_coroutines != s->waiting_coroutines) { main_loop_wait(false); } /Anton ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] qemu-img: wait for convert coroutines to complete 2017-04-21 12:19 ` Anton Nefedov @ 2017-04-21 12:37 ` Peter Lieven 2017-04-24 16:27 ` Anton Nefedov 0 siblings, 1 reply; 10+ messages in thread From: Peter Lieven @ 2017-04-21 12:37 UTC (permalink / raw) To: Anton Nefedov, qemu-devel; +Cc: den, qemu-block, qemu-stable, kwolf, mreitz Am 21.04.2017 um 14:19 schrieb Anton Nefedov: > On 04/21/2017 01:44 PM, Peter Lieven wrote: >> 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 <anton.nefedov@virtuozzo.com> >>> --- >>> [..] >>> >> >> >> 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 >> > > seemed a bit too daring to me to re-enter every coroutine potentially including the ones that yielded waiting for I/O completion. > If that's ok - that is for sure easier :) I think we should enter every coroutine that is still running and have it terminate correctly. It was my mistake that I have not done this in the original patch. Can you check if the above fixes your test cases that triggered the bug? Peter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] qemu-img: wait for convert coroutines to complete 2017-04-21 12:37 ` Peter Lieven @ 2017-04-24 16:27 ` Anton Nefedov 2017-04-24 18:16 ` [Qemu-devel] [Qemu-stable] " Peter Lieven 0 siblings, 1 reply; 10+ messages in thread From: Anton Nefedov @ 2017-04-24 16:27 UTC (permalink / raw) To: Peter Lieven, qemu-devel; +Cc: den, qemu-block, qemu-stable, kwolf, mreitz On 04/21/2017 03:37 PM, Peter Lieven wrote: > Am 21.04.2017 um 14:19 schrieb Anton Nefedov: >> On 04/21/2017 01:44 PM, Peter Lieven wrote: >>> 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 <anton.nefedov@virtuozzo.com> >>>> --- >>>> [..] >>>> >>> >>> >>> 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 >>> >> >> seemed a bit too daring to me to re-enter every coroutine potentially including the ones that yielded waiting for I/O completion. >> If that's ok - that is for sure easier :) > > I think we should enter every coroutine that is still running and have it terminate correctly. It was my mistake that I have not > done this in the original patch. Can you check if the above fixes your test cases that triggered the bug? > hi, sorry I'm late with the answer this segfaults in bdrv_close(). Looks like it tries to finish some i/o which coroutine we have already entered and terminated? (gdb) run Starting program: /vz/anefedov/qemu-build/us/./qemu-img convert -O qcow2 ./harddisk.hdd.c ./harddisk.hdd [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". [New Thread 0x7fffeac2d700 (LWP 436020)] [New Thread 0x7fffe4ed6700 (LWP 436021)] qemu-img: error while reading sector 20480: Input/output error qemu-img: error while writing sector 19712: Operation now in progress Program received signal SIGSEGV, Segmentation fault. aio_co_wake (co=0x0) at /mnt/code/us-qemu/util/async.c:454 454 ctx = atomic_read(&co->ctx); (gdb) bt #0 aio_co_wake (co=0x0) at /mnt/code/us-qemu/util/async.c:454 /* [Anton]: thread_pool_co_cb () here */ #1 0x0000555555634629 in thread_pool_completion_bh (opaque=0x555555cfe020) at /mnt/code/us-qemu/util/thread-pool.c:189 #2 0x0000555555633b31 in aio_bh_call (bh=0x555555cfe0f0) at /mnt/code/us-qemu/util/async.c:90 #3 aio_bh_poll (ctx=ctx@entry=0x555555cee6d0) at /mnt/code/us-qemu/util/async.c:118 #4 0x0000555555636f14 in aio_poll (ctx=ctx@entry=0x555555cee6d0, blocking=<optimized out>) at /mnt/code/us-qemu/util/aio-posix.c:682 #5 0x00005555555c52d4 in bdrv_drain_recurse (bs=bs@entry=0x555555d22560) at /mnt/code/us-qemu/block/io.c:164 #6 0x00005555555c5aed in bdrv_drained_begin (bs=bs@entry=0x555555d22560) at /mnt/code/us-qemu/block/io.c:248 #7 0x0000555555581443 in bdrv_close (bs=0x555555d22560) at /mnt/code/us-qemu/block.c:2909 #8 bdrv_delete (bs=0x555555d22560) at /mnt/code/us-qemu/block.c:3100 #9 bdrv_unref (bs=0x555555d22560) at /mnt/code/us-qemu/block.c:4087 #10 0x00005555555baf44 in blk_remove_bs (blk=blk@entry=0x555555d22380) at /mnt/code/us-qemu/block/block-backend.c:552 #11 0x00005555555bb173 in blk_delete (blk=0x555555d22380) at /mnt/code/us-qemu/block/block-backend.c:238 #12 blk_unref (blk=blk@entry=0x555555d22380) at /mnt/code/us-qemu/block/block-backend.c:282 #13 0x000055555557a22c in img_convert (argc=<optimized out>, argv=<optimized out>) at /mnt/code/us-qemu/qemu-img.c:2359 #14 0x0000555555574189 in main (argc=5, argv=0x7fffffffe4a0) at /mnt/code/us-qemu/qemu-img.c:4464 > Peter > /Anton ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-stable] [PATCH v2 1/1] qemu-img: wait for convert coroutines to complete 2017-04-24 16:27 ` Anton Nefedov @ 2017-04-24 18:16 ` Peter Lieven 2017-04-24 20:13 ` Anton Nefedov 0 siblings, 1 reply; 10+ messages in thread From: Peter Lieven @ 2017-04-24 18:16 UTC (permalink / raw) To: Anton Nefedov; +Cc: qemu-devel, kwolf, mreitz, den, qemu-block, qemu-stable > Am 24.04.2017 um 18:27 schrieb Anton Nefedov <anton.nefedov@virtuozzo.com>: > >> On 04/21/2017 03:37 PM, Peter Lieven wrote: >>> Am 21.04.2017 um 14:19 schrieb Anton Nefedov: >>>> On 04/21/2017 01:44 PM, Peter Lieven wrote: >>>>> 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 <anton.nefedov@virtuozzo.com> >>>>> --- >>>>> [..] >>>>> >>>> >>>> >>>> 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 >>>> >>> >>> seemed a bit too daring to me to re-enter every coroutine potentially including the ones that yielded waiting for I/O completion. >>> If that's ok - that is for sure easier :) >> >> I think we should enter every coroutine that is still running and have it terminate correctly. It was my mistake that I have not >> done this in the original patch. Can you check if the above fixes your test cases that triggered the bug? >> > > hi, sorry I'm late with the answer > > this segfaults in bdrv_close(). Looks like it tries to finish some i/o which coroutine we have already entered and terminated? > > (gdb) run > Starting program: /vz/anefedov/qemu-build/us/./qemu-img convert -O qcow2 ./harddisk.hdd.c ./harddisk.hdd > [Thread debugging using libthread_db enabled] > Using host libthread_db library "/lib64/libthread_db.so.1". > [New Thread 0x7fffeac2d700 (LWP 436020)] > [New Thread 0x7fffe4ed6700 (LWP 436021)] > qemu-img: error while reading sector 20480: Input/output error > qemu-img: error while writing sector 19712: Operation now in progress > > Program received signal SIGSEGV, Segmentation fault. > aio_co_wake (co=0x0) at /mnt/code/us-qemu/util/async.c:454 > 454 ctx = atomic_read(&co->ctx); > (gdb) bt > #0 aio_co_wake (co=0x0) at /mnt/code/us-qemu/util/async.c:454 > /* [Anton]: thread_pool_co_cb () here */ > #1 0x0000555555634629 in thread_pool_completion_bh (opaque=0x555555cfe020) at /mnt/code/us-qemu/util/thread-pool.c:189 > #2 0x0000555555633b31 in aio_bh_call (bh=0x555555cfe0f0) at /mnt/code/us-qemu/util/async.c:90 > #3 aio_bh_poll (ctx=ctx@entry=0x555555cee6d0) at /mnt/code/us-qemu/util/async.c:118 > #4 0x0000555555636f14 in aio_poll (ctx=ctx@entry=0x555555cee6d0, blocking=<optimized out>) at /mnt/code/us-qemu/util/aio-posix.c:682 > #5 0x00005555555c52d4 in bdrv_drain_recurse (bs=bs@entry=0x555555d22560) at /mnt/code/us-qemu/block/io.c:164 > #6 0x00005555555c5aed in bdrv_drained_begin (bs=bs@entry=0x555555d22560) at /mnt/code/us-qemu/block/io.c:248 > #7 0x0000555555581443 in bdrv_close (bs=0x555555d22560) at /mnt/code/us-qemu/block.c:2909 > #8 bdrv_delete (bs=0x555555d22560) at /mnt/code/us-qemu/block.c:3100 > #9 bdrv_unref (bs=0x555555d22560) at /mnt/code/us-qemu/block.c:4087 > #10 0x00005555555baf44 in blk_remove_bs (blk=blk@entry=0x555555d22380) at /mnt/code/us-qemu/block/block-backend.c:552 > #11 0x00005555555bb173 in blk_delete (blk=0x555555d22380) at /mnt/code/us-qemu/block/block-backend.c:238 > #12 blk_unref (blk=blk@entry=0x555555d22380) at /mnt/code/us-qemu/block/block-backend.c:282 > #13 0x000055555557a22c in img_convert (argc=<optimized out>, argv=<optimized out>) at /mnt/code/us-qemu/qemu-img.c:2359 > #14 0x0000555555574189 in main (argc=5, argv=0x7fffffffe4a0) at /mnt/code/us-qemu/qemu-img.c:4464 > > >> Peter >> > > /Anton > it seems that this is a bit tricky, can you share how your test case works? thanks, peter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-stable] [PATCH v2 1/1] qemu-img: wait for convert coroutines to complete 2017-04-24 18:16 ` [Qemu-devel] [Qemu-stable] " Peter Lieven @ 2017-04-24 20:13 ` Anton Nefedov 2017-04-25 9:55 ` Peter Lieven 0 siblings, 1 reply; 10+ messages in thread From: Anton Nefedov @ 2017-04-24 20:13 UTC (permalink / raw) To: Peter Lieven; +Cc: qemu-devel, kwolf, mreitz, den, qemu-block, qemu-stable On 24/04/2017 21:16, Peter Lieven wrote: > > >> Am 24.04.2017 um 18:27 schrieb Anton Nefedov <anton.nefedov@virtuozzo.com>: >> >>> On 04/21/2017 03:37 PM, Peter Lieven wrote: >>>> Am 21.04.2017 um 14:19 schrieb Anton Nefedov: >>>>> On 04/21/2017 01:44 PM, Peter Lieven wrote: >>>>>> 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 <anton.nefedov@virtuozzo.com> >>>>>> --- >>>>>> [..] >>>>>> >>>>> >>>>> >>>>> 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 >>>>> >>>> >>>> seemed a bit too daring to me to re-enter every coroutine potentially including the ones that yielded waiting for I/O completion. >>>> If that's ok - that is for sure easier :) >>> >>> I think we should enter every coroutine that is still running and have it terminate correctly. It was my mistake that I have not >>> done this in the original patch. Can you check if the above fixes your test cases that triggered the bug? >>> >> >> hi, sorry I'm late with the answer >> >> this segfaults in bdrv_close(). Looks like it tries to finish some i/o which coroutine we have already entered and terminated? >> >> (gdb) run >> Starting program: /vz/anefedov/qemu-build/us/./qemu-img convert -O qcow2 ./harddisk.hdd.c ./harddisk.hdd >> [Thread debugging using libthread_db enabled] >> Using host libthread_db library "/lib64/libthread_db.so.1". >> [New Thread 0x7fffeac2d700 (LWP 436020)] >> [New Thread 0x7fffe4ed6700 (LWP 436021)] >> qemu-img: error while reading sector 20480: Input/output error >> qemu-img: error while writing sector 19712: Operation now in progress >> >> Program received signal SIGSEGV, Segmentation fault. >> aio_co_wake (co=0x0) at /mnt/code/us-qemu/util/async.c:454 >> 454 ctx = atomic_read(&co->ctx); >> (gdb) bt >> #0 aio_co_wake (co=0x0) at /mnt/code/us-qemu/util/async.c:454 >> /* [Anton]: thread_pool_co_cb () here */ >> #1 0x0000555555634629 in thread_pool_completion_bh (opaque=0x555555cfe020) at /mnt/code/us-qemu/util/thread-pool.c:189 >> #2 0x0000555555633b31 in aio_bh_call (bh=0x555555cfe0f0) at /mnt/code/us-qemu/util/async.c:90 >> #3 aio_bh_poll (ctx=ctx@entry=0x555555cee6d0) at /mnt/code/us-qemu/util/async.c:118 >> #4 0x0000555555636f14 in aio_poll (ctx=ctx@entry=0x555555cee6d0, blocking=<optimized out>) at /mnt/code/us-qemu/util/aio-posix.c:682 >> #5 0x00005555555c52d4 in bdrv_drain_recurse (bs=bs@entry=0x555555d22560) at /mnt/code/us-qemu/block/io.c:164 >> #6 0x00005555555c5aed in bdrv_drained_begin (bs=bs@entry=0x555555d22560) at /mnt/code/us-qemu/block/io.c:248 >> #7 0x0000555555581443 in bdrv_close (bs=0x555555d22560) at /mnt/code/us-qemu/block.c:2909 >> #8 bdrv_delete (bs=0x555555d22560) at /mnt/code/us-qemu/block.c:3100 >> #9 bdrv_unref (bs=0x555555d22560) at /mnt/code/us-qemu/block.c:4087 >> #10 0x00005555555baf44 in blk_remove_bs (blk=blk@entry=0x555555d22380) at /mnt/code/us-qemu/block/block-backend.c:552 >> #11 0x00005555555bb173 in blk_delete (blk=0x555555d22380) at /mnt/code/us-qemu/block/block-backend.c:238 >> #12 blk_unref (blk=blk@entry=0x555555d22380) at /mnt/code/us-qemu/block/block-backend.c:282 >> #13 0x000055555557a22c in img_convert (argc=<optimized out>, argv=<optimized out>) at /mnt/code/us-qemu/qemu-img.c:2359 >> #14 0x0000555555574189 in main (argc=5, argv=0x7fffffffe4a0) at /mnt/code/us-qemu/qemu-img.c:4464 >> >> >>> Peter >>> >> >> /Anton >> > > it seems that this is a bit tricky, can you share how your test case works? > > thanks, > peter > how I tested today was basically diff --git a/qemu-img.c b/qemu-img.c index 4425aaa..3d2d506 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1788,6 +1788,10 @@ static void coroutine_fn convert_co_do_copy(void *opaque) if (status == BLK_DATA) { ret = convert_co_read(s, sector_num, n, buf); + const uint64_t fsector = 10*1024*1024/512; + if (sector_num <= fsector && fsector < sector_num+n) { + ret = -EIO; + } if (ret < 0) { error_report("error while reading sector %" PRId64 ": %s", sector_num, strerror(-ret)); ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-stable] [PATCH v2 1/1] qemu-img: wait for convert coroutines to complete 2017-04-24 20:13 ` Anton Nefedov @ 2017-04-25 9:55 ` Peter Lieven 2017-04-25 13:27 ` Anton Nefedov 2017-04-25 13:39 ` Peter Lieven 0 siblings, 2 replies; 10+ messages in thread From: Peter Lieven @ 2017-04-25 9:55 UTC (permalink / raw) To: Anton Nefedov; +Cc: qemu-devel, kwolf, mreitz, den, qemu-block, qemu-stable Am 24.04.2017 um 22:13 schrieb Anton Nefedov: > > On 24/04/2017 21:16, Peter Lieven wrote: >> >> >>> Am 24.04.2017 um 18:27 schrieb Anton Nefedov <anton.nefedov@virtuozzo.com>: >>> >>>> On 04/21/2017 03:37 PM, Peter Lieven wrote: >>>>> Am 21.04.2017 um 14:19 schrieb Anton Nefedov: >>>>>> On 04/21/2017 01:44 PM, Peter Lieven wrote: >>>>>>> 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 <anton.nefedov@virtuozzo.com> >>>>>>> --- >>>>>>> [..] >>>>>>> >>>>>> >>>>>> >>>>>> 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 >>>>>> >>>>> >>>>> seemed a bit too daring to me to re-enter every coroutine potentially including the ones that yielded waiting for I/O completion. >>>>> If that's ok - that is for sure easier :) >>>> >>>> I think we should enter every coroutine that is still running and have it terminate correctly. It was my mistake that I have not >>>> done this in the original patch. Can you check if the above fixes your test cases that triggered the bug? >>>> >>> >>> hi, sorry I'm late with the answer >>> >>> this segfaults in bdrv_close(). Looks like it tries to finish some i/o which coroutine we have already entered and terminated? >>> >>> (gdb) run >>> Starting program: /vz/anefedov/qemu-build/us/./qemu-img convert -O qcow2 ./harddisk.hdd.c ./harddisk.hdd >>> [Thread debugging using libthread_db enabled] >>> Using host libthread_db library "/lib64/libthread_db.so.1". >>> [New Thread 0x7fffeac2d700 (LWP 436020)] >>> [New Thread 0x7fffe4ed6700 (LWP 436021)] >>> qemu-img: error while reading sector 20480: Input/output error >>> qemu-img: error while writing sector 19712: Operation now in progress >>> >>> Program received signal SIGSEGV, Segmentation fault. >>> aio_co_wake (co=0x0) at /mnt/code/us-qemu/util/async.c:454 >>> 454 ctx = atomic_read(&co->ctx); >>> (gdb) bt >>> #0 aio_co_wake (co=0x0) at /mnt/code/us-qemu/util/async.c:454 >>> /* [Anton]: thread_pool_co_cb () here */ >>> #1 0x0000555555634629 in thread_pool_completion_bh (opaque=0x555555cfe020) at /mnt/code/us-qemu/util/thread-pool.c:189 >>> #2 0x0000555555633b31 in aio_bh_call (bh=0x555555cfe0f0) at /mnt/code/us-qemu/util/async.c:90 >>> #3 aio_bh_poll (ctx=ctx@entry=0x555555cee6d0) at /mnt/code/us-qemu/util/async.c:118 >>> #4 0x0000555555636f14 in aio_poll (ctx=ctx@entry=0x555555cee6d0, blocking=<optimized out>) at /mnt/code/us-qemu/util/aio-posix.c:682 >>> #5 0x00005555555c52d4 in bdrv_drain_recurse (bs=bs@entry=0x555555d22560) at /mnt/code/us-qemu/block/io.c:164 >>> #6 0x00005555555c5aed in bdrv_drained_begin (bs=bs@entry=0x555555d22560) at /mnt/code/us-qemu/block/io.c:248 >>> #7 0x0000555555581443 in bdrv_close (bs=0x555555d22560) at /mnt/code/us-qemu/block.c:2909 >>> #8 bdrv_delete (bs=0x555555d22560) at /mnt/code/us-qemu/block.c:3100 >>> #9 bdrv_unref (bs=0x555555d22560) at /mnt/code/us-qemu/block.c:4087 >>> #10 0x00005555555baf44 in blk_remove_bs (blk=blk@entry=0x555555d22380) at /mnt/code/us-qemu/block/block-backend.c:552 >>> #11 0x00005555555bb173 in blk_delete (blk=0x555555d22380) at /mnt/code/us-qemu/block/block-backend.c:238 >>> #12 blk_unref (blk=blk@entry=0x555555d22380) at /mnt/code/us-qemu/block/block-backend.c:282 >>> #13 0x000055555557a22c in img_convert (argc=<optimized out>, argv=<optimized out>) at /mnt/code/us-qemu/qemu-img.c:2359 >>> #14 0x0000555555574189 in main (argc=5, argv=0x7fffffffe4a0) at /mnt/code/us-qemu/qemu-img.c:4464 >>> >>> >>>> Peter >>>> >>> >>> /Anton >>> >> >> it seems that this is a bit tricky, can you share how your test case works? >> >> thanks, >> peter >> > > how I tested today was basically > > diff --git a/qemu-img.c b/qemu-img.c > index 4425aaa..3d2d506 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1788,6 +1788,10 @@ static void coroutine_fn convert_co_do_copy(void *opaque) > > if (status == BLK_DATA) { > ret = convert_co_read(s, sector_num, n, buf); > + const uint64_t fsector = 10*1024*1024/512; > + if (sector_num <= fsector && fsector < sector_num+n) { > + ret = -EIO; > + } > if (ret < 0) { > error_report("error while reading sector %" PRId64 > ": %s", sector_num, strerror(-ret)); > I ended up with sth similar to your approach. Can you check this? Thanks, Peter diff --git a/qemu-img.c b/qemu-img.c index b94fc11..ed9ce9e 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1761,13 +1761,13 @@ static void coroutine_fn convert_co_do_copy(void *opaque) qemu_co_mutex_lock(&s->lock); if (s->ret != -EINPROGRESS || s->sector_num >= s->total_sectors) { qemu_co_mutex_unlock(&s->lock); - goto out; + break; } n = convert_iteration_sectors(s, s->sector_num); if (n < 0) { qemu_co_mutex_unlock(&s->lock); s->ret = n; - goto out; + break; } /* save current sector and allocation status to local variables */ sector_num = s->sector_num; @@ -1792,7 +1792,6 @@ 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; - goto out; } } else if (!s->min_sparse && status == BLK_ZERO) { status = BLK_DATA; @@ -1801,22 +1800,20 @@ static void coroutine_fn convert_co_do_copy(void *opaque) if (s->wr_in_order) { /* keep writes in order */ - while (s->wr_offs != sector_num) { - if (s->ret != -EINPROGRESS) { - goto out; - } + while (s->wr_offs != sector_num && s->ret == -EINPROGRESS) { s->wait_sector_num[index] = sector_num; qemu_coroutine_yield(); } s->wait_sector_num[index] = -1; } - ret = convert_co_write(s, sector_num, n, buf, status); - if (ret < 0) { - error_report("error while writing sector %" PRId64 - ": %s", sector_num, strerror(-ret)); - s->ret = ret; - goto out; + if (s->ret == -EINPROGRESS) { + ret = convert_co_write(s, sector_num, n, buf, status); + if (ret < 0) { + error_report("error while writing sector %" PRId64 + ": %s", sector_num, strerror(-ret)); + s->ret = ret; + } } if (s->wr_in_order) { @@ -1837,7 +1834,6 @@ static void coroutine_fn convert_co_do_copy(void *opaque) } } -out: qemu_vfree(buf); s->co[index] = NULL; s->running_coroutines--; @@ -1899,7 +1895,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); } ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-stable] [PATCH v2 1/1] qemu-img: wait for convert coroutines to complete 2017-04-25 9:55 ` Peter Lieven @ 2017-04-25 13:27 ` Anton Nefedov 2017-04-25 13:39 ` Peter Lieven 1 sibling, 0 replies; 10+ messages in thread From: Anton Nefedov @ 2017-04-25 13:27 UTC (permalink / raw) To: Peter Lieven; +Cc: qemu-devel, kwolf, mreitz, den, qemu-block, qemu-stable On 04/25/2017 12:55 PM, Peter Lieven wrote: > Am 24.04.2017 um 22:13 schrieb Anton Nefedov: >> >> On 24/04/2017 21:16, Peter Lieven wrote: >>> >>> >>>> Am 24.04.2017 um 18:27 schrieb Anton Nefedov >>>> <anton.nefedov@virtuozzo.com>: >>>> >>>>> On 04/21/2017 03:37 PM, Peter Lieven wrote: >>>>>> Am 21.04.2017 um 14:19 schrieb Anton Nefedov: >>>>>>> On 04/21/2017 01:44 PM, Peter Lieven wrote: >>>>>>>> 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 <anton.nefedov@virtuozzo.com> >>>>>>>> --- >>>>>>>> [..] >>>>>>>> >>>>>>> >>>>>>> >>>>>>> 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 >>>>>>> >>>>>> >>>>>> seemed a bit too daring to me to re-enter every coroutine >>>>>> potentially including the ones that yielded waiting for I/O >>>>>> completion. >>>>>> If that's ok - that is for sure easier :) >>>>> >>>>> I think we should enter every coroutine that is still running and >>>>> have it terminate correctly. It was my mistake that I have not >>>>> done this in the original patch. Can you check if the above fixes >>>>> your test cases that triggered the bug? >>>>> >>>> >>>> hi, sorry I'm late with the answer >>>> >>>> this segfaults in bdrv_close(). Looks like it tries to finish some >>>> i/o which coroutine we have already entered and terminated? >>>> >>>> (gdb) run >>>> Starting program: /vz/anefedov/qemu-build/us/./qemu-img convert -O >>>> [..] >>>> >>>> >>>>> Peter >>>>> >>>> >>>> /Anton >>>> >>> >>> it seems that this is a bit tricky, can you share how your test case >>> works? >>> >>> thanks, >>> peter >>> >> >> how I tested today was basically >> >> diff --git a/qemu-img.c b/qemu-img.c >> index 4425aaa..3d2d506 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -1788,6 +1788,10 @@ static void coroutine_fn >> convert_co_do_copy(void *opaque) >> >> if (status == BLK_DATA) { >> ret = convert_co_read(s, sector_num, n, buf); >> + const uint64_t fsector = 10*1024*1024/512; >> + if (sector_num <= fsector && fsector < sector_num+n) { >> + ret = -EIO; >> + } >> if (ret < 0) { >> error_report("error while reading sector %" PRId64 >> ": %s", sector_num, strerror(-ret)); >> > > I ended up with sth similar to your approach. Can you check this? > > > Thanks, > > Peter > > > diff --git a/qemu-img.c b/qemu-img.c > index b94fc11..ed9ce9e 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1761,13 +1761,13 @@ static void coroutine_fn convert_co_do_copy(void > *opaque) > qemu_co_mutex_lock(&s->lock); > if (s->ret != -EINPROGRESS || s->sector_num >= s->total_sectors) { > qemu_co_mutex_unlock(&s->lock); > - goto out; > + break; > } > n = convert_iteration_sectors(s, s->sector_num); > if (n < 0) { > qemu_co_mutex_unlock(&s->lock); > s->ret = n; > - goto out; > + break; > } > /* save current sector and allocation status to local variables */ > sector_num = s->sector_num; > @@ -1792,7 +1792,6 @@ 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; > - goto out; > } > } else if (!s->min_sparse && status == BLK_ZERO) { > status = BLK_DATA; > @@ -1801,22 +1800,20 @@ static void coroutine_fn convert_co_do_copy(void > *opaque) > > if (s->wr_in_order) { > /* keep writes in order */ > - while (s->wr_offs != sector_num) { > - if (s->ret != -EINPROGRESS) { > - goto out; > - } > + while (s->wr_offs != sector_num && s->ret == -EINPROGRESS) { > s->wait_sector_num[index] = sector_num; > qemu_coroutine_yield(); > } > s->wait_sector_num[index] = -1; > } > > - ret = convert_co_write(s, sector_num, n, buf, status); > - if (ret < 0) { > - error_report("error while writing sector %" PRId64 > - ": %s", sector_num, strerror(-ret)); > - s->ret = ret; > - goto out; > + if (s->ret == -EINPROGRESS) { > + ret = convert_co_write(s, sector_num, n, buf, status); > + if (ret < 0) { > + error_report("error while writing sector %" PRId64 > + ": %s", sector_num, strerror(-ret)); > + s->ret = ret; > + } > } > > if (s->wr_in_order) { > @@ -1837,7 +1834,6 @@ static void coroutine_fn convert_co_do_copy(void > *opaque) > } > } > > -out: > qemu_vfree(buf); > s->co[index] = NULL; > s->running_coroutines--; > @@ -1899,7 +1895,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); > } > > This one seems to work; I've been running for several hours qemu-img convert from nbd which spontaneously fails in ~ every 2nd convert; and yet qemu-img keeps terminating smoothly /Anton ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-stable] [PATCH v2 1/1] qemu-img: wait for convert coroutines to complete 2017-04-25 9:55 ` Peter Lieven 2017-04-25 13:27 ` Anton Nefedov @ 2017-04-25 13:39 ` Peter Lieven 1 sibling, 0 replies; 10+ messages in thread From: Peter Lieven @ 2017-04-25 13:39 UTC (permalink / raw) To: Anton Nefedov; +Cc: qemu-devel, kwolf, mreitz, den, qemu-block, qemu-stable Am 25.04.2017 um 11:55 schrieb Peter Lieven: > Am 24.04.2017 um 22:13 schrieb Anton Nefedov: >> >> On 24/04/2017 21:16, Peter Lieven wrote: >>> >>> >>>> Am 24.04.2017 um 18:27 schrieb Anton Nefedov <anton.nefedov@virtuozzo.com>: >>>> >>>>> On 04/21/2017 03:37 PM, Peter Lieven wrote: >>>>>> Am 21.04.2017 um 14:19 schrieb Anton Nefedov: >>>>>>> On 04/21/2017 01:44 PM, Peter Lieven wrote: >>>>>>>> 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 <anton.nefedov@virtuozzo.com> >>>>>>>> --- >>>>>>>> [..] >>>>>>>> >>>>>>> >>>>>>> >>>>>>> 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 >>>>>>> >>>>>> >>>>>> seemed a bit too daring to me to re-enter every coroutine potentially including the ones that yielded waiting for I/O completion. >>>>>> If that's ok - that is for sure easier :) >>>>> >>>>> I think we should enter every coroutine that is still running and have it terminate correctly. It was my mistake that I have not >>>>> done this in the original patch. Can you check if the above fixes your test cases that triggered the bug? >>>>> >>>> >>>> hi, sorry I'm late with the answer >>>> >>>> this segfaults in bdrv_close(). Looks like it tries to finish some i/o which coroutine we have already entered and terminated? >>>> >>>> (gdb) run >>>> Starting program: /vz/anefedov/qemu-build/us/./qemu-img convert -O qcow2 ./harddisk.hdd.c ./harddisk.hdd >>>> [Thread debugging using libthread_db enabled] >>>> Using host libthread_db library "/lib64/libthread_db.so.1". >>>> [New Thread 0x7fffeac2d700 (LWP 436020)] >>>> [New Thread 0x7fffe4ed6700 (LWP 436021)] >>>> qemu-img: error while reading sector 20480: Input/output error >>>> qemu-img: error while writing sector 19712: Operation now in progress >>>> >>>> Program received signal SIGSEGV, Segmentation fault. >>>> aio_co_wake (co=0x0) at /mnt/code/us-qemu/util/async.c:454 >>>> 454 ctx = atomic_read(&co->ctx); >>>> (gdb) bt >>>> #0 aio_co_wake (co=0x0) at /mnt/code/us-qemu/util/async.c:454 >>>> /* [Anton]: thread_pool_co_cb () here */ >>>> #1 0x0000555555634629 in thread_pool_completion_bh (opaque=0x555555cfe020) at /mnt/code/us-qemu/util/thread-pool.c:189 >>>> #2 0x0000555555633b31 in aio_bh_call (bh=0x555555cfe0f0) at /mnt/code/us-qemu/util/async.c:90 >>>> #3 aio_bh_poll (ctx=ctx@entry=0x555555cee6d0) at /mnt/code/us-qemu/util/async.c:118 >>>> #4 0x0000555555636f14 in aio_poll (ctx=ctx@entry=0x555555cee6d0, blocking=<optimized out>) at /mnt/code/us-qemu/util/aio-posix.c:682 >>>> #5 0x00005555555c52d4 in bdrv_drain_recurse (bs=bs@entry=0x555555d22560) at /mnt/code/us-qemu/block/io.c:164 >>>> #6 0x00005555555c5aed in bdrv_drained_begin (bs=bs@entry=0x555555d22560) at /mnt/code/us-qemu/block/io.c:248 >>>> #7 0x0000555555581443 in bdrv_close (bs=0x555555d22560) at /mnt/code/us-qemu/block.c:2909 >>>> #8 bdrv_delete (bs=0x555555d22560) at /mnt/code/us-qemu/block.c:3100 >>>> #9 bdrv_unref (bs=0x555555d22560) at /mnt/code/us-qemu/block.c:4087 >>>> #10 0x00005555555baf44 in blk_remove_bs (blk=blk@entry=0x555555d22380) at /mnt/code/us-qemu/block/block-backend.c:552 >>>> #11 0x00005555555bb173 in blk_delete (blk=0x555555d22380) at /mnt/code/us-qemu/block/block-backend.c:238 >>>> #12 blk_unref (blk=blk@entry=0x555555d22380) at /mnt/code/us-qemu/block/block-backend.c:282 >>>> #13 0x000055555557a22c in img_convert (argc=<optimized out>, argv=<optimized out>) at /mnt/code/us-qemu/qemu-img.c:2359 >>>> #14 0x0000555555574189 in main (argc=5, argv=0x7fffffffe4a0) at /mnt/code/us-qemu/qemu-img.c:4464 >>>> >>>> >>>>> Peter >>>>> >>>> >>>> /Anton >>>> >>> >>> it seems that this is a bit tricky, can you share how your test case works? >>> >>> thanks, >>> peter >>> >> >> how I tested today was basically >> >> diff --git a/qemu-img.c b/qemu-img.c >> index 4425aaa..3d2d506 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -1788,6 +1788,10 @@ static void coroutine_fn convert_co_do_copy(void *opaque) >> >> if (status == BLK_DATA) { >> ret = convert_co_read(s, sector_num, n, buf); >> + const uint64_t fsector = 10*1024*1024/512; >> + if (sector_num <= fsector && fsector < sector_num+n) { >> + ret = -EIO; >> + } >> if (ret < 0) { >> error_report("error while reading sector %" PRId64 >> ": %s", sector_num, strerror(-ret)); >> > > I ended up with sth similar to your approach. Can you check this? > > > Thanks, > > Peter > > > diff --git a/qemu-img.c b/qemu-img.c > index b94fc11..ed9ce9e 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1761,13 +1761,13 @@ static void coroutine_fn convert_co_do_copy(void *opaque) > qemu_co_mutex_lock(&s->lock); > if (s->ret != -EINPROGRESS || s->sector_num >= s->total_sectors) { > qemu_co_mutex_unlock(&s->lock); > - goto out; > + break; > } > n = convert_iteration_sectors(s, s->sector_num); > if (n < 0) { > qemu_co_mutex_unlock(&s->lock); > s->ret = n; > - goto out; > + break; > } > /* save current sector and allocation status to local variables */ > sector_num = s->sector_num; > @@ -1792,7 +1792,6 @@ 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; > - goto out; > } > } else if (!s->min_sparse && status == BLK_ZERO) { > status = BLK_DATA; > @@ -1801,22 +1800,20 @@ static void coroutine_fn convert_co_do_copy(void *opaque) > > if (s->wr_in_order) { > /* keep writes in order */ > - while (s->wr_offs != sector_num) { > - if (s->ret != -EINPROGRESS) { > - goto out; > - } > + while (s->wr_offs != sector_num && s->ret == -EINPROGRESS) { > s->wait_sector_num[index] = sector_num; > qemu_coroutine_yield(); > } > s->wait_sector_num[index] = -1; > } > > - ret = convert_co_write(s, sector_num, n, buf, status); > - if (ret < 0) { > - error_report("error while writing sector %" PRId64 > - ": %s", sector_num, strerror(-ret)); > - s->ret = ret; > - goto out; > + if (s->ret == -EINPROGRESS) { > + ret = convert_co_write(s, sector_num, n, buf, status); > + if (ret < 0) { > + error_report("error while writing sector %" PRId64 > + ": %s", sector_num, strerror(-ret)); > + s->ret = ret; > + } > } > > if (s->wr_in_order) { > @@ -1837,7 +1834,6 @@ static void coroutine_fn convert_co_do_copy(void *opaque) > } > } > > -out: > qemu_vfree(buf); > s->co[index] = NULL; > s->running_coroutines--; > @@ -1899,7 +1895,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); > } > > Great. Shall we take this one? I would prefer it because it avoids the separate convert_reenter_waiting function. If you want you can take my code and submit a V3. Peter ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-04-25 16:02 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-04-21 10:04 [Qemu-devel] [PATCH v2 1/1] qemu-img: wait for convert coroutines to complete Anton Nefedov 2017-04-21 10:44 ` Peter Lieven 2017-04-21 12:19 ` Anton Nefedov 2017-04-21 12:37 ` Peter Lieven 2017-04-24 16:27 ` Anton Nefedov 2017-04-24 18:16 ` [Qemu-devel] [Qemu-stable] " Peter Lieven 2017-04-24 20:13 ` Anton Nefedov 2017-04-25 9:55 ` Peter Lieven 2017-04-25 13:27 ` Anton Nefedov 2017-04-25 13:39 ` Peter Lieven
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).