Re: [RFC PATCH] migration/savevm: do not schedule snapshot_save_job_bh in qemu_aio_context

[Fiona Ebner <f.ebner@proxmox.com>]

Am 06.06.24 um 20:36 schrieb Stefan Hajnoczi:
> On Wed, Jun 05, 2024 at 02:08:48PM +0200, Fiona Ebner wrote:
>> The fact that the snapshot_save_job_bh() is scheduled in the main
>> loop's qemu_aio_context AioContext means that it might get executed
>> during a vCPU thread's aio_poll(). But saving of the VM state cannot
>> happen while the guest or devices are active and can lead to assertion
>> failures. See issue #2111 for two examples. Avoid the problem by
>> scheduling the snapshot_save_job_bh() in the iohandler AioContext,
>> which is not polled by vCPU threads.
>>
>> Solves Issue #2111.
>>
>> This change also solves the following issue:
>>
>> Since commit effd60c878 ("monitor: only run coroutine commands in
>> qemu_aio_context"), the 'snapshot-save' QMP call would not respond
>> right after starting the job anymore, but only after the job finished,
>> which can take a long time. The reason is, because after commit
>> effd60c878, do_qmp_dispatch_bh() runs in the iohandler AioContext.
>> When do_qmp_dispatch_bh() wakes the qmp_dispatch() coroutine, the
>> coroutine cannot be entered immediately anymore, but needs to be
>> scheduled to the main loop's qemu_aio_context AioContext. But
>> snapshot_save_job_bh() was scheduled first to the same AioContext and
>> thus gets executed first.
>>
>> Buglink: https://gitlab.com/qemu-project/qemu/-/issues/2111
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>> ---
>>
>> While initial smoke testing seems fine, I'm not familiar enough with
>> this to rule out any pitfalls with the approach. Any reason why
>> scheduling to the iohandler AioContext could be wrong here?
> 
> If something waits for a BlockJob to finish using aio_poll() from
> qemu_aio_context then a deadlock is possible since the iohandler_ctx
> won't get a chance to execute. The only suspicious code path I found was
> job_completed_txn_abort_locked() -> job_finish_sync_locked() but I'm not
> sure whether it triggers this scenario. Please check that code path.
> 

Sorry, I don't understand. Isn't executing the scheduled BH the only
additional progress that the iohandler_ctx needs to make compared to
before the patch? How exactly would that cause issues when waiting for a
BlockJob?

Or do you mean something waiting for the SnapshotJob from
qemu_aio_context before snapshot_save_job_bh had the chance to run?

Best Regards,
Fiona