Re: [RFC PATCH 0/3] block: Synchronous bdrv_*() from coroutine in different AioContext

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

14.05.2020 17:26, Kevin Wolf wrote:
> Am 14.05.2020 um 15:21 hat Thomas Lamprecht geschrieben:
>> On 5/12/20 4:43 PM, Kevin Wolf wrote:
>>> Stefan (Reiter), after looking a bit closer at this, I think there is no
>>> bug in QEMU, but the bug is in your coroutine code that calls block
>>> layer functions without moving into the right AioContext first. I've
>>> written this series anyway as it potentially makes the life of callers
>>> easier and would probably make your buggy code correct.
>>
>>> However, it doesn't feel right to commit something like patch 2 without
>>> having a user for it. Is there a reason why you can't upstream your
>>> async snapshot code?
>>
>> I mean I understand what you mean, but it would make the interface IMO so
>> much easier to use, if one wants to explicit schedule it beforehand they
>> can still do. But that would open the way for two styles doing things, not
>> sure if this would seen as bad. The assert about from patch 3/3 would be
>> already really helping a lot, though.
> 
> I think patches 1 and 3 are good to be committed either way if people
> think they are useful. They make sense without the async snapshot code.
> 
> My concern with the interface in patch 2 is both that it could give
> people a false sense of security and that it would be tempting to write
> inefficient code.
> 
> Usually, you won't have just a single call into the block layer for a
> given block node, but you'll perform multiple operations. Switching to
> the target context once rather than switching back and forth in every
> operation is obviously more efficient.
> 
> But chances are that even if one of these function is bdrv_flush(),
> which now works correctly from a different thread, you might need
> another function that doesn't implement the same magic. So you always
> need to be aware which functions support cross-context calls which
> ones don't.
> 
> I feel we'd see a few bugs related to this.
> 
>> Regarding upstreaming, there was some historical attempt to upstream it
>> from Dietmar, but in the time frame of ~ 8 to 10 years ago or so.
>> I'm not quite sure why it didn't went through then, I see if I can get
>> some time searching the mailing list archive.
>>
>> We'd be naturally open and glad to upstream it, what it effectively
>> allow us to do is to not block the VM to much during snapshoting it
>> live.
> 
> Yes, there is no doubt that this is useful functionality. There has been
> talk about this every now and then, but I don't think we ever got to a
> point where it actually could be implemented.
> 
> Vladimir, I seem to remember you (or someone else from your team?) were
> interested in async snapshots as well a while ago?

Den is working on this (add him to CC)

> 
>> I pushed a tree[0] with mostly just that specific code squashed together (hope
>> I did not break anything), most of the actual code is in commit [1].
>> It'd be cleaned up a bit and checked for coding style issues, but works good
>> here.
>>
>> Anyway, thanks for your help and pointers!
>>
>> [0]: https://github.com/ThomasLamprecht/qemu/tree/savevm-async
>> [1]: https://github.com/ThomasLamprecht/qemu/commit/ffb9531f370ef0073e4b6f6021f4c47ccd702121
> 
> It doesn't even look that bad in terms of patch size. I had imagined it
> a bit larger.
> 
> But it seems this is not really just an async 'savevm' (which would save
> the VM state in a qcow2 file), but you store the state in a separate
> raw file. What is the difference between this and regular migration into
> a file?
> 
> I remember people talking about how snapshotting can store things in a
> way that a normal migration stream can't do, like overwriting outdated
> RAM state instead of just appending the new state, but you don't seem to
> implement something like this.
> 
> Kevin
> 


-- 
Best regards,
Vladimir