Re: [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes

[Juan Quintela]

Greg Kurz <address@hidden> wrote:
> On Tue, 17 Nov 2015 12:08:20 +0300
> "Denis V. Lunev" <address@hidden> wrote:
>
>> with test
>>     while /bin/true ; do
>>         virsh snapshot-create rhel7
>>         sleep 10
>>         virsh snapshot-delete rhel7 --current
>>     done
>> with enabled iothreads on a running VM leads to a lot of troubles: hangs,
>> asserts, errors.
>> 
>
> In my case, when using a virtio-blk-dataplane device, calling savevm *always*
> result in a QEMU hang.

Oops

> With this series (plus the s/bs/bs_vm_state/ change in patch 11), 
> savevm/loadvm
> now works like a charm.

Nice, thanks for the testing.

> I saw that Juan does not like aio_context being used in migration code, but
> in case this series gets applied anyway:
>
> Tested-by: Greg Kurz <address@hidden>

I *think* that we should get better API's exported from block layer, but
*at least* we will get this series in.

Thanks, Juan.


>
>> Anyway, I think that the construction like
>>     assert(aio_context_is_locked(aio_context));
>> should be widely used to ensure proper locking.
>> 
>> Changes from v8:
>> - split patch 5 into 2 separate patches making changes better to understand 
>> and
>>   review
>> 
>> Changes from v7:
>> - patch 5: fixes for bdrv_all_find_snapshot. Changed parameter name to
>>   skip_read_only which better reflects the meaning of the value and
>>   fixed checks inside to conform the note from Stefan
>> 
>> Changes from v6:
>> - tricky part dropped from patch 7
>> - patch 5 reworked to process snapshot list differently in info commands
>>   and on savevm
>> 
>> Changes from v5:
>> - dropped already merged patch 11
>> - fixed spelling in patch 1
>> - changed order of condition in loops in all patches. Thank you Stefan
>> - dropped patch 9
>> - aio_context is not acquired any more in bdrv_all_find_vmstate_bs by request
>>   of Stefan
>> - patch 10 is implemented in completely different way
>> 
>> Changes from v4:
>> - only migration/savevm.c code and monitor is affected now. Generic block
>>   layer stuff will be sent separately to speedup merging. The approach
>>   in general was negotiated with Juan and Stefan.
>> 
>> Changes from v3:
>> - more places found
>> - new aio_poll concept, see patch 10
>> 
>> Changes from v2:
>> - droppped patch 5 as already merged
>> - changed locking scheme in patch 4 by suggestion of Juan
>> 
>> Changes from v1:
>> - aio-context locking added
>> - comment is rewritten
>> 
>> Signed-off-by: Denis V. Lunev <address@hidden>
>> CC: Stefan Hajnoczi <address@hidden>
>> CC: Juan Quintela <address@hidden>
>> CC: Kevin Wolf <address@hidden>
>> 
>> Denis V. Lunev (10):
>>   snapshot: create helper to test that block drivers supports snapshots
>>   snapshot: return error code from bdrv_snapshot_delete_by_id_or_name
>>   snapshot: create bdrv_all_delete_snapshot helper
>>   snapshot: create bdrv_all_goto_snapshot helper
>>   snapshot: create bdrv_all_find_snapshot helper
>>   migration: drop find_vmstate_bs check in hmp_delvm
>>   snapshot: create bdrv_all_create_snapshot helper
>>   migration: reorder processing in hmp_savevm
>>   migration: implement bdrv_all_find_vmstate_bs helper
>>   migration: normalize locking in migration/savevm.c
>> 
>>  block/snapshot.c         | 135 ++++++++++++++++++++++++++++++-
>>  include/block/snapshot.h |  25 +++++-
>>  migration/savevm.c       | 207 
>> +++++++++++++++--------------------------------
>>  3 files changed, 217 insertions(+), 150 deletions(-)