Am 21.05.25 um 18:36 schrieb Kevin Wolf: > Am 20.05.2025 um 12:29 hat Fiona Ebner geschrieben: >> This is part of resolving the deadlock mentioned in commit "block: >> move draining out of bdrv_change_aio_context() and mark GRAPH_RDLOCK". >> >> Convert the function to a _locked() version that has to be called with >> the graph lock held and add a convenience wrapper that has to be >> called with the graph unlocked, which drains and takes the lock >> itself. Since bdrv_try_change_aio_context() is global state code, the >> wrapper is too. >> >> Callers are adapted to use the appropriate variant. In the >> test_set_aio_context() unit test, prior drains can be removed, because >> draining already happens inside the new wrapper. > > I'm not sure what's the difference between qmp_x_blockdev_set_iothread() > which is converted to _locked() and e.g. qmp_blockdev_mirror().
For deciding on the variant, I only looked at whether the caller holds the graph lock or not. > The reason I could see in qmp_x_blockdev_set_iothread() is that (as we > discussed in the RFC version of this series) draining can change the > graph and that could in theory invalidate bs. But the same is true for > other functions in blockdev.c. Right, my patches are not enough to address that kind of problem. > If it's just that qmp_x_blockdev_set_iothread() was easy to fix and the > others are more complex, that's fine with me, but maybe worth mentioning > in the commit message. Yes, I can mention something in the commit message, maybe: Functions like qmp_blockdev_mirror() query the nodes to act on before draining and locking. In theory, draining could invalidate those nodes. This kind of issue is not addressed by these commits. Best Regards, Fiona
