Message ID | 5423E686.20109@ozlabs.ru |
---|---|
State | New |
Headers | show |
Am 25.09.2014 um 11:55 hat Alexey Kardashevskiy geschrieben: > Right. Cool. So is below what was suggested? I am doublechecking as it does > not solve the original issue - the bottomhalf is called first and then > nbd_trip() crashes in qcow2_co_flush_to_os(). > > diff --git a/block.c b/block.c > index d06dd51..1e6dfd1 100644 > --- a/block.c > +++ b/block.c > @@ -5037,20 +5037,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs, > Error **errp) > if (local_err) { > error_propagate(errp, local_err); > return; > } > > ret = refresh_total_sectors(bs, bs->total_sectors); > if (ret < 0) { > error_setg_errno(errp, -ret, "Could not refresh total sector count"); > return; > } > + > + bdrv_drain_all(); > } Try moving the bdrv_drain_all() call to the top of the function (at least it must be called before bs->drv->bdrv_invalidate_cache). > +static QEMUBH *migration_complete_bh; > +static void process_incoming_migration_complete(void *opaque); > + > static void process_incoming_migration_co(void *opaque) > { > QEMUFile *f = opaque; > - Error *local_err = NULL; > int ret; > > ret = qemu_loadvm_state(f); > qemu_fclose(f); Paolo suggested to move eveything starting from here, but as far as I can tell, leaving the next few lines here shouldn't hurt. > free_xbzrle_decoded_buf(); > if (ret < 0) { > error_report("load of migration failed: %s", strerror(-ret)); > exit(EXIT_FAILURE); > } > qemu_announce_self(); > > bdrv_clear_incoming_migration_all(); > + > + migration_complete_bh = aio_bh_new(qemu_get_aio_context(), > + process_incoming_migration_complete, > + NULL); > + qemu_bh_schedule(migration_complete_bh); > +} > + > +static void process_incoming_migration_complete(void *opaque) > +{ > + Error *local_err = NULL; > + > /* Make sure all file formats flush their mutable metadata */ > bdrv_invalidate_cache_all(&local_err); > if (local_err) { > qerror_report_err(local_err); > error_free(local_err); > exit(EXIT_FAILURE); > } > > if (autostart) { > vm_start(); > } else { > runstate_set(RUN_STATE_PAUSED); > } > + qemu_bh_delete(migration_complete_bh); > + migration_complete_bh = NULL; > } That part looks good to me. I hope moving bdrv_drain_all() does the trick, otherwise there's somthing wrong with our reasoning. Kevin
On 09/25/2014 08:20 PM, Kevin Wolf wrote: > Am 25.09.2014 um 11:55 hat Alexey Kardashevskiy geschrieben: >> Right. Cool. So is below what was suggested? I am doublechecking as it does >> not solve the original issue - the bottomhalf is called first and then >> nbd_trip() crashes in qcow2_co_flush_to_os(). >> >> diff --git a/block.c b/block.c >> index d06dd51..1e6dfd1 100644 >> --- a/block.c >> +++ b/block.c >> @@ -5037,20 +5037,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs, >> Error **errp) >> if (local_err) { >> error_propagate(errp, local_err); >> return; >> } >> >> ret = refresh_total_sectors(bs, bs->total_sectors); >> if (ret < 0) { >> error_setg_errno(errp, -ret, "Could not refresh total sector count"); >> return; >> } >> + >> + bdrv_drain_all(); >> } > > Try moving the bdrv_drain_all() call to the top of the function (at > least it must be called before bs->drv->bdrv_invalidate_cache). Ok, I did. Did not help. > >> +static QEMUBH *migration_complete_bh; >> +static void process_incoming_migration_complete(void *opaque); >> + >> static void process_incoming_migration_co(void *opaque) >> { >> QEMUFile *f = opaque; >> - Error *local_err = NULL; >> int ret; >> >> ret = qemu_loadvm_state(f); >> qemu_fclose(f); > > Paolo suggested to move eveything starting from here, but as far as I > can tell, leaving the next few lines here shouldn't hurt. Ouch. I was looking at wrong qcow2_fclose() all this time :) Aaaany what you suggested did not help - bdrv_co_flush() calls qemu_coroutine_yield() while this BH is being executed and the situation is still the same. > >> free_xbzrle_decoded_buf(); >> if (ret < 0) { >> error_report("load of migration failed: %s", strerror(-ret)); >> exit(EXIT_FAILURE); >> } >> qemu_announce_self(); >> >> bdrv_clear_incoming_migration_all(); >> + >> + migration_complete_bh = aio_bh_new(qemu_get_aio_context(), >> + process_incoming_migration_complete, >> + NULL); >> + qemu_bh_schedule(migration_complete_bh); >> +} >> + >> +static void process_incoming_migration_complete(void *opaque) >> +{ >> + Error *local_err = NULL; >> + >> /* Make sure all file formats flush their mutable metadata */ >> bdrv_invalidate_cache_all(&local_err); >> if (local_err) { >> qerror_report_err(local_err); >> error_free(local_err); >> exit(EXIT_FAILURE); >> } >> >> if (autostart) { >> vm_start(); >> } else { >> runstate_set(RUN_STATE_PAUSED); >> } >> + qemu_bh_delete(migration_complete_bh); >> + migration_complete_bh = NULL; >> } > > That part looks good to me. I hope moving bdrv_drain_all() does the > trick, otherwise there's somthing wrong with our reasoning. > > Kevin >
Am 25.09.2014 um 14:29 hat Alexey Kardashevskiy geschrieben: > On 09/25/2014 08:20 PM, Kevin Wolf wrote: > > Am 25.09.2014 um 11:55 hat Alexey Kardashevskiy geschrieben: > >> Right. Cool. So is below what was suggested? I am doublechecking as it does > >> not solve the original issue - the bottomhalf is called first and then > >> nbd_trip() crashes in qcow2_co_flush_to_os(). > >> > >> diff --git a/block.c b/block.c > >> index d06dd51..1e6dfd1 100644 > >> --- a/block.c > >> +++ b/block.c > >> @@ -5037,20 +5037,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs, > >> Error **errp) > >> if (local_err) { > >> error_propagate(errp, local_err); > >> return; > >> } > >> > >> ret = refresh_total_sectors(bs, bs->total_sectors); > >> if (ret < 0) { > >> error_setg_errno(errp, -ret, "Could not refresh total sector count"); > >> return; > >> } > >> + > >> + bdrv_drain_all(); > >> } > > > > Try moving the bdrv_drain_all() call to the top of the function (at > > least it must be called before bs->drv->bdrv_invalidate_cache). > > > Ok, I did. Did not help. > > > > > >> +static QEMUBH *migration_complete_bh; > >> +static void process_incoming_migration_complete(void *opaque); > >> + > >> static void process_incoming_migration_co(void *opaque) > >> { > >> QEMUFile *f = opaque; > >> - Error *local_err = NULL; > >> int ret; > >> > >> ret = qemu_loadvm_state(f); > >> qemu_fclose(f); > > > > Paolo suggested to move eveything starting from here, but as far as I > > can tell, leaving the next few lines here shouldn't hurt. > > > Ouch. I was looking at wrong qcow2_fclose() all this time :) > Aaaany what you suggested did not help - > bdrv_co_flush() calls qemu_coroutine_yield() while this BH is being > executed and the situation is still the same. Hm, do you have a backtrace? The idea with the BH was that it would be executed _outside_ coroutine context and therefore wouldn't be able to yield. If it's still executed in coroutine context, it would be interesting to see who that caller is. Kevin
On 09/25/2014 10:39 PM, Kevin Wolf wrote: > Am 25.09.2014 um 14:29 hat Alexey Kardashevskiy geschrieben: >> On 09/25/2014 08:20 PM, Kevin Wolf wrote: >>> Am 25.09.2014 um 11:55 hat Alexey Kardashevskiy geschrieben: >>>> Right. Cool. So is below what was suggested? I am doublechecking as it does >>>> not solve the original issue - the bottomhalf is called first and then >>>> nbd_trip() crashes in qcow2_co_flush_to_os(). >>>> >>>> diff --git a/block.c b/block.c >>>> index d06dd51..1e6dfd1 100644 >>>> --- a/block.c >>>> +++ b/block.c >>>> @@ -5037,20 +5037,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs, >>>> Error **errp) >>>> if (local_err) { >>>> error_propagate(errp, local_err); >>>> return; >>>> } >>>> >>>> ret = refresh_total_sectors(bs, bs->total_sectors); >>>> if (ret < 0) { >>>> error_setg_errno(errp, -ret, "Could not refresh total sector count"); >>>> return; >>>> } >>>> + >>>> + bdrv_drain_all(); >>>> } >>> >>> Try moving the bdrv_drain_all() call to the top of the function (at >>> least it must be called before bs->drv->bdrv_invalidate_cache). >> >> >> Ok, I did. Did not help. >> >> >>> >>>> +static QEMUBH *migration_complete_bh; >>>> +static void process_incoming_migration_complete(void *opaque); >>>> + >>>> static void process_incoming_migration_co(void *opaque) >>>> { >>>> QEMUFile *f = opaque; >>>> - Error *local_err = NULL; >>>> int ret; >>>> >>>> ret = qemu_loadvm_state(f); >>>> qemu_fclose(f); >>> >>> Paolo suggested to move eveything starting from here, but as far as I >>> can tell, leaving the next few lines here shouldn't hurt. >> >> >> Ouch. I was looking at wrong qcow2_fclose() all this time :) >> Aaaany what you suggested did not help - >> bdrv_co_flush() calls qemu_coroutine_yield() while this BH is being >> executed and the situation is still the same. > > Hm, do you have a backtrace? The idea with the BH was that it would be > executed _outside_ coroutine context and therefore wouldn't be able to > yield. If it's still executed in coroutine context, it would be > interesting to see who that caller is. Like this? process_incoming_migration_complete bdrv_invalidate_cache_all bdrv_drain_all aio_dispatch node->io_read (which is nbd_read) nbd_trip bdrv_co_flush [...]
On 09/26/2014 12:05 AM, Alexey Kardashevskiy wrote: > On 09/25/2014 10:39 PM, Kevin Wolf wrote: >> Am 25.09.2014 um 14:29 hat Alexey Kardashevskiy geschrieben: >>> On 09/25/2014 08:20 PM, Kevin Wolf wrote: >>>> Am 25.09.2014 um 11:55 hat Alexey Kardashevskiy geschrieben: >>>>> Right. Cool. So is below what was suggested? I am doublechecking as it does >>>>> not solve the original issue - the bottomhalf is called first and then >>>>> nbd_trip() crashes in qcow2_co_flush_to_os(). >>>>> >>>>> diff --git a/block.c b/block.c >>>>> index d06dd51..1e6dfd1 100644 >>>>> --- a/block.c >>>>> +++ b/block.c >>>>> @@ -5037,20 +5037,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs, >>>>> Error **errp) >>>>> if (local_err) { >>>>> error_propagate(errp, local_err); >>>>> return; >>>>> } >>>>> >>>>> ret = refresh_total_sectors(bs, bs->total_sectors); >>>>> if (ret < 0) { >>>>> error_setg_errno(errp, -ret, "Could not refresh total sector count"); >>>>> return; >>>>> } >>>>> + >>>>> + bdrv_drain_all(); >>>>> } >>>> >>>> Try moving the bdrv_drain_all() call to the top of the function (at >>>> least it must be called before bs->drv->bdrv_invalidate_cache). >>> >>> >>> Ok, I did. Did not help. >>> >>> >>>> >>>>> +static QEMUBH *migration_complete_bh; >>>>> +static void process_incoming_migration_complete(void *opaque); >>>>> + >>>>> static void process_incoming_migration_co(void *opaque) >>>>> { >>>>> QEMUFile *f = opaque; >>>>> - Error *local_err = NULL; >>>>> int ret; >>>>> >>>>> ret = qemu_loadvm_state(f); >>>>> qemu_fclose(f); >>>> >>>> Paolo suggested to move eveything starting from here, but as far as I >>>> can tell, leaving the next few lines here shouldn't hurt. >>> >>> >>> Ouch. I was looking at wrong qcow2_fclose() all this time :) >>> Aaaany what you suggested did not help - >>> bdrv_co_flush() calls qemu_coroutine_yield() while this BH is being >>> executed and the situation is still the same. >> >> Hm, do you have a backtrace? The idea with the BH was that it would be >> executed _outside_ coroutine context and therefore wouldn't be able to >> yield. If it's still executed in coroutine context, it would be >> interesting to see who that caller is. > > Like this? > process_incoming_migration_complete > bdrv_invalidate_cache_all > bdrv_drain_all > aio_dispatch > node->io_read (which is nbd_read) > nbd_trip > bdrv_co_flush > [...] Ping? I do not know how to understand this backtrace - in fact, in gdb at the moment of crash I only see traces up to nbd_trip and coroutine_trampoline (below). What is the context here then?... Program received signal SIGSEGV, Segmentation fault. 0x000000001050a8d4 in qcow2_cache_flush (bs=0x100363531a0, c=0x0) at /home/alexey/p/qemu/block/qcow2-cache.c:174 (gdb) bt #0 0x000000001050a8d4 in qcow2_cache_flush (bs=0x100363531a0, c=0x0) at /home/alexey/p/qemu/block/qcow2-cache.c:174 #1 0x00000000104fbc4c in qcow2_co_flush_to_os (bs=0x100363531a0) at /home/alexey/p/qemu/block/qcow2.c:2162 #2 0x00000000104c7234 in bdrv_co_flush (bs=0x100363531a0) at /home/alexey/p/qemu/block.c:4978 #3 0x00000000104b7e68 in nbd_trip (opaque=0x1003653e530) at /home/alexey/p/qemu/nbd.c:1260 #4 0x00000000104d7d84 in coroutine_trampoline (i0=0x100, i1=0x36549850) at /home/alexey/p/qemu/coroutine-ucontext.c:118 #5 0x000000804db01a9c in .__makecontext () from /lib64/libc.so.6 #6 0x0000000000000000 in ?? ()
diff --git a/block.c b/block.c index d06dd51..1e6dfd1 100644 --- a/block.c +++ b/block.c @@ -5037,20 +5037,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) if (local_err) { error_propagate(errp, local_err); return; } ret = refresh_total_sectors(bs, bs->total_sectors); if (ret < 0) { error_setg_errno(errp, -ret, "Could not refresh total sector count"); return; } + + bdrv_drain_all(); } void bdrv_invalidate_cache_all(Error **errp) { BlockDriverState *bs; Error *local_err = NULL; QTAILQ_FOREACH(bs, &bdrv_states, device_list) { AioContext *aio_context = bdrv_get_aio_context(bs); diff --git a/migration.c b/migration.c index 6db04a6..dc026a9 100644 --- a/migration.c +++ b/migration.c @@ -81,49 +81,64 @@ void qemu_start_incoming_migration(const char *uri, Error **errp) else if (strstart(uri, "unix:", &p)) unix_start_incoming_migration(p, errp); else if (strstart(uri, "fd:", &p)) fd_start_incoming_migration(p, errp); #endif else { error_setg(errp, "unknown migration protocol: %s", uri); } } +static QEMUBH *migration_complete_bh; +static void process_incoming_migration_complete(void *opaque); + static void process_incoming_migration_co(void *opaque) { QEMUFile *f = opaque; - Error *local_err = NULL; int ret; ret = qemu_loadvm_state(f); qemu_fclose(f); free_xbzrle_decoded_buf(); if (ret < 0) { error_report("load of migration failed: %s", strerror(-ret)); exit(EXIT_FAILURE); } qemu_announce_self(); bdrv_clear_incoming_migration_all(); + + migration_complete_bh = aio_bh_new(qemu_get_aio_context(), + process_incoming_migration_complete, + NULL); + qemu_bh_schedule(migration_complete_bh); +} + +static void process_incoming_migration_complete(void *opaque) +{ + Error *local_err = NULL; + /* Make sure all file formats flush their mutable metadata */ bdrv_invalidate_cache_all(&local_err); if (local_err) { qerror_report_err(local_err); error_free(local_err); exit(EXIT_FAILURE); } if (autostart) { vm_start(); } else { runstate_set(RUN_STATE_PAUSED); } + qemu_bh_delete(migration_complete_bh); + migration_complete_bh = NULL; } -- Alexey