Am 23.07.2019 um 12:21 hat Max Reitz geschrieben: > On 23.07.19 12:02, Kevin Wolf wrote: > > Am 23.07.2019 um 11:41 hat Max Reitz geschrieben: > >> 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... > > > > Nothing that should hold up your pull request, but I'd prefer to fix the > > comment in a follow-up. > > On second thought, does aio_wait_kick() wake up any context but the main > context? I was under the impression that it doesn’t. If it doesn’t, I > don’t know how bdrv_drained_end()’s AIO_WAIT_WHILE() will be woken up if > it doesn’t run in the main context and it has to wait for yet another > thread.
Hm, I think you're right. And your comment actually says main loop _or_ everything in the same context, so it's correct (but I misread it at first and thought the condition would always apply). Kevin
signature.asc
Description: PGP signature
