Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> On 01/20/2015 03:26 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> On 01/19/2015 05:08 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>
>>>>> On 01/16/2015 10:36 AM, Max Reitz wrote:
>>>>>> On 2015-01-12 at 11:30, John Snow wrote:
>>>>>>> From: Fam Zheng <famz@redhat.com>
>>>>>>>
>>>>>>> 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 <famz@redhat.com>
>>>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>>>> ---
>>>>>>>     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