[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and b
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove |
Date: |
Wed, 21 Jan 2015 10:34:20 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
John Snow <address@hidden> writes:
> 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.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.
I'm afraid I forgot much of the discussion we had before the break, and
only now it's coming back, slowly.
Quoting myself on naming parameters identifying nodes[*]:
John Snow pointed out to me that we still haven't spelled out how this
single parameter should be named.
On obvious option is calling it node-name, or FOO-node-name when we have
several. However, we'd then have two subtly different kinds of
parameters called like that: the old ones accept *only* node names, the
new ones also accept backend names, which automatically resolve to the
backend's root node.
Three ways to cope with that:
* Find a better name.
* Make the old ones accept backend names, too. Only a few, not that
much work. However, there are exceptions:
- blockdev-add's node-name *defines* the node name.
- query-named-block-nodes's node-name *is* the node's name.
* Stop worrying and embrace the inconsistency. The affected commands
are headed for deprecation anyway.
I think I'd go with "node" or "FOO-node" for parameters that reference
nodes and accept both node names and backend names, and refrain from
touching the existing node-name parameters.
Let's go through existing uses of @node-name again:
1. Define a node name
QMP commands blockdev-add (type BlockdevOptionsBase), drive-mirror
2. Report a node name
QMP command query-named-block-nodes (type BlockDeviceInfo)
3. Node reference with backend names permitted for convenience
New QMP command block-dirty-bitmap-add (type BlockDirtyBitmapAdd) and
others
4. Node reference with backend names not permitted
QMP commands drive-mirror @replaces, change-backing-file
@image-node-name
We may want to support the "backend name resolves to root node"
convenience feature here, for consistency. Then this moves under 3.
Note interface wart: change-backing-file additionally requires the
backend owning the node. We need the backend to set op-blockers, we
can't easily find it from the node, so we make the user point it out
to us.
5. "Pair of names" node reference, specify exactly one
QMP commands block_passwd, block_resize, blockdev-snapshot-sync
We can ignore this one, because we intend to replace the commands and
deprecate the old ones.
If I understand you correctly, you're proposing to use @node-name or
@FOO-node-name when the value must be a node name (items 1+2 and 4), and
@node-ref or @FOO-node-ref where we additionally support the "backend
name resolves to root node" convenience feature (item 3).
Is that a fair description of your proposal?
PRO: the name makes it clear when the convenience feature is supported.
CON: if we eliminate 4 by supporting the convenience feature, we either
create ugly exceptions to the naming convention, or rename the
parameters.
Opinions?
>>> 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?
Looks like you announced the cc:, but didn't actually do it, twice %-)
Let me have a try.
[1] https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg03151.html
- Re: [Qemu-devel] [PATCH v11 04/13] block: Introduce bdrv_dirty_bitmap_granularity(), (continued)
- [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove, John Snow, 2015/01/12
- Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove, Max Reitz, 2015/01/16
- Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove, Markus Armbruster, 2015/01/19
- Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove, John Snow, 2015/01/19
- Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove, Markus Armbruster, 2015/01/20
- Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove, John Snow, 2015/01/20
- Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove,
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove, Eric Blake, 2015/01/21
- Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove, Kevin Wolf, 2015/01/30
- Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove, John Snow, 2015/01/30
- Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove, Kevin Wolf, 2015/01/30
Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove, Vladimir Sementsov-Ogievskiy, 2015/01/29
[Qemu-devel] [PATCH v11 01/13] block: fix spoiling all dirty bitmaps by mirror and migration, John Snow, 2015/01/12
[Qemu-devel] [PATCH v11 05/13] block: Add bdrv_clear_dirty_bitmap, John Snow, 2015/01/12
[Qemu-devel] [PATCH v11 09/13] qmp: Add support of "dirty-bitmap" sync mode for drive-backup, John Snow, 2015/01/12