Am 08.05.2025 um 16:09 hat Fiona Ebner geschrieben: > bdrv_drained_begin() polls and is not allowed to be called with the > block graph lock held. Mark the function as such. > > Suggested-by: Kevin Wolf <kw...@redhat.com> > Signed-off-by: Fiona Ebner <f.eb...@proxmox.com> > --- > > This does not catch the issue reported by Andrey, because there > is a bdrv_graph_rdunlock_main_loop() before bdrv_drained_begin() in > the function bdrv_change_aio_context(). That unlock is of course > ineffective if the exclusive lock is held, but it seems to prevent TSA > from finiding the issue.
Yes, GRAPH_UNLOCKED is weak. It doesn't actually prove that the graph is unlocked, but just flags direct callers that are known to hold the lock. So because bdrv_try_change_aio_context() and bdrv_change_aio_context() don't have TSA annotations, it misses the problem. If they had an annotation, it would find the problem. (If they were GRAPH_UNLOCKED, because they can't be called, or if they were GRAPH_RDLOCK/WRLOCK because of double locking.) TSA does have negative requirements that would prove it, but that would essentially mean annotating everything in QEMU, even outside the block layer, down to main() as GRAPH_UNLOCKED. I don't think that's practical. Hmm, or maybe it wouldn't be that bad if we could have something like assume_graph_unlocked()...? Not for this series anyway. > The next patch is concerned with that. > > include/block/block-io.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/block/block-io.h b/include/block/block-io.h > index b49e0537dd..34b9f1cbfc 100644 > --- a/include/block/block-io.h > +++ b/include/block/block-io.h > @@ -429,7 +429,8 @@ bdrv_drain_poll(BlockDriverState *bs, BdrvChild > *ignore_parent, > * > * This function can be recursive. > */ > -void bdrv_drained_begin(BlockDriverState *bs); > +void GRAPH_UNLOCKED > +bdrv_drained_begin(BlockDriverState *bs); No need for the line break, it fits easily on a single line. Reviewed-by: Kevin Wolf <kw...@redhat.com>
