[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 11/11] migration: normalize locking in migration
From: |
Juan Quintela |
Subject: |
Re: [Qemu-devel] [PATCH 11/11] migration: normalize locking in migration/savevm.c |
Date: |
Wed, 18 Nov 2015 12:25:17 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
"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.
> 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.
- [Qemu-devel] [PATCH 06/11] snapshot: create bdrv_all_find_snapshot helper, (continued)
- [Qemu-devel] [PATCH 06/11] snapshot: create bdrv_all_find_snapshot helper, Denis V. Lunev, 2015/11/17
- [Qemu-devel] [PATCH 08/11] snapshot: create bdrv_all_create_snapshot helper, Denis V. Lunev, 2015/11/17
- [Qemu-devel] [PATCH 09/11] migration: reorder processing in hmp_savevm, Denis V. Lunev, 2015/11/17
- [Qemu-devel] [PATCH 10/11] migration: implement bdrv_all_find_vmstate_bs helper, Denis V. Lunev, 2015/11/17
- [Qemu-devel] [PATCH 11/11] migration: normalize locking in migration/savevm.c, Denis V. Lunev, 2015/11/17
- Re: [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes, Juan Quintela, 2015/11/18
- Re: [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes, Juan Quintela, 2015/11/18
- Re: [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes, Greg Kurz, 2015/11/18