On 23.07.19 10:52, Kevin Wolf wrote: > Am 22.07.2019 um 15:30 hat Max Reitz geschrieben: >> bdrv_set_aio_context_ignore() can only work in the main loop: >> bdrv_drained_begin() only works in the main loop and the node's (old) >> AioContext; and bdrv_drained_end() really only works in the main loop >> and the node's (new) AioContext (contrary to its current comment, which >> is just wrong). >> >> Consequentially, bdrv_set_aio_context_ignore() must be called from the >> main loop. Luckily, assuming that we can make block graph changes only >> from the main loop as well, all its callers do that already. >> >> Note that changing a node's context in a sense is an operation that >> changes the block graph, so it actually makes sense to require this >> function to be called from the main loop. >> >> Also, fix bdrv_drained_end()'s description. You can only use it from >> the main loop or the node's AioContext, and in the latter case, the >> whole subtree must be in the same context. >> >> Fixes: e037c09c78520cbdb6da7cfc6ad0256d5870b814 >> Signed-off-by: Max Reitz <mre...@redhat.com> >> --- >> include/block/block.h | 8 +++----- >> block.c | 13 ++++++++----- >> 2 files changed, 11 insertions(+), 10 deletions(-) >> >> diff --git a/include/block/block.h b/include/block/block.h >> index 60f00479e0..50a07c1c33 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -667,11 +667,9 @@ void bdrv_subtree_drained_begin(BlockDriverState *bs); >> * >> * This polls @bs's AioContext until all scheduled sub-drained_ends >> * have settled. On one hand, that may result in graph changes. On >> - * the other, this requires that all involved nodes (@bs and all of >> - * its parents) are in the same AioContext, and that the caller has >> - * acquired it. >> - * If there are any nodes that are in different contexts from @bs, >> - * these contexts must not be acquired. >> + * the other, this requires that the caller either runs in the main >> + * loop; or that all involved nodes (@bs and all of its parents) are >> + * in the caller's AioContext. >> */ >> void bdrv_drained_end(BlockDriverState *bs); > > I think you are right about the requirement that bdrv_drained_end() is > only called from the main or the BDS AioContext, which is a requirement > that directly comes from AIO_WAIT_WHILE(). > > However, I don't see why we have requirements on the AioContext of the > parent nodes (or any other nodes), except possibly not holding their > lock. We don't poll their context, so it shouldn't matter in which > context they are?
Hm. I don’t know how I got confused there, you’re right. I don’t think the (falsely given) restriction hurts, though, because a subtree should be within a single context anyway (unless you’re in bdrv_set_aio_context_ignore(), which needs to be in the main context). So, hm, yes, I messed up this comment a bit now. But now it’s just more restrictive than it needs to be and I don’t think callers are going to care, so... Max
signature.asc
Description: OpenPGP digital signature
