Re: [libvirt] [RFC PATCH] qcow2: Fix race in cache invalidation

Am 23.09.2014 um 10:47 hat Alexey Kardashevskiy geschrieben: > On 09/19/2014 06:47 PM, Kevin Wolf wrote:> Am 16.09.2014 um 14:59 hat Paolo Bonzini geschrieben: >>> Il 16/09/2014 14:52, Kevin Wolf ha scritto: >>>> Yes, that's true. We can't fix this problem in qcow2, though, because >>>> it's a more general one. I think we must make sure that >>>> bdrv_invalidate_cache() doesn't yield. >>>> >>>> Either by forbidding to run bdrv_invalidate_cache() in a coroutine and >>>> moving the problem to the caller (where and why is it even called from a >>>> coroutine?), or possibly by creating a new coroutine for the driver >>>> callback and running that in a nested event loop that only handles >>>> bdrv_invalidate_cache() callbacks, so that the NBD server doesn't get a >>>> chance to process new requests in this thread. >>> >>> Incoming migration runs in a coroutine (the coroutine entry point is >>> process_incoming_migration_co). But everything after qemu_fclose() can >>> probably be moved into a separate bottom half, so that it gets out of >>> coroutine context. >> >> Alexey, you should probably rather try this (and add a bdrv_drain_all() >> in bdrv_invalidate_cache) than messing around with qcow2 locks. This >> isn't a problem that can be completely fixed in qcow2. > > > Ok. Tried :) Not very successful though. The patch is below. > > Is that the correct bottom half? When I did it, I started getting crashes > in various sport on accesses to s->l1_cache which is NULL after qcow2_close. > Normally the code would check s->l1_size and then use but they are out of sync. No, that's not the place we were talking about. What Paolo meant is that in process_incoming_migration_co(), you can split out the final part that calls bdrv_invalidate_cache_all() into a BH (you need to move everything until the end of the function into the BH then). His suggestion was to move everything below the qemu_fclose().