Peter Xu <[email protected]> wrote: Hi
[Adding Kevin to the party] > On Fri, May 05, 2023 at 03:46:52PM +0200, Fiona Ebner wrote: >> To fix it, ensure that the BQL is held during setup. To avoid changing >> the behavior for migration too, introduce conditionals for the setup >> callbacks that need the BQL and only take the lock if it's not already >> held. > > The major complexity of this patch is the "conditionally taking" part. Yeap. I don't want that bit. This is another case of: - I have a problem - I will use recursive mutexes to solve it Now you have two problems O:-) > Pure question: what is the benefit of not holding BQL always during > save_setup(), if after all we have this coroutine issue to be solved? Dunno. I would like that paolo commented on this. I "reviewed the code" 10 years ago. I don't remember at all why we wanted to change that. > I can understand that it helps us to avoid taking BQL too long, but we'll > need to take it anyway during ramblock dirty track initializations, and so > far IIUC it's the major time to be consumed during setup(). > > Commit message of 9b0950375277467 says, "Only the migration_bitmap_sync() > call needs the iothread lock". Firstly I think it's also covering > enablement of dirty tracking: > > + qemu_mutex_lock_iothread(); > + qemu_mutex_lock_ramlist(); > + bytes_transferred = 0; > + reset_ram_globals(); > + > memory_global_dirty_log_start(); > migration_bitmap_sync(); > + qemu_mutex_unlock_iothread(); > > And I think enablement itself can be slow too, maybe even slower than > migration_bitmap_sync() especially with KVM_DIRTY_LOG_INITIALLY_SET > supported in the kernel. > > Meanwhile I always got confused on why we need to sync dirty bitmap when > setup at all. Say, what if we drop migration_bitmap_sync() here? After > all, shouldn't all pages be dirty from start (ram_list_init_bitmaps())? How do you convince KVM (or the other lists) to start doing dirty tracking? Doing a bitmap sync. And yeap, probably there is a better way of doing it. > This is slightly off-topic, but I'd like to know if someone can help > answer. > > My whole point is still questioning whether we can unconditionally take bql > during save_setup(). I agree with you. > I could have missed something, though, where we want to do in setup() but > avoid holding BQL. Help needed on figuring this out (and if there is, IMHO > it'll be worthwhile to put that into comment of save_setup() hook). I am more towards revert completely 9b0950375277467fd74a9075624477ae43b9bb22 and call it a day. On migration we don't use coroutines on the sending side (I mean the migration code, the block layer uses coroutines for everything/anything). Paolo, Stefan any clues for not run setup with the BKL? Later, Juan.
