On 2018-08-28 22:25, John Snow wrote:
>
>
> On 08/25/2018 11:02 AM, Max Reitz wrote:
>> If you say so... I have to admit I don't really understand. The
>> comment doesn't explain why it's so important to keep src around until
>> job_completed(), so I don't know. I thought AioContexts are recursive
>> so it doesn't matter whether you take them recursively or not.
>
> bdrv_flush has troubles under a recursive lock if it is invoked from a
> different thread. It tries to poll for flush completion but the
> coroutine which gets scheduled (instead of entered) can't actually run
> if we hold the lock twice from, say, the main thread -- which is where
> we're doing the graph manipulation from.
>
>> co = qemu_coroutine_create(bdrv_flush_co_entry, &flush_co);
>> bdrv_coroutine_enter(bs, co);
>> BDRV_POLL_WHILE(bs, flush_co.ret == NOT_DONE);
>
> BDRV_POLL_WHILE there causes us the grief via AIO_WAIT_WHILE, which only
> puts down one reference, so we deadlock in bdrv_flush in a recursive
> context.
>
> Kevin helped me figure it out; I CC'd him off-list on a mail that you
> were also CC'd on ("hang in mirror_exit") that's probably pretty helpful:
>
>> Took a little more than five minutes, but I think I've got it now. The
>> important thing is that the test case is for dataplane, i.e. the job
>> runs in an I/O thread AioContext. Job completion, however, happens in
>> the main loop thread.
>>
>> Therefore, when bdrv_delete() wants to flush the node first, it needs to
>> run bdrv_co_flush() in a foreign AioContext, so the coroutine is
>> scheduled. The I/O thread backtrace shows that it's waiting for the
>> AioContext lock before it can actually enter the bdrv_co_flush()
>> coroutine, so we must have deadlocked:OK, that makes more sense. I still would have thought that you should always be allowed to take an AioContext lock more than once in a single other AioContext, but on my way through the commit log, I found bd6458e410c1e7, d79df2a2ceb3cb07711, and maybe most importantly 17e2a4a47d4. So maybe not. So at some point we decided that, yeah, you can take them multiple times in a single context, and, yeah, that was how it was designed, but don't do that if you expect a BDRV_POLL_WHILE(). OK. Got it now. Max
signature.asc
Description: OpenPGP digital signature
