[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH for-4.1 2/2] block: Only the main loop can chang
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH for-4.1 2/2] block: Only the main loop can change AioContexts |
Date: |
Tue, 23 Jul 2019 12:21:07 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 |
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 <address@hidden>
>>>> ---
>>>> 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.
Max
> 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
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH for-4.1 0/2] block: bdrv_drained_end() changes fallout, Max Reitz, 2019/07/22