Re: [PATCH v2 09/24] block: move drain outside of bdrv_try_change_aio_context()

[Kevin Wolf <kwolf@redhat.com>]

Am 22.05.2025 um 11:56 hat Fiona Ebner geschrieben:
> 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.

Oh, I see. I was thinking much too complicated once again.

Maybe just add "the appropriate variant, depending on whether the caller
already holds the lock"?

> > 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.

Doesn't hurt, but it's also not really needed.

Kevin