From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40533) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zz0rm-00076I-Lr for qemu-devel@nongnu.org; Wed, 18 Nov 2015 06:25:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zz0rj-0001Pg-SR for qemu-devel@nongnu.org; Wed, 18 Nov 2015 06:25:34 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60658) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zz0rj-0001PZ-Nk for qemu-devel@nongnu.org; Wed, 18 Nov 2015 06:25:31 -0500 From: Juan Quintela In-Reply-To: <1447751311-2317-12-git-send-email-den@openvz.org> (Denis V. Lunev's message of "Tue, 17 Nov 2015 12:08:31 +0300") References: <1447751311-2317-1-git-send-email-den@openvz.org> <1447751311-2317-12-git-send-email-den@openvz.org> Date: Wed, 18 Nov 2015 12:25:17 +0100 Message-ID: <87ziybmtwi.fsf@emacs.mitica> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 11/11] migration: normalize locking in migration/savevm.c Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: Kevin Wolf , qemu-devel@nongnu.org, stefanha@redhat.com "Denis V. Lunev" wrote: > basically all bdrv_* operations must be called under aio_context_acquire > except ones with bdrv_all prefix. > > Signed-off-by: Denis V. Lunev > Reviewed-by: Stefan Hajnoczi > Reviewed-by: Fam Zheng > CC: Juan Quintela > CC: Kevin Wolf 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.