From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34366) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YMOtL-0001CU-6M for qemu-devel@nongnu.org; Fri, 13 Feb 2015 17:39:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YMOtH-0005uQ-2x for qemu-devel@nongnu.org; Fri, 13 Feb 2015 17:39:19 -0500 Received: from mx1.redhat.com ([209.132.183.28]:44525) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YMOtG-0005uC-R6 for qemu-devel@nongnu.org; Fri, 13 Feb 2015 17:39:15 -0500 Message-ID: <54DE7D10.9080703@redhat.com> Date: Fri, 13 Feb 2015 17:39:12 -0500 From: John Snow MIME-Version: 1.0 References: <1423532117-14490-1-git-send-email-jsnow@redhat.com> <1423532117-14490-3-git-send-email-jsnow@redhat.com> <54DE79B7.3030802@redhat.com> In-Reply-To: <54DE79B7.3030802@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v12 02/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, armbru@redhat.com, mreitz@redhat.com, vsementsov@parallels.com, stefanha@redhat.com Missed you by several seconds, I submitted a v13 fixup to cover Max's comments and had wrongly assumed I wouldn't be hearing anything else at 5PM on a Friday :) On 02/13/2015 05:24 PM, Eric Blake wrote: > On 02/09/2015 06:35 PM, John Snow wrote: >> The new command pair is added to manage user created dirty bitmap. The >> dirty bitmap's name is mandatory and must be unique for the same device, >> but different devices can have bitmaps with the same names. > > I haven't been following this series closely (I know I should be doing > that, though). Is the bitmap associated with the BDS (a host resource, > independent of which device(s) are currently viewing that content) or > with the BlockBackend (only one bitmap namespace per device)? I'm a bit > worried that we will WANT to have bitmaps associated with BDS (if we > don't already) because of image fleecing. That is, if we start with: > > base <- mid <- active > > and request an image fleecing operation, we want: > > base <- mid <- active > \- overlay > Currently: I am intending to allow users to attach them to any old BDS, per-node. However, they are currently only /useful/ if you attach them to the root, since that's what Drive Backup is going to operate on. > where overlay serves the NBD that sees the point in time. If we then > allow a block-commit, then writes to 'mid' containing the content from > 'active' will trigger another write to 'overlay' with the pre-modified > contents, so that the NBD fleecing operation doesn't see any changes. > If we then migrate, it means we need multiple bitmaps: the map for the > commit of active into mid (how much remains to be committed), and the > map for mid to overlay (how much of mid has been changed since the > point-in-time overlay was created). > > By associating bitmaps with a device (a BB), rather than a BDS, you may > be artificially limiting which operations can be performed. On the > other hand, if you associate with a BDS, and we improve things to allow > arbitrary refactoring relationships where a BDS can be in more than one > tree at once, it starts to be hard to prove that bitmap names won't be > duplicated. > > Am I overthinking something here, or are we okay limiting bitmap names > to just the BB device, rather than a BDS? > Not my intention to create an artificial limitation, just an implicit one: There are currently no users of named BdrvDirtyBitmaps that act per-node. You may review this series under this premise and yell at me if I have diverged from this assumption. >> >> The granularity is an optional field. If it is not specified, we will >> choose a default granularity based on the cluster size if available, >> clamped to between 4K and 64K to mirror how the 'mirror' code was >> already choosing granularity. If we do not have cluster size info >> available, we choose 64K. This code has been factored out into a helper >> shared with block/mirror. >> >> This patch also introduces the 'block_dirty_bitmap_lookup' helper, >> which takes a device name and a dirty bitmap name and validates the >> lookup, returning NULL and setting errp if there is a problem with >> either field. This helper will be re-used in future patches in this >> series. >> >> The types added to block-core.json will be re-used in future patches >> in this series, see: >> 'qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}' >> >> Signed-off-by: Fam Zheng >> Signed-off-by: John Snow >> --- >> block.c | 20 ++++++++++ >> block/mirror.c | 10 +---- >> blockdev.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> include/block/block.h | 1 + >> qapi/block-core.json | 55 +++++++++++++++++++++++++++ >> qmp-commands.hx | 51 +++++++++++++++++++++++++ >> 6 files changed, 228 insertions(+), 9 deletions(-) >> > >> +++ b/blockdev.c >> @@ -1173,6 +1173,48 @@ out_aio_context: >> return NULL; >> } >> >> +/** >> + * Return a dirty bitmap (if present), after validating >> + * the node reference and bitmap names. Returns NULL on error, >> + * including when the BDS and/or bitmap is not found. >> + */ >> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node, >> + const char *name, >> + BlockDriverState **pbs, >> + Error **errp) >> +{ >> + BlockDriverState *bs; >> + BdrvDirtyBitmap *bitmap; >> + >> + if (!node) { >> + error_setg(errp, "Node cannot be NULL"); >> + return NULL; >> + } > > Node tends to be the term we use for BDS, rather than for device BB. > So far so good. >> + if (!name) { >> + error_setg(errp, "Bitmap name cannot be NULL"); >> + return NULL; >> + } >> + >> + bs = bdrv_lookup_bs(node, node, NULL); >> + if (!bs) { >> + error_setg(errp, "Node '%s' not found", node); >> + return NULL; >> + } >> + >> + /* If caller provided a BDS*, provide the result of that lookup, too. */ >> + if (pbs) { >> + *pbs = bs; >> + } >> + >> + bitmap = bdrv_find_dirty_bitmap(bs, name); > > and this seems to say that bitmap names are per-BDS, not per-device. > We're still on the same page. >> + if (!bitmap) { >> + error_setg(errp, "Dirty bitmap '%s' not found", name); >> + return NULL; >> + } >> + >> + return bitmap; >> +} >> + >> /* New and old BlockDriverState structs for atomic group operations */ >> +++ b/qapi/block-core.json >> @@ -959,6 +959,61 @@ >> '*on-target-error': 'BlockdevOnError' } } >> >> ## >> +# @BlockDirtyBitmap >> +# >> +# @node: name of device/node which the bitmap is tracking >> +# >> +# @name: name of the dirty bitmap >> +# >> +# Since 2.3 >> +## >> +{ 'type': 'BlockDirtyBitmap', >> + 'data': { 'node': 'str', 'name': 'str' } } > > This naming implies that bitmap is a per-BDS option, but that as a > convenience, we allow a device name (BB) as shorthand for the top-level > BDS associated with the BB. I can live with that. > It sounds like I am not going to the mental facility after all. This is what I wanted. >> +++ b/qmp-commands.hx >> @@ -1244,6 +1244,57 @@ Example: >> EQMP >> >> { >> + .name = "block-dirty-bitmap-add", >> + .args_type = "node:B,name:s,granularity:i?", >> + .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_add, >> + }, >> + { >> + .name = "block-dirty-bitmap-remove", >> + .args_type = "node:B,name:s", >> + .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove, >> + }, > > I don't know if it is worth interleaving these declarations... > I can demingle them if I must; I just carried forth the convention in the original patches I received and nobody has mentioned it yet. >> + >> +SQMP >> + >> +block-dirty-bitmap-add >> +---------------------- >> +Since 2.3 >> + >> +Create a dirty bitmap with a name on the device, and start tracking the writes. >> + >> +Arguments: >> + >> +- "node": device/node on which to create dirty bitmap (json-string) >> +- "name": name of the new dirty bitmap (json-string) >> +- "granularity": granularity to track writes with (int, optional) >> + >> +Example: >> + >> +-> { "execute": "block-dirty-bitmap-add", "arguments": { "node": "drive0", >> + "name": "bitmap0" } } >> +<- { "return": {} } >> + >> +block-dirty-bitmap-remove >> +------------------------- >> +Since 2.3 > > ...to align with their examples. I don't see much else grouping in this > file. > OK, if a v14 is needed for other reasons I will decouple the examples in this file. >> + >> +Stop write tracking and remove the dirty bitmap that was created with >> +block-dirty-bitmap-add. >> + >> +Arguments: >> + >> +- "node": device/node on which to remove dirty bitmap (json-string) >> +- "name": name of the dirty bitmap to remove (json-string) >> + >> +Example: >> + >> +-> { "execute": "block-dirty-bitmap-remove", "arguments": { "node": "drive0", >> + "name": "bitmap0" } } >> +<- { "return": {} } >> + >> +EQMP >> + >> + { >> .name = "blockdev-snapshot-sync", >> .args_type = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?", >> .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync, >> > > The interface seems okay; I'm not sure if there are any tweaks we need > to the commit message or documentation. >