From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36838) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zz3hd-0000lz-06 for qemu-devel@nongnu.org; Wed, 18 Nov 2015 09:27:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zz3hY-0006No-0x for qemu-devel@nongnu.org; Wed, 18 Nov 2015 09:27:16 -0500 Received: from relay.parallels.com ([195.214.232.42]:40705) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zz3hX-0006NR-PY for qemu-devel@nongnu.org; Wed, 18 Nov 2015 09:27:11 -0500 References: <1447751311-2317-1-git-send-email-den@openvz.org> <1447751311-2317-12-git-send-email-den@openvz.org> <87ziybmtwi.fsf@emacs.mitica> From: "Denis V. Lunev" Message-ID: <564C8AAF.3060904@openvz.org> Date: Wed, 18 Nov 2015 17:26:55 +0300 MIME-Version: 1.0 In-Reply-To: <87ziybmtwi.fsf@emacs.mitica> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 11/11] migration: normalize locking in migration/savevm.c List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: quintela@redhat.com Cc: Kevin Wolf , qemu-devel@nongnu.org, stefanha@redhat.com On 11/18/2015 02:25 PM, Juan Quintela wrote: > "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. 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