From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34192) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuFRq-0002s7-79 for qemu-devel@nongnu.org; Thu, 05 Nov 2015 02:59:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZuFRm-00051U-Uz for qemu-devel@nongnu.org; Thu, 05 Nov 2015 02:59:06 -0500 Received: from relay.parallels.com ([195.214.232.42]:56270) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuFRm-0004zu-Nn for qemu-devel@nongnu.org; Thu, 05 Nov 2015 02:59:02 -0500 References: <1446657582-21619-1-git-send-email-den@openvz.org> <1446657582-21619-2-git-send-email-den@openvz.org> <20151105072650.6c35f118@bahia.local> From: "Denis V. Lunev" Message-ID: <563B0C34.4020004@openvz.org> Date: Thu, 5 Nov 2015 10:58:44 +0300 MIME-Version: 1.0 In-Reply-To: <20151105072650.6c35f118@bahia.local> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 01/11] snapshot: create helper to test that block drivers supports snapshots List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi , Juan Quintela On 11/05/2015 09:26 AM, Greg Kurz wrote: > On Wed, 4 Nov 2015 20:19:32 +0300 > "Denis V. Lunev" wrote: > >> The patch enforces proper locking for this operation. >> >> Signed-off-by: Denis V. Lunev >> CC: Juan Quintela >> CC: Stefan Hajnoczi >> CC: Kevin Wolf >> --- >> block/snapshot.c | 27 +++++++++++++++++++++++++++ >> include/block/snapshot.h | 9 +++++++++ >> migration/savevm.c | 17 ++++------------- >> 3 files changed, 40 insertions(+), 13 deletions(-) >> >> diff --git a/block/snapshot.c b/block/snapshot.c >> index 89500f2..2ef4545 100644 >> --- a/block/snapshot.c >> +++ b/block/snapshot.c >> @@ -356,3 +356,30 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs, >> >> return ret; >> } >> + >> + >> +/* Group operations. All block drivers are involved. >> + * These functions will properly handle dataplace (take aio_context_acquire > s/dataplace/dataplane > >> + * when appropriate for appropriate block drivers > Missing ) and anyway the "(take aio_context_acquire..." part more or less > quotes the code and is not needed. > >> + * >> + * Returned block driver will be always locked. >> + */ >> + >> +bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs) > Juan suggested bdrv_snapshot_is_possible() and FWIW you already emphasize > that it involves all block drivers in the above comment. > >> +{ >> + bool ok = true; >> + BlockDriverState *bs = NULL; >> + >> + while ((bs = bdrv_next(bs)) && ok) { >> + AioContext *ctx = bdrv_get_aio_context(bs); >> + >> + aio_context_acquire(ctx); >> + if (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs)) { >> + ok = bdrv_can_snapshot(bs); >> + } >> + aio_context_release(ctx); >> + } >> + >> + *first_bad_bs = bs; >> + return ok; >> +} >> diff --git a/include/block/snapshot.h b/include/block/snapshot.h >> index 770d9bb..0a3cf0d 100644 >> --- a/include/block/snapshot.h >> +++ b/include/block/snapshot.h >> @@ -75,4 +75,13 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs, >> int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs, >> const char *id_or_name, >> Error **errp); >> + >> + >> +/* Group operations. All block drivers are involved. >> + * These functions will properly handle dataplace (take aio_context_acquire >> + * when appropriate for appropriate block drivers >> + * >> + */ > This comment is not needed here, already in block/snapshot.c it is better to have it twice. >> +bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs); >> + >> #endif >> diff --git a/migration/savevm.c b/migration/savevm.c >> index dbcc39a..3ac3f07 100644 >> --- a/migration/savevm.c >> +++ b/migration/savevm.c >> @@ -1290,19 +1290,10 @@ void hmp_savevm(Monitor *mon, const QDict *qdict) >> const char *name = qdict_get_try_str(qdict, "name"); >> Error *local_err = NULL; >> >> - /* Verify if there is a device that doesn't support snapshots and is writable */ >> - bs = NULL; >> - while ((bs = bdrv_next(bs))) { >> - >> - if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) { >> - continue; >> - } >> - >> - if (!bdrv_can_snapshot(bs)) { >> - monitor_printf(mon, "Device '%s' is writable but does not support snapshots.\n", >> - bdrv_get_device_name(bs)); >> - return; >> - } >> + if (!bdrv_all_can_snapshot(&bs)) { >> + monitor_printf(mon, "Device '%s' is writable but does not " >> + "support snapshots.\n", bdrv_get_device_name(bs)); >> + return; >> } >> >> bs = find_vmstate_bs(); > Reviewed-by: Greg Kurz > Thank you for spelling check.