Re: [Qemu-devel] [PATCH 11/11] migration: normalize locking in migration/savevm.c

["Denis V. Lunev" <den@openvz.org>]

On 11/18/2015 02:25 PM, Juan Quintela wrote:
> "Denis V. Lunev" <den@openvz.org> wrote:
>> basically all bdrv_* operations must be called under aio_context_acquire
>> except ones with bdrv_all prefix.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
> I will preffer that migration code don't know about aiocontexts.
>
>
>> @@ -2048,9 +2054,12 @@ int load_vmstate(const char *name)
>>           error_report("No block device supports snapshots");
>>           return -ENOTSUP;
>>       }
>> +    aio_context = bdrv_get_aio_context(bs);
>>   
>>       /* Don't even try to load empty VM states */
>> +    aio_context_acquire(aio_context);
>>       ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
>> +    aio_context_release(aio_context);
> Why are we dropping it here?
> I can understand doing it on the error case.
acquire == lock
release == unlock

The lock should be dropped....

>>       if (ret < 0) {
>>           return ret;
>>       } else if (sn.vm_state_size == 0) {
>> @@ -2078,9 +2087,12 @@ int load_vmstate(const char *name)
>>   
>>       qemu_system_reset(VMRESET_SILENT);
>>       migration_incoming_state_new(f);
>> -    ret = qemu_loadvm_state(f);
>>   
>> +    aio_context_acquire(aio_context);
> We have done a qemu_fopen_bdrv() without acquiring the context, not sure
> if we really need it (or not).  My understanding of locking is that we
> should get the context on first use and maintain it until last use.
>
> Does it work in a different way?
>
I was requested to drop that code in one of the versions.
Stefan, Juan, can you pls come into agreement.


>> +    ret = qemu_loadvm_state(f);
>>       qemu_fclose(f);
>> +    aio_context_release(aio_context);
>> +
>>       migration_incoming_state_destroy();
>>       if (ret < 0) {
>>           error_report("Error %d while loading VM state", ret);
>> @@ -2111,14 +2123,19 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
>>       int nb_sns, i;
>>       int total;
>>       int *available_snapshots;
>> +    AioContext *aio_context;
>>   
>>       bs = bdrv_all_find_vmstate_bs();
>>       if (!bs) {
>>           monitor_printf(mon, "No available block device supports snapshots\n");
>>           return;
>>       }
>> +    aio_context = bdrv_get_aio_context(bs);
>>   
>> +    aio_context_acquire(aio_context);
>>       nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>> +    aio_context_release(aio_context);
>> +
>>       if (nb_sns < 0) {
>>           monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
>>           return;
>
> I will very much preffer to have a:
>
>
> bdrv_snapshot_list_full_whatever()
>
> That does the two operations and don't make me know about aio_contexts.
> What I need there really is just the block layer to fill the sn_tab and
> to told me if there is a problem, no real need of knowing about
> contexts, no?
>
> Thanks, Juan.
>
I know :( This is the most thing I can do at the moment keeping 
agreement from Stefan :(

Den