From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57781) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YDbz5-0002Sp-1r for qemu-devel@nongnu.org; Tue, 20 Jan 2015 11:48:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YDbz1-0003Eo-R6 for qemu-devel@nongnu.org; Tue, 20 Jan 2015 11:48:54 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60293) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YDbz1-0003Ec-Hg for qemu-devel@nongnu.org; Tue, 20 Jan 2015 11:48:51 -0500 Message-ID: <54BE86F0.2000408@redhat.com> Date: Tue, 20 Jan 2015 11:48:48 -0500 From: John Snow MIME-Version: 1.0 References: <1421080265-2228-1-git-send-email-jsnow@redhat.com> <1421080265-2228-4-git-send-email-jsnow@redhat.com> <54B93014.8010203@redhat.com> <54B940F7.7000904@redhat.com> <87oapvnkv1.fsf@blackfin.pond.sub.org> <54BD7191.2060404@redhat.com> <87sif57t8k.fsf@blackfin.pond.sub.org> In-Reply-To: <87sif57t8k.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, Max Reitz , vsementsov@parallels.com, stefanha@redhat.com On 01/20/2015 03:26 AM, Markus Armbruster wrote: > John Snow writes: > >> On 01/19/2015 05:08 AM, Markus Armbruster wrote: >>> John Snow writes: >>> >>>> On 01/16/2015 10:36 AM, Max Reitz wrote: >>>>> On 2015-01-12 at 11:30, John Snow wrote: >>>>>> From: Fam Zheng >>>>>> >>>>>> 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. >>>>>> >>>>>> 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(-) >>>>>> >>>>>> diff --git a/block.c b/block.c >>>>>> index bfeae6b..3eb77ee 100644 >>>>>> --- a/block.c >>>>>> +++ b/block.c >>>>>> @@ -5417,6 +5417,26 @@ int bdrv_get_dirty(BlockDriverState *bs, >>>>>> BdrvDirtyBitmap *bitmap, int64_t sector >>>>>> } >>>>>> } >>>>>> +/** >>>>>> + * Chooses a default granularity based on the existing cluster size, >>>>>> + * but clamped between [4K, 64K]. Defaults to 64K in the case that there >>>>>> + * is no cluster size information available. >>>>>> + */ >>>>>> +uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs) >>>>>> +{ >>>>>> + BlockDriverInfo bdi; >>>>>> + uint64_t granularity; >>>>>> + >>>>>> + if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) { >>>>>> + granularity = MAX(4096, bdi.cluster_size); >>>>>> + granularity = MIN(65536, granularity); >>>>>> + } else { >>>>>> + granularity = 65536; >>>>>> + } >>>>>> + >>>>>> + return granularity; >>>>>> +} >>>>>> + >>>>>> void bdrv_dirty_iter_init(BlockDriverState *bs, >>>>>> BdrvDirtyBitmap *bitmap, HBitmapIter *hbi) >>>>>> { >>>>>> diff --git a/block/mirror.c b/block/mirror.c >>>>>> index d819952..fc545f1 100644 >>>>>> --- a/block/mirror.c >>>>>> +++ b/block/mirror.c >>>>>> @@ -667,15 +667,7 @@ static void mirror_start_job(BlockDriverState >>>>>> *bs, BlockDriverState *target, >>>>>> MirrorBlockJob *s; >>>>>> if (granularity == 0) { >>>>>> - /* Choose the default granularity based on the target file's >>>>>> cluster >>>>>> - * size, clamped between 4k and 64k. */ >>>>>> - BlockDriverInfo bdi; >>>>>> - if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != 0) { >>>>>> - granularity = MAX(4096, bdi.cluster_size); >>>>>> - granularity = MIN(65536, granularity); >>>>>> - } else { >>>>>> - granularity = 65536; >>>>>> - } >>>>>> + granularity = bdrv_get_default_bitmap_granularity(target); >>>>>> } >>>>>> assert ((granularity & (granularity - 1)) == 0); >>>>>> diff --git a/blockdev.c b/blockdev.c >>>>>> index 5651a8e..95251c7 100644 >>>>>> --- a/blockdev.c >>>>>> +++ 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_ref, >>>>>> + const char *name, >>>>>> + BlockDriverState >>>>>> **pbs, >>>>>> + Error **errp) >>>>>> +{ >>>>>> + BlockDriverState *bs; >>>>>> + BdrvDirtyBitmap *bitmap; >>>>>> + >>>>>> + if (!node_ref) { >>>>>> + error_setg(errp, "Node reference cannot be NULL"); >>>>>> + return NULL; >>>>>> + } >>>>>> + if (!name) { >>>>>> + error_setg(errp, "Bitmap name cannot be NULL"); >>>>>> + return NULL; >>>>>> + } >>>>>> + >>>>>> + bs = bdrv_lookup_bs(node_ref, node_ref, errp); >>>>>> + if (!bs) { >>>>>> + error_setg(errp, "Node reference '%s' not found", node_ref); >>>>> >>>>> No need to throw the (hopefully) perfectly fine Error code returned by >>>>> bdrv_lookup_bs() away. >>>>> >>>> >>>> I just wanted an error message consistent with the parameter name, in >>>> this case. i.e., We couldn't find the "Node reference" instead of >>>> "device" or "node name." Just trying to distinguish the fact that this >>>> is an arbitrary reference in the error message. >>>> >>>> I can still remove it, but I am curious to see what Markus thinks of >>>> the names I have chosen before I monkey with the errors too much more. >>> >>> bdrv_lookup_bs() is an awkward interface. >>> >>> If @device is non-null, try to look up a backend (BB) named @device. If >>> it exists, return the backend's root node (BDS). >>> >>> Else if @node_name is non-null, try to look up a node (BDS) named >>> @node_name. If it exists, return it. >>> >>> Else, set this error: >>> >>> error_setg(errp, "Cannot find device=%s nor node_name=%s", >>> device ? device : "", >>> node_name ? node_name : ""); >>> >>> The error message is crap unless both device and node_name are non-null >>> and different. Which is never the case: we always either pass two >>> identical non-null arguments, or we pass a null and a non-null >>> argument[*]. In other words, the error message is always crap. >>> >>> In case you wonder why @device takes precedence over node_name when both >>> are given: makes no sense. But when both are given, they are always >>> identical, and since backend and node names share a name space, only one >>> can resolve. >>> >>> A couple of cleaner solutions come to mind: >>> >>> * Make bdrv_lookup_bs() suck less >>> >>> Assert its tacit preconditions: >>> >>> assert(device || node_name); >>> assert(!device || !node_name || device == node_name); >>> >>> Then make it produce a decent error: >>> >>> if (device && node_name) { >>> error_setg(errp, "Neither block backend nor node %s found", device); >>> else if (device) { >>> error_setg(errp, "Block backend %s not found", device); >>> } else if (node_name) { >>> error_setg(errp, "Block node %s not found", node_name); >>> } >>> >>> Note how the three cases mirror the three usage patterns. >>> >>> Further note that the proposed error messages deviate from the >>> existing practice of calling block backends "devices". Calling >>> everything and its dog a "device" is traditional, but it's also lazy >>> and confusing. End of digression. >>> >>> * Make bdrv_lookup_bs suck less by doing less: leave error_setg() to its >>> callers >>> >>> Drop the Error ** parameter. Callers know whether a failed lookup was >>> for a device name, a node name or both, and can set an appropriate >>> error themselves. >>> >>> I'd still assert the preconditions. >>> >>> * Replace the function by one for each of its usage patterns >>> >>> I think that's what I'd do. >>> >>> [...] >>> >>> >>> [*] See >>> https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg01298.html >>> >> >> I can submit a patch for making bdrv_lookup_bs "nicer," or at least >> its usage more clear, in a separate patch. > > Yes, please. > >> If you really want me to fold it into this series, I'd invite you to >> review explicitly my usage of the parameter "node-ref" before I embark >> on cleaning up other interface areas. > > Follow-up patch is fine. Adding one more bad error message along the > way before you fix them all doesn't bother me. > >> Does this naming scheme look sane to you, and fit with your general >> expectations? >> >> I can also add a "bdrv_lookup_noderef" function that takes only one >> argument, which will help enforce the "If both arguments are provided, >> they must be the same" paradigm. >> >> This patch (#3) covers my shot at a unified parameter, and you can see >> further consequences in #7, and #10 (transactions). > > Do we want to introduce a @node-ref naming convention? > > Currently, QMP calls parameters or members naming nodes @node-name or > similar, and parameters/members naming backends @device or similar. The > one place where we already accept either is called @reference in the > schema, but it's a member of an anonymous union, so it's not really > visible in QMP. > > Previously[*], we agreed (I think) to replace and deprecate the four > commands that use the "pair of names" convention to identify a node. > Their replacement would use the "single name" convention. The name can > either be a node name or a backend name, and the latter automatically > resolves to its root node. > > The "backend name resolves to its root node" convenience feature should > be available consistently or not at all. I think the consensus it to > want it consistently. > > Therefore, your new @node-ref is really the same as the existing > @node-name, isn't it? bdrv_lookup_bs() as used in the patch, as that function exists today, will resolve backends to root nodes, so these QMP commands will operate with device/backend names or node-names. > Why a new naming convention @node-ref? Is it meant to be in addition to > @node-name, or is it meant to replace it? Is it the same? It was my understanding that we didn't have a QMP command currently that accepted /only/ nodes; from your previous mail characterizing the existing patterns: "3. Node name only No known example." So this patch is /intending/ to add a command wherein you can identify either a "node-name" or a "device," where the real goal is to obtain any arbitrary node -- so I used a new name. If we want a new unified parameter in the future, we should probably figure out what it is and start using it. I propose "node-ref." I *did* see that "reference" was already used for this purpose, but as you say, the semantics are somewhat different there, so I opted for a new name to not confuse the usages. Maybe this is what we want, maybe it isn't: A case could be made for either case. I'm making my case for node-ref: Short for Node Reference, it's different from "Node Name" in that it does not describe a single node's name, it's simply a reference to one. To me, this means that it could either be a node-name OR a backend-name, because a backend name could be considered a reference to the root node of that tree. So it seems generic-y enough to be a unified parameter. >> CC'ing Eric Blake, as well, for comments on a "unified parameter" >> interface in general. > > Good move. > > > [*] https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02572.html > Adding Eric back in, where'd he go? --js