Re: [Qemu-devel] [PATCH for-4.1 2/2] block: Only the main loop can change AioContexts

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3638 bytes --]

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

One thing where I could imagine it becoming relevant in the future is
cross-context block jobs. At the moment, we automatically pull the
target node into the AioContext of the source and fail if this isn't
possible, but that's really overly restrictive.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]