Re: [Qemu-devel] [PATCH v9 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[John Snow <jsnow@redhat.com>]

On 12/15/2014 03:50 AM, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@gmail.com> writes:
>
>> On Wed, Dec 10, 2014 at 01:43:47PM +0100, Markus Armbruster wrote:
>>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>>>
>>>> On Mon, Dec 01, 2014 at 03:30:08PM -0500, John Snow wrote:
>>>>> diff --git a/block.c b/block.c
>>>>> index e5c6ccf..3f27519 100644
>>>>> --- a/block.c
>>>>> +++ b/block.c
>>>>> @@ -5420,6 +5420,25 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
>>>>>       }
>>>>>   }
>>>>>
>>>>> +#define BDB_MIN_DEF_GRANULARITY 4096
>>>>> +#define BDB_MAX_DEF_GRANULARITY 65536
>>>>> +#define BDB_DEFAULT_GRANULARITY BDB_MAX_DEF_GRANULARITY
>>>>> +
>>>>> +uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
>>>>
>>>> Long names are unwieldy but introducing multiple abbreviations is not a
>>>> good solution, it makes the code more confusing (BDB vs dbm).
>>>>
>>>> I would call the function bdrv_get_default_bitmap_granularity().
>>>>
>>>> The constants weren't necessary since the point of this function is to
>>>> capture the default value.  No one else should use the constants -
>>>> otherwise there is a high probability that they are reimplementing this
>>>> logic.  I would just leave them as literals in the code.
>>>>
>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>> index 5651a8e..4d30b09 100644
>>>>> --- a/blockdev.c
>>>>> +++ b/blockdev.c
>>>>> @@ -1894,6 +1894,60 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
>>>>>       aio_context_release(aio_context);
>>>>>   }
>>>>>
>>>>> +void qmp_block_dirty_bitmap_add(const char *device, const char *name,
>>>>> +                                bool has_granularity, int64_t granularity,
>>>>> +                                Error **errp)
>>>>> +{
>>>>> +    BlockDriverState *bs;
>>>>> +
>>>>> +    bs = bdrv_lookup_bs(device, NULL, errp);
>>>>
>>>> Markus: I think we need to support node-name here so dirty bitmaps can
>>>> be applied at any node in the graph?
>>>
>>> We need to consider node names for all new QMP commands.  Whenever we
>>> add a backend name parameter, like we do for the two new commands in
>>> this patch, we need to decide whether nodes make sense, too.  Do they?
>>>
>>> I'm afraid we haven't quite made up our mind what to do when nodes make
>>> sense.
>>>
>>> Existing patterns of backend / node name parameters:
>>>
>>> 1. Backend name only
>>>
>>>     Parameter is commonly called @device.  Examples: eject,
>>>     block_set_io_throttle.
>>>
>>>     Code uses blk_by_name() or bdrv_find().  The latter needs to be
>>>     converted to the former.
>>>
>>> 2. Backend or node name
>>>
>>> 2a. Two optional parameters, commonly called @device and @node-name,
>>>     of which exactly one must be given.  Example: block_passwd.
>>>
>>>     Code uses
>>>
>>>          bs = bdrv_lookup_bs(has_device ? device : NULL,
>>>                              has_node_name ? node_name : NULL,
>>>                                  &local_err);
>>>
>>>     which is a roundabout way to say
>>>
>>>          bs = bdrv_lookup_bs(device, node_name, &local_err);
>>>
>>> 2b. Single parameter.  The single example is anonymous union
>>>     BlockdevRef.
>>>
>>>     Code uses
>>>
>>>          bs = bdrv_lookup_bs(reference, reference, errp);
>>>
>>>     If we want to adopt "single parameter" going forward, we need to
>>>     decide on a naming convention.  Existing commands are probably stuck
>>>     with @device for compatibility.  Do we care for names enough to
>>>     improve on that?
>>>
>>>     A convenience wrapper around bdrv_lookup_bs() to avoid stuttering
>>>     name argument could make sense.
>>
>> Initially only the backend needs dirty bitmap support (this satisfies
>> the incremental backup use case).
>>
>> It is possible that future use cases will require node-name.
>>
>> I'm happy with just allowing the device parameter today and adding the
>> node-name parameter if needed later.
>>
>> This conservative approach seems good because IMO we shouldn't add
>> unused features to the API since they need to be tested and maintained
>> forever.
>>
>> So maybe use a device argument with blk_by_name() for now.
>>
>> In the future switch to bdrv_lookup_bs() with has_device/has_node_name.
>>
>> If anyone strongly feels we should support node-name from day 1, I'm
>> okay with that too but there needs to be a test case which actually
>> exercises that code!
>
> Agree with not adding unused features.
>
> However, we should make up our minds how we want QMP to do backend and
> node names in the future.  I see two basic options:
>
> 1. Inertia
>
>     Keep adding separate arguments for backend name (commonly called
>     @device) and node name (commonly called @node-name).  If the command
>     can take only one, make it mandatory.  If it can take either, make it
>     either/or.
>
>     Cements the inconsistency between single parameter in BlockdevRef and
>     the two either/or parameters elsewhere.
>
> 2. Clean up the mess
>
>     New commands take a single parameter.  The command accepts backends
>     or nodes as they make sense for the command.  Acceptable parameter
>     name needed.
>
>     Either existing commands get changed to single parameter (with the
>     necessary compatibility and discoverability gunk), or they get
>     replaced by new commands.
>
>     I'll analyze how the gunk could be done in a separate message,
>     hopefully soon.
>

OK, given the most recent email, it seems as if you would prefer to use 
"@device" for backends and "@node-name" for arbitrary node selection. 
This command only needs to support the backend for now, I think.

Assuming we want to allow arbitrary nodes in the future, should I leave 
the parameters as @device but rename the monitor commands to allow for 
an arbitrary node version sometime later? I don't want to introduce a 
new command that creates a new mess for us to clean up as we unify these 
parameter semantics.

I said I'd use your mail as a guide but I hadn't skimmed it yet to see 
how specific the prescriptions were ;)