|
| From: | John Snow |
| Subject: | Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove |
| Date: | Tue, 20 Jan 2015 11:48:48 -0500 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 |
On 01/20/2015 03:26 AM, Markus Armbruster wrote:
John Snow <address@hidden> writes:On 01/19/2015 05:08 AM, Markus Armbruster wrote:John Snow <address@hidden> writes:On 01/16/2015 10:36 AM, Max Reitz wrote:On 2015-01-12 at 11:30, John Snow wrote:From: Fam Zheng <address@hidden> 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 <address@hidden> Signed-off-by: John Snow <address@hidden> --- 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.htmlI 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
| [Prev in Thread] | Current Thread | [Next in Thread] |