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

[John Snow <jsnow@redhat.com>]

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.

>> 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