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

[Peter Krempa]

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.