From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46447) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuE0n-0008AT-3q for qemu-devel@nongnu.org; Thu, 05 Nov 2015 01:27:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZuE0j-0008Nu-Sk for qemu-devel@nongnu.org; Thu, 05 Nov 2015 01:27:05 -0500 Received: from e06smtp12.uk.ibm.com ([195.75.94.108]:57331) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuE0j-0008Nk-JH for qemu-devel@nongnu.org; Thu, 05 Nov 2015 01:27:01 -0500 Received: from localhost by e06smtp12.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 5 Nov 2015 06:26:58 -0000 Received: from b06cxnps3075.portsmouth.uk.ibm.com (d06relay10.portsmouth.uk.ibm.com [9.149.109.195]) by d06dlp03.portsmouth.uk.ibm.com (Postfix) with ESMTP id AC4181B0805F for ; Thu, 5 Nov 2015 06:27:09 +0000 (GMT) Received: from d06av07.portsmouth.uk.ibm.com (d06av07.portsmouth.uk.ibm.com [9.149.37.248]) by b06cxnps3075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id tA56QtqJ3801442 for ; Thu, 5 Nov 2015 06:26:55 GMT Received: from d06av07.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av07.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id tA56Qtrg029084 for ; Wed, 4 Nov 2015 23:26:55 -0700 Date: Thu, 5 Nov 2015 07:26:50 +0100 From: Greg Kurz Message-ID: <20151105072650.6c35f118@bahia.local> In-Reply-To: <1446657582-21619-2-git-send-email-den@openvz.org> References: <1446657582-21619-1-git-send-email-den@openvz.org> <1446657582-21619-2-git-send-email-den@openvz.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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: "Denis V. Lunev" Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi , Juan Quintela 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 > +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