Am 09.05.2023 um 12:26 hat Fiona Ebner geschrieben: > Am 08.05.23 um 12:40 schrieb Kevin Wolf: > > Am 05.05.2023 um 11:35 hat Fiona Ebner geschrieben: > >> Hi, > >> I noticed that the bdrv_graph_co_rd_lock() and bdrv_graph_co_rd_unlock() > >> functions use qemu_in_main_thread() as a conditional to return early. > >> What high-level requirements ensure that qemu_in_main_thread() will > >> evaluate to the same value during locking and unlocking? > > > > I think it's actually wrong. > > > > There are some conditions under which we don't actually need to do > > anything for locking: Writing the graph is only possible from the main > > context while holding the BQL. So if a reader is running in the main > > contextunder the BQL and knows that it won't be interrupted until the > > next writer runs, we don't actually need to do anything. > > > > This is the case if the reader code neither has a nested event loop > > (this is forbidden anyway while you hold the lock) nor is a coroutine > > (because a writer could run when the coroutine has yielded). > > Sorry, I don't understand the first part. If bdrv_writev_vmstate() is > called, then, because it is a generated coroutine wrapper, > AIO_WAIT_WHILE()/aio_poll() is used. And that is the case regardless of > whether the lock is held or not, i.e. there is a nested event loop even > if the lock is held?
With "lock" you mean the graph lock here, not the BQL, right? These generated coroutine wrapper are a bit ugly because they behave different when called from a coroutine and when called outside of coroutine context: * In coroutine context, the caller MUST hold the lock * Outside of coroutine context, the caller MUST NOT hold the lock The second requirement comes from AIO_WAIT_WHILE(), like you say. If you hold the lock and you're not in a coroutine, you simply can't call such functions. However, as bdrv_graph_co_rdlock() is a coroutine_fn, you won't usually hold the lock outside of coroutine context anyway. But it's something to be careful with when you have a writer lock. > > These conditions are exactly what bdrv_graph_rdlock_main_loop() asserts. > > They are not fulfilled in bdrv_graph_co_rdlock, which always runs in a > > coroutine. > > > > Thank you for the explanation! > > >> In the output below, the boolean value after the backtraces of > >> bdrv_graph_co_rd(un)lock is the value of qemu_in_main_thread(). > >> > >> AFAICT, what happened below is: > >> 1. QMP function is executed in the main thread and drops the BQL. > >> 2. bdrv_co_writev_vmstate_entry is called, increasing the reader count, > >> because qemu_in_main_thread() is false. > >> 3. A vCPU thread issued a write, not increasing the reader count, > >> because qemu_in_main_thread() is true. > >> 4. The write is finished in the main thread, decreasing the reader > >> count, because qemu_in_main_thread() is false. > > > > This one is weird. Why don't we hold the BQL there? > > > > Ah, I actually have an idea: Maybe it is executed by the nested event > > loop in bdrv_writev_vmstate(). This nested event loop runs now without > > the BQL, too, and it can call any sort of callback in QEMU that really > > expects to be called with the BQL. Scary! > > > > If this is what happens, you actually have your proof that not holding > > the BQL can cause problems even if there is no other thread active that > > could interfere. > > > > Can you try to confirm this in gdb? In case you haven't debugged > > coroutines much yet: If you have this state in gdb for a running > > instance, you can repeat 'fin' until you reach the next coroutine > > switch, and then you should be able to get the stack trace to see who > > entered the coroutine. > > > > Thank you for the guidance! Hope I did it correctly, but AFAICS, you > are spot-on :) Yes, this looks like what I suspected. Thanks for confirming! Kevin
