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

[Juan Quintela <quintela@redhat.com>]

"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.

>      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?


> +    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.