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

[Vladimir Sementsov-Ogievskiy]

20.12.2019 13:40, Peter Krempa wrote:
> On Fri, Dec 20, 2019 at 10:06:17 +0000, Vladimir Sementsov-Ogievskiy wrote:
>> 20.12.2019 12:56, Peter Krempa wrote:
>>> On Fri, Dec 20, 2019 at 09:39:17 +0000, Vladimir Sementsov-Ogievskiy wrote:
>>>> 19.12.2019 18:55, Nir Soffer wrote:
>>>>> On Thu, Dec 19, 2019 at 5:17 PM Vladimir Sementsov-Ogievskiy
>>>>> <address@hidden> wrote:
>>>>>>
>>>>>> 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?
>>>>>
>>>>> Because the user configuring qemu (libvirt) is not the same user
>>>>> accessing qemu NBD
>>>>> server (ovirt, or some backup application).
>>>>>
>>>>>> This name may be defined in you exporting protocol.. What are you 
>>>>>> exporting?
>>>>>
>>>>> We export HTTP API, allowing getting dirty extents and reading data:
>>>>> https://www.ovirt.org/develop/release-management/features/storage/incremental-backup.html#map-request
>>>>> (this document is outdated, but it gives the general idea)
>>>>>
>>>>> Or provide the NBD URL directly to user (future).
>>>>>
>>>>>> Note also, that client may use NBD_OPT_LIST_META_CONTEXT with query
>>>>>> "qemu:dirty-bitmap:", to get list of all exported bitmaps.
>>>>>
>>>>> This is another option, I did not try to use this yet. We can use the 
>>>>> single
>>>>> exported bitmap and fail if we get more than one. This is probably better
>>>>> than changing the entire stack to support bitmap name.
>>>>>
>>>>>>> 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?
>>>>>
>>>>> But we don't use qemu directly. We use libvirt, and libvirt does not 
>>>>> expose
>>>>> the name of the bitmap, or let use control this name.
>>>>>
>>>>> This is a simplified flow:
>>>>> 1. libvirt starts a backup, creating the "backup-exportname" bitmap
>>>>
>>>> But do you manage exportname, or not?
>>>
>>> They can't manage the export name either, but apparently the default
>>> that libvirt uses suits them.
>>>
>>> I can add possibility to name the actual backup output bitmap
>>> arbitrarily. Obviously the user then has to pass a sensible name of a
>>> non existant bitmap to proceed.
>>>
>>> Making both configurable at the same time may be worth doing as it will
>>> be basically the same code.
>>>
>>
>> Intuitively, I think it's better leave bitmap name as is: libvirt manages
>> bitmaps, so let it manage their names as it wants. And I don't think that
>> Nir want to care about existing bitmaps.
> 
> Actually for an incremental backup we are creating a new bitmap which is
> used for merging all the appropriate bitmaps for the expected backup
> range. This is done also when only one bitmap would be selected this way
> as there's no point in optimizing this algorithm.
> 
> This means we can name the new bitmap arbitrarily (if it doesn't clash
> with existing bitmap) and since that name will be also exported it's the
> same they request.
> 

Hm, clear. Then it should work too.

-- 
Best regards,
Vladimir