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

[Denis V. Lunev]

On 11/18/2015 02:25 PM, Juan Quintela wrote:
> "Denis V. Lunev" <address@hidden> wrote:
> > basically all bdrv_* operations must be called under aio_context_acquire
> > except ones with bdrv_all prefix.
> >
> > Signed-off-by: Denis V. Lunev <address@hidden>
> > Reviewed-by: Stefan Hajnoczi <address@hidden>
> > Reviewed-by: Fam Zheng <address@hidden>
> > CC: Juan Quintela <address@hidden>
> > CC: Kevin Wolf <address@hidden>
> 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