Re: [PATCH] block: nbd: Fix dirty bitmap context name

[Vladimir Sementsov-Ogievskiy]

19.12.2019 17:59, Nir Soffer wrote:
> On Thu, Dec 19, 2019 at 4:04 PM Kevin Wolf <address@hidden> wrote:
>>
>> Am 19.12.2019 um 14:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>> Ahh, I see, it's documented as
>>>
>>> +# @bitmap: Also export the dirty bitmap reachable from @device, so the
>>> +#          NBD client can use NBD_OPT_SET_META_CONTEXT with
>>> +#          "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0)
>>>
>>> and it is logical to assume that export name (which is @name argument) is
>>> mentioned. But we never mentioned it. This is just documented after
>>> removed experimenatl command x-nbd-server-add-bitmap,
>>>
>>> look at
>>>
>>> commit 7dc570b3806e5b0a4c9219061556ed5a4a0de80c
>>> Author: Eric Blake <address@hidden>
>>> Date:   Fri Jan 11 13:47:18 2019 -0600
>>>
>>>       nbd: Remove x-nbd-server-add-bitmap
>>>
>>> ...
>>>
>>> -# @bitmap-export-name: How the bitmap will be seen by nbd clients
>>> -#                      (default @bitmap)
>>> -#
>>> -# Note: the client must use NBD_OPT_SET_META_CONTEXT with a query of
>>> -# "qemu:dirty-bitmap:NAME" (where NAME matches @bitmap-export-name) to 
>>> access
>>> -# the exposed bitmap.
>>>
>>>
>>> So, this "NAME" is saved and now looks incorrect. What should be fixed, is 
>>> Qapi
>>> documentation.
>>
>> Hm, I don't know these interfaces very well, but from you explanation it
>> looks to me as if having a bitmap name made sense with
>> x-nbd-server-add-bitmap because it could be called more than once for
>> exporting multiple bitmaps.
>>
>> But now, we have only nbd-server-add, which takes a single bitmap name.
>> As we don't have to distinguish multiple bitmaps any more, wouldn't it
>> make more sense to use "qemu:dirty-bitmap" without any other
>> information? Both export name and bitmap name are already identified by
>> the connection.
> 
> We can use empty string (like the default export name), so the bitmap
> would be exposed as:
> 
>      "qemu:dirty-bitmap:"
> 
> This would solve the issue for users, and keep the API extensible.

As I already said, we can not. If we really wont such thing, use another name,
likq qemu:default-dirty-bitmap..

Or call bitmap export "default", to produce
  "qemu:dirty-bitmaps:default"

But don't change default behavior of nbd-server-add

> 
>> But if we have to have something there, using the bitmap name (which may
>> or may not be the same as used in the image file) makes a little more
>> sense because it makes the interface extensible for the case that we
>> ever want to re-introduce an nbd-server-add-bitmap.
> 
> But using the bitmap name means user of the NBD server need to know this name.

Why not? What is your case exactly? User knows, what kind of bitmap you are
exporting, so user is in some relation with exporting process anyway. Why
shouldn't it know the name?

This name may be defined in you exporting protocol.. What are you exporting?

Note also, that client may use NBD_OPT_LIST_META_CONTEXT with query
"qemu:dirty-bitmap:", to get list of all exported bitmaps.

> 
> One option is that libvirt would publish the name of the bitmap in the
> xml describing
> the backup, and oVirt will have to propagate this name to the actual
> program accessing
> the NBD server, which may be a user program in the case when we expose the NBD
> URL to users (planned for future version).
> 
> Another option is that the user will control this name, and libvirt
> will use the name specified
> by the user. This means oVirt will have to provide API to set this
> name and pass it to libvirt.
> 
> Both cases require lot of effort which does not help anyone in the
> task of getting dirty
> extents from an image - which is our current goal. We need to have
> good defaults that
> save unneeded effort in the entire stack.

So, you implementing some protocol, and need to export only one bitmap for
your specified scenario. Why not just give a constant name? Like ovirt-bitmap,
or something like this?

(of course, we need new optional paramter export-bitmap-name for nbd-server-add,
as I proposes, or reimplement nbd-server-add-bitmap)

> 
>> (By the way, even if the patch were correct,
> 
> I don't think this is about correctness, but having better default for users.
> 

Changing defaults always breaks existing users. For example Virtuozzo. We live
with these defaults, you can't just change it without any new option.. I hope.


-- 
Best regards,
Vladimir