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

Nir Soffer posted 1 patch 4 years, 3 months ago
Test asan failed
Test checkpatch failed
Test FreeBSD failed
Test docker-mingw@fedora failed
Test docker-clang@ubuntu failed
Test docker-quick@centos7 failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20191219125151.21482-1-nsoffer@redhat.com
Maintainers: Max Reitz <mreitz@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Eric Blake <eblake@redhat.com>
nbd/server.c               |  2 +-
tests/qemu-iotests/223     | 16 ++++++++--------
tests/qemu-iotests/223.out |  8 ++++----
3 files changed, 13 insertions(+), 13 deletions(-)
[PATCH] block: nbd: Fix dirty bitmap context name
Posted by Nir Soffer 4 years, 3 months ago
When adding an export with a dirty bitmap, expose the bitmap at:

    qemu:dirty-bitmap:export-name

This matches qapi documentation, and user expectations.

Without this, qemu leaks libvirt implementations details to clients by
exposing the bitmap using the actual bitmap name:

    qemu:dirty-bitmap:bitmap-name

And all clients need to duplicate code like:

    meta_context = "qemu:dirty-bitmap:backup-" + export_name

NBD allows exposing multiple bitmaps under "qemu:dirty-bitmap:"
namespace, and clients can query the available bitmaps, but it is not
clear what a client should do if a server provides multiple bitmaps.
---
 nbd/server.c               |  2 +-
 tests/qemu-iotests/223     | 16 ++++++++--------
 tests/qemu-iotests/223.out |  8 ++++----
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 24ebc1a805..f20f2994c0 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1574,7 +1574,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
         exp->export_bitmap = bm;
         assert(strlen(bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
         exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
-                                                     bitmap);
+                                                     name);
         assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
     }
 
diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index ea69cd4b8b..3068a7c280 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -167,7 +167,7 @@ $QEMU_IO -r -c 'r -P 0x22 512 512' -c 'r -P 0 512k 512k' -c 'r -P 0x11 1m 1m' \
 $QEMU_IMG map --output=json --image-opts \
   "$IMG" | _filter_qemu_img_map
 $QEMU_IMG map --output=json --image-opts \
-  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b" | _filter_qemu_img_map
+  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n" | _filter_qemu_img_map
 
 echo
 echo "=== Contrast to small granularity dirty-bitmap ==="
@@ -175,7 +175,7 @@ echo
 
 IMG="driver=nbd,export=n2,server.type=unix,server.path=$SOCK_DIR/nbd"
 $QEMU_IMG map --output=json --image-opts \
-  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
+  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n2" | _filter_qemu_img_map
 
 echo
 echo "=== End qemu NBD server ==="
@@ -199,15 +199,15 @@ echo
 echo "=== Use qemu-nbd as server ==="
 echo
 
-nbd_server_start_unix_socket -r -f $IMGFMT -B b "$TEST_IMG"
-IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
+nbd_server_start_unix_socket -r -f $IMGFMT -x n -B b "$TEST_IMG"
+IMG="driver=nbd,export=n,server.type=unix,server.path=$nbd_unix_socket"
 $QEMU_IMG map --output=json --image-opts \
-  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b" | _filter_qemu_img_map
+  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n" | _filter_qemu_img_map
 
-nbd_server_start_unix_socket -f $IMGFMT -B b2 "$TEST_IMG"
-IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
+nbd_server_start_unix_socket -f $IMGFMT -x n -B b2 "$TEST_IMG"
+IMG="driver=nbd,export=n,server.type=unix,server.path=$nbd_unix_socket"
 $QEMU_IMG map --output=json --image-opts \
-  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
+  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n" | _filter_qemu_img_map
 
 # success, all done
 echo '*** done'
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index f175598802..9f879add60 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -61,7 +61,7 @@ exports available: 2
   max block: 33554432
   available meta contexts: 2
    base:allocation
-   qemu:dirty-bitmap:b
+   qemu:dirty-bitmap:n
  export: 'n2'
   size:  4194304
   flags: 0xced ( flush fua trim zeroes df cache fast-zero )
@@ -70,7 +70,7 @@ exports available: 2
   max block: 33554432
   available meta contexts: 2
    base:allocation
-   qemu:dirty-bitmap:b2
+   qemu:dirty-bitmap:n2
 
 === Contrast normal status to large granularity dirty-bitmap ===
 
@@ -141,7 +141,7 @@ exports available: 2
   max block: 33554432
   available meta contexts: 2
    base:allocation
-   qemu:dirty-bitmap:b
+   qemu:dirty-bitmap:n
  export: 'n2'
   size:  4194304
   flags: 0xced ( flush fua trim zeroes df cache fast-zero )
@@ -150,7 +150,7 @@ exports available: 2
   max block: 33554432
   available meta contexts: 2
    base:allocation
-   qemu:dirty-bitmap:b2
+   qemu:dirty-bitmap:n2
 
 === Contrast normal status to large granularity dirty-bitmap ===
 
-- 
2.21.0


Re: [PATCH] block: nbd: Fix dirty bitmap context name
Posted by Vladimir Sementsov-Ogievskiy 4 years, 3 months ago
I'd not call it a "fix".. As it implies something broken.

[edit: OK, now I see that something is broken, and why you called it "fix",
  see below]

19.12.2019 15:51, Nir Soffer wrote:
> When adding an export with a dirty bitmap, expose the bitmap at:
> 
>      qemu:dirty-bitmap:export-name

export-name? But it would be extra information, as client already knows
with which export it works.

NBD commands NBD_OPT_GET/SET_META_CONTEXT includes export name as a
parameter, so, any queried metadata (bitmaps, etc) is always bound to
specified export.

> 
> This matches qapi documentation, and user expectations.

Hmmm,
"qemu" namespace is documented in docs/interop/nbd.txt, not in Qapi,
which is also mention in official NBD spec.


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 <eblake@redhat.com>
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.


> 
> Without this, qemu leaks libvirt implementations details to clients by
> exposing the bitmap using the actual bitmap name:
> 
>      qemu:dirty-bitmap:bitmap-name

Yes, "qemu" namespace specification says:
qemu:dirty-bitmap:<dirty-bitmap-export-name>

so, <dirty-bitmap-export-name> may be exact bitmap name or may be something other.

We just don't have an interface to set such name. It was in removed
x-nbd-server-add-bitmap

So, if you need this possibility now, the correct way is to add 'export-bitmap-name'
optional parameter to nbd-server-add, like it was in x-nbd-server-add-bitmap

> 
> And all clients need to duplicate code like:
> 
>      meta_context = "qemu:dirty-bitmap:backup-" + export_name
> 
> NBD allows exposing multiple bitmaps under "qemu:dirty-bitmap:"
> namespace, and clients can query the available bitmaps, but it is not
> clear what a client should do if a server provides multiple bitmaps.
> ---
>   nbd/server.c               |  2 +-
>   tests/qemu-iotests/223     | 16 ++++++++--------
>   tests/qemu-iotests/223.out |  8 ++++----
>   3 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 24ebc1a805..f20f2994c0 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1574,7 +1574,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>           exp->export_bitmap = bm;
>           assert(strlen(bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
>           exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
> -                                                     bitmap);
> +                                                     name);

I think it's a bad idea to automatically name bitmap after export. Actually export may
have several bitmaps (we just don't support it). "NAME" in Qapi spec is a mistake.

>           assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
>       }
>   
> diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
> index ea69cd4b8b..3068a7c280 100755
> --- a/tests/qemu-iotests/223
> +++ b/tests/qemu-iotests/223
> @@ -167,7 +167,7 @@ $QEMU_IO -r -c 'r -P 0x22 512 512' -c 'r -P 0 512k 512k' -c 'r -P 0x11 1m 1m' \
>   $QEMU_IMG map --output=json --image-opts \
>     "$IMG" | _filter_qemu_img_map
>   $QEMU_IMG map --output=json --image-opts \
> -  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b" | _filter_qemu_img_map
> +  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n" | _filter_qemu_img_map
>   
>   echo
>   echo "=== Contrast to small granularity dirty-bitmap ==="
> @@ -175,7 +175,7 @@ echo
>   
>   IMG="driver=nbd,export=n2,server.type=unix,server.path=$SOCK_DIR/nbd"
>   $QEMU_IMG map --output=json --image-opts \
> -  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
> +  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n2" | _filter_qemu_img_map
>   
>   echo
>   echo "=== End qemu NBD server ==="
> @@ -199,15 +199,15 @@ echo
>   echo "=== Use qemu-nbd as server ==="
>   echo
>   
> -nbd_server_start_unix_socket -r -f $IMGFMT -B b "$TEST_IMG"
> -IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
> +nbd_server_start_unix_socket -r -f $IMGFMT -x n -B b "$TEST_IMG"
> +IMG="driver=nbd,export=n,server.type=unix,server.path=$nbd_unix_socket"
>   $QEMU_IMG map --output=json --image-opts \
> -  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b" | _filter_qemu_img_map
> +  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n" | _filter_qemu_img_map
>   
> -nbd_server_start_unix_socket -f $IMGFMT -B b2 "$TEST_IMG"
> -IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
> +nbd_server_start_unix_socket -f $IMGFMT -x n -B b2 "$TEST_IMG"
> +IMG="driver=nbd,export=n,server.type=unix,server.path=$nbd_unix_socket"
>   $QEMU_IMG map --output=json --image-opts \
> -  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
> +  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n" | _filter_qemu_img_map
>   
>   # success, all done
>   echo '*** done'
> diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
> index f175598802..9f879add60 100644
> --- a/tests/qemu-iotests/223.out
> +++ b/tests/qemu-iotests/223.out
> @@ -61,7 +61,7 @@ exports available: 2
>     max block: 33554432
>     available meta contexts: 2
>      base:allocation
> -   qemu:dirty-bitmap:b
> +   qemu:dirty-bitmap:n
>    export: 'n2'
>     size:  4194304
>     flags: 0xced ( flush fua trim zeroes df cache fast-zero )
> @@ -70,7 +70,7 @@ exports available: 2
>     max block: 33554432
>     available meta contexts: 2
>      base:allocation
> -   qemu:dirty-bitmap:b2
> +   qemu:dirty-bitmap:n2
>   
>   === Contrast normal status to large granularity dirty-bitmap ===
>   
> @@ -141,7 +141,7 @@ exports available: 2
>     max block: 33554432
>     available meta contexts: 2
>      base:allocation
> -   qemu:dirty-bitmap:b
> +   qemu:dirty-bitmap:n
>    export: 'n2'
>     size:  4194304
>     flags: 0xced ( flush fua trim zeroes df cache fast-zero )
> @@ -150,7 +150,7 @@ exports available: 2
>     max block: 33554432
>     available meta contexts: 2
>      base:allocation
> -   qemu:dirty-bitmap:b2
> +   qemu:dirty-bitmap:n2
>   
>   === Contrast normal status to large granularity dirty-bitmap ===
>   
> 


-- 
Best regards,
Vladimir
Re: [PATCH] block: nbd: Fix dirty bitmap context name
Posted by Kevin Wolf 4 years, 3 months ago
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 <eblake@redhat.com>
> 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.

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.

(By the way, even if the patch were correct, it lacks a Signed-off-by.)

Kevin


Re: [PATCH] block: nbd: Fix dirty bitmap context name
Posted by Nir Soffer 4 years, 3 months ago
On Thu, Dec 19, 2019 at 4:04 PM Kevin Wolf <kwolf@redhat.com> 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 <eblake@redhat.com>
> > 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.

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

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.

> (By the way, even if the patch were correct,

I don't think this is about correctness, but having better default for users.

Nir


Re: [PATCH] block: nbd: Fix dirty bitmap context name
Posted by Vladimir Sementsov-Ogievskiy 4 years, 3 months ago
19.12.2019 17:59, Nir Soffer wrote:
> On Thu, Dec 19, 2019 at 4:04 PM Kevin Wolf <kwolf@redhat.com> 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 <eblake@redhat.com>
>>> 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
Re: [PATCH] block: nbd: Fix dirty bitmap context name
Posted by Nir Soffer 4 years, 3 months ago
On Thu, Dec 19, 2019 at 5:17 PM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
>
> 19.12.2019 17:59, Nir Soffer wrote:
> > On Thu, Dec 19, 2019 at 4:04 PM Kevin Wolf <kwolf@redhat.com> 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 <eblake@redhat.com>
> >>> 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
2. oVirt host agent reports the nbd url for the disk
3. oVirt manager configures nbd/http proxy with nbd url
4. User (e.g. backup application) access the http/nbd proxy to get the
dirty extents

Alternatively, oVirt mangaer would provide the NBD url directly to the user,
and it will access it directly with some NBD client.

To make this work we need to pass more information between the
different programs
and change APIs to publish or accept bitmap names. This is a lot of
work that does
not help the current use case, accessing a single dirty bitmap.

We can solve it in libvirt if we don't have another choice.

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

That's a good point, we should not break existing usage.

Nir


Re: [PATCH] block: nbd: Fix dirty bitmap context name
Posted by Nir Soffer 4 years, 3 months ago
On Thu, Dec 19, 2019 at 5:55 PM Nir Soffer <nsoffer@redhat.com> wrote:
>
> On Thu, Dec 19, 2019 at 5:17 PM Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com> wrote:
...
> > 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.

I went with this option for now, getting the single bitmap published by qemu.
https://gerrit.ovirt.org/c/105861/2/common/ovirt_imageio_common/nbd.py

This way I can work with current libvirt, and I don't care about the name of
the bitmap.

Practically this will break if libvirt will configure another dirty
bitmap, but it is fine.
Using API that promise list with unknown bitmap name is the same as API
returning only one bitmap. Libvirt cannot change it now since it will break
existing users :-)


Re: [PATCH] block: nbd: Fix dirty bitmap context name
Posted by Vladimir Sementsov-Ogievskiy 4 years, 3 months ago
19.12.2019 18:55, Nir Soffer wrote:
> On Thu, Dec 19, 2019 at 5:17 PM Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com> wrote:
>>
>> 19.12.2019 17:59, Nir Soffer wrote:
>>> On Thu, Dec 19, 2019 at 4:04 PM Kevin Wolf <kwolf@redhat.com> 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 <eblake@redhat.com>
>>>>> 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?

> 2. oVirt host agent reports the nbd url for the disk
> 3. oVirt manager configures nbd/http proxy with nbd url
> 4. User (e.g. backup application) access the http/nbd proxy to get the
> dirty extents
> 
> Alternatively, oVirt mangaer would provide the NBD url directly to the user,
> and it will access it directly with some NBD client.
> 
> To make this work we need to pass more information between the
> different programs
> and change APIs to publish or accept bitmap names. This is a lot of
> work that does
> not help the current use case, accessing a single dirty bitmap.
> 
> We can solve it in libvirt if we don't have another choice.

Aha. So, the actual problem is that you use libvirt feature, which on the output
gives you bitmap, with some name chosen by libvirt, and it is not comfortable.

Then may be correct way is to make in libvirt an option to chose export-bitmap-name,
backed in Qemu by corresponding option in nbd-server-add (or nbd-server-add-bitmap)?

Then you just chose some name, like __org_ovirt_bitmap, which would be additional
check for nbd client, that this is the correct bitmap to use, and everything is good?

> 
>> (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.
> 
> That's a good point, we should not break existing usage.
> 
> Nir
> 


-- 
Best regards,
Vladimir
Re: [PATCH] block: nbd: Fix dirty bitmap context name
Posted by Peter Krempa 4 years, 3 months ago
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
> > <vsementsov@virtuozzo.com> wrote:
> >>
> >> 19.12.2019 17:59, Nir Soffer wrote:
> >>> On Thu, Dec 19, 2019 at 4:04 PM Kevin Wolf <kwolf@redhat.com> 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 <eblake@redhat.com>
> >>>>> 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.


Re: [PATCH] block: nbd: Fix dirty bitmap context name
Posted by Vladimir Sementsov-Ogievskiy 4 years, 3 months ago
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
>>> <vsementsov@virtuozzo.com> wrote:
>>>>
>>>> 19.12.2019 17:59, Nir Soffer wrote:
>>>>> On Thu, Dec 19, 2019 at 4:04 PM Kevin Wolf <kwolf@redhat.com> 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 <eblake@redhat.com>
>>>>>>> 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.

But bitmap-export-name is the other thing. It's a bitmap name, as it
represented to NBD client. And it may be something other. By default
Qemu uses name of the bitmap as this name, but we can add an option
to alter this behavior (actually we had such option in experimental
x-nbd-server-add-bitmap, which was dropped).

Then, for this "nbd bitmap name" or "export bitmap name", Nir will be
able to use some constant name, which will never conflict with other
bitmaps, as it is the other thing (part of NBD metadata context name)


-- 
Best regards,
Vladimir
Re: [PATCH] block: nbd: Fix dirty bitmap context name
Posted by Peter Krempa 4 years, 3 months ago
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
> >>> <vsementsov@virtuozzo.com> wrote:
> >>>>
> >>>> 19.12.2019 17:59, Nir Soffer wrote:
> >>>>> On Thu, Dec 19, 2019 at 4:04 PM Kevin Wolf <kwolf@redhat.com> 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 <eblake@redhat.com>
> >>>>>>> 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.


Re: [PATCH] block: nbd: Fix dirty bitmap context name
Posted by Vladimir Sementsov-Ogievskiy 4 years, 3 months ago
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
>>>>> <vsementsov@virtuozzo.com> wrote:
>>>>>>
>>>>>> 19.12.2019 17:59, Nir Soffer wrote:
>>>>>>> On Thu, Dec 19, 2019 at 4:04 PM Kevin Wolf <kwolf@redhat.com> 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 <eblake@redhat.com>
>>>>>>>>> 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
Re: [PATCH] block: nbd: Fix dirty bitmap context name
Posted by Vladimir Sementsov-Ogievskiy 4 years, 3 months ago
19.12.2019 17:04, Kevin Wolf 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 <eblake@redhat.com>
>> 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.

I think, it will a bit in conflict with already documented

* "qemu:dirty-bitmap:" - returns list of all available dirty-bitmap
                          metadata contexts.

So, if we want some "default dirty bitmap", we'd better use something different
from just dirty-bitmap. For example,

    "qemu:default-dirty-bitmap"

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

Agree


> 
> (By the way, even if the patch were correct, it lacks a Signed-off-by.)
> 
> Kevin
> 


-- 
Best regards,
Vladimir
Re: [PATCH] block: nbd: Fix dirty bitmap context name
Posted by Nir Soffer 4 years, 3 months ago
On Thu, Dec 19, 2019 at 3:42 PM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
>
> I'd not call it a "fix".. As it implies something broken.
>
> [edit: OK, now I see that something is broken, and why you called it "fix",
>   see below]
>
> 19.12.2019 15:51, Nir Soffer wrote:
> > When adding an export with a dirty bitmap, expose the bitmap at:
> >
> >      qemu:dirty-bitmap:export-name
>
> export-name? But it would be extra information, as client already knows
> with which export it works.

Right, using empty string would be good as well.

> NBD commands NBD_OPT_GET/SET_META_CONTEXT includes export name as a
> parameter, so, any queried metadata (bitmaps, etc) is always bound to
> specified export.
>
> >
> > This matches qapi documentation, and user expectations.
>
> Hmmm,
> "qemu" namespace is documented in docs/interop/nbd.txt, not in Qapi,
> which is also mention in official NBD spec.
>
>
> 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 <eblake@redhat.com>
> 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.
>
>
> >
> > Without this, qemu leaks libvirt implementations details to clients by
> > exposing the bitmap using the actual bitmap name:
> >
> >      qemu:dirty-bitmap:bitmap-name
>
> Yes, "qemu" namespace specification says:
> qemu:dirty-bitmap:<dirty-bitmap-export-name>
>
> so, <dirty-bitmap-export-name> may be exact bitmap name or may be something other.
>
> We just don't have an interface to set such name. It was in removed
> x-nbd-server-add-bitmap
>
> So, if you need this possibility now, the correct way is to add 'export-bitmap-name'
> optional parameter to nbd-server-add, like it was in x-nbd-server-add-bitmap

I don't think we need such API. How would it help users trying to get
dirty extents
from an image?

> > And all clients need to duplicate code like:
> >
> >      meta_context = "qemu:dirty-bitmap:backup-" + export_name
> >
> > NBD allows exposing multiple bitmaps under "qemu:dirty-bitmap:"
> > namespace, and clients can query the available bitmaps, but it is not
> > clear what a client should do if a server provides multiple bitmaps.
> > ---
> >   nbd/server.c               |  2 +-
> >   tests/qemu-iotests/223     | 16 ++++++++--------
> >   tests/qemu-iotests/223.out |  8 ++++----
> >   3 files changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/nbd/server.c b/nbd/server.c
> > index 24ebc1a805..f20f2994c0 100644
> > --- a/nbd/server.c
> > +++ b/nbd/server.c
> > @@ -1574,7 +1574,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
> >           exp->export_bitmap = bm;
> >           assert(strlen(bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
> >           exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
> > -                                                     bitmap);
> > +                                                     name);
>
> I think it's a bad idea to automatically name bitmap after export. Actually export may
> have several bitmaps (we just don't support it).

What are the semantics of multiple dirty bitmaps for same export? How
users are going
to use this?

>  "NAME" in Qapi spec is a mistake.
>
> >           assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
> >       }
> >
> > diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
> > index ea69cd4b8b..3068a7c280 100755
> > --- a/tests/qemu-iotests/223
> > +++ b/tests/qemu-iotests/223
> > @@ -167,7 +167,7 @@ $QEMU_IO -r -c 'r -P 0x22 512 512' -c 'r -P 0 512k 512k' -c 'r -P 0x11 1m 1m' \
> >   $QEMU_IMG map --output=json --image-opts \
> >     "$IMG" | _filter_qemu_img_map
> >   $QEMU_IMG map --output=json --image-opts \
> > -  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b" | _filter_qemu_img_map
> > +  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n" | _filter_qemu_img_map
> >
> >   echo
> >   echo "=== Contrast to small granularity dirty-bitmap ==="
> > @@ -175,7 +175,7 @@ echo
> >
> >   IMG="driver=nbd,export=n2,server.type=unix,server.path=$SOCK_DIR/nbd"
> >   $QEMU_IMG map --output=json --image-opts \
> > -  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
> > +  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n2" | _filter_qemu_img_map
> >
> >   echo
> >   echo "=== End qemu NBD server ==="
> > @@ -199,15 +199,15 @@ echo
> >   echo "=== Use qemu-nbd as server ==="
> >   echo
> >
> > -nbd_server_start_unix_socket -r -f $IMGFMT -B b "$TEST_IMG"
> > -IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
> > +nbd_server_start_unix_socket -r -f $IMGFMT -x n -B b "$TEST_IMG"
> > +IMG="driver=nbd,export=n,server.type=unix,server.path=$nbd_unix_socket"
> >   $QEMU_IMG map --output=json --image-opts \
> > -  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b" | _filter_qemu_img_map
> > +  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n" | _filter_qemu_img_map
> >
> > -nbd_server_start_unix_socket -f $IMGFMT -B b2 "$TEST_IMG"
> > -IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
> > +nbd_server_start_unix_socket -f $IMGFMT -x n -B b2 "$TEST_IMG"
> > +IMG="driver=nbd,export=n,server.type=unix,server.path=$nbd_unix_socket"
> >   $QEMU_IMG map --output=json --image-opts \
> > -  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
> > +  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n" | _filter_qemu_img_map
> >
> >   # success, all done
> >   echo '*** done'
> > diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
> > index f175598802..9f879add60 100644
> > --- a/tests/qemu-iotests/223.out
> > +++ b/tests/qemu-iotests/223.out
> > @@ -61,7 +61,7 @@ exports available: 2
> >     max block: 33554432
> >     available meta contexts: 2
> >      base:allocation
> > -   qemu:dirty-bitmap:b
> > +   qemu:dirty-bitmap:n
> >    export: 'n2'
> >     size:  4194304
> >     flags: 0xced ( flush fua trim zeroes df cache fast-zero )
> > @@ -70,7 +70,7 @@ exports available: 2
> >     max block: 33554432
> >     available meta contexts: 2
> >      base:allocation
> > -   qemu:dirty-bitmap:b2
> > +   qemu:dirty-bitmap:n2
> >
> >   === Contrast normal status to large granularity dirty-bitmap ===
> >
> > @@ -141,7 +141,7 @@ exports available: 2
> >     max block: 33554432
> >     available meta contexts: 2
> >      base:allocation
> > -   qemu:dirty-bitmap:b
> > +   qemu:dirty-bitmap:n
> >    export: 'n2'
> >     size:  4194304
> >     flags: 0xced ( flush fua trim zeroes df cache fast-zero )
> > @@ -150,7 +150,7 @@ exports available: 2
> >     max block: 33554432
> >     available meta contexts: 2
> >      base:allocation
> > -   qemu:dirty-bitmap:b2
> > +   qemu:dirty-bitmap:n2
> >
> >   === Contrast normal status to large granularity dirty-bitmap ===
> >
> >
>
>
> --
> Best regards,
> Vladimir


Re: [PATCH] block: nbd: Fix dirty bitmap context name
Posted by Vladimir Sementsov-Ogievskiy 4 years, 3 months ago
19.12.2019 17:49, Nir Soffer wrote:
> On Thu, Dec 19, 2019 at 3:42 PM Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com> wrote:
>>
>> I'd not call it a "fix".. As it implies something broken.
>>
>> [edit: OK, now I see that something is broken, and why you called it "fix",
>>    see below]
>>
>> 19.12.2019 15:51, Nir Soffer wrote:
>>> When adding an export with a dirty bitmap, expose the bitmap at:
>>>
>>>       qemu:dirty-bitmap:export-name
>>
>> export-name? But it would be extra information, as client already knows
>> with which export it works.
> 
> Right, using empty string would be good as well.

It will conflict with already documented in docs/interop/nbd.txt:

* "qemu:dirty-bitmap:" - returns list of all available dirty-bitmap
                          metadata contexts.

> 
>> NBD commands NBD_OPT_GET/SET_META_CONTEXT includes export name as a
>> parameter, so, any queried metadata (bitmaps, etc) is always bound to
>> specified export.
>>
>>>
>>> This matches qapi documentation, and user expectations.
>>
>> Hmmm,
>> "qemu" namespace is documented in docs/interop/nbd.txt, not in Qapi,
>> which is also mention in official NBD spec.
>>
>>
>> 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 <eblake@redhat.com>
>> 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.
>>
>>
>>>
>>> Without this, qemu leaks libvirt implementations details to clients by
>>> exposing the bitmap using the actual bitmap name:
>>>
>>>       qemu:dirty-bitmap:bitmap-name
>>
>> Yes, "qemu" namespace specification says:
>> qemu:dirty-bitmap:<dirty-bitmap-export-name>
>>
>> so, <dirty-bitmap-export-name> may be exact bitmap name or may be something other.
>>
>> We just don't have an interface to set such name. It was in removed
>> x-nbd-server-add-bitmap
>>
>> So, if you need this possibility now, the correct way is to add 'export-bitmap-name'
>> optional parameter to nbd-server-add, like it was in x-nbd-server-add-bitmap
> 
> I don't think we need such API. How would it help users trying to get
> dirty extents
> from an image?

It will solve the issue, which you mentioned in your commit message:
"qemu leaks libvirt implementations details"

> 
>>> And all clients need to duplicate code like:
>>>
>>>       meta_context = "qemu:dirty-bitmap:backup-" + export_name
>>>
>>> NBD allows exposing multiple bitmaps under "qemu:dirty-bitmap:"
>>> namespace, and clients can query the available bitmaps, but it is not
>>> clear what a client should do if a server provides multiple bitmaps.
>>> ---
>>>    nbd/server.c               |  2 +-
>>>    tests/qemu-iotests/223     | 16 ++++++++--------
>>>    tests/qemu-iotests/223.out |  8 ++++----
>>>    3 files changed, 13 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/nbd/server.c b/nbd/server.c
>>> index 24ebc1a805..f20f2994c0 100644
>>> --- a/nbd/server.c
>>> +++ b/nbd/server.c
>>> @@ -1574,7 +1574,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>>>            exp->export_bitmap = bm;
>>>            assert(strlen(bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
>>>            exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
>>> -                                                     bitmap);
>>> +                                                     name);
>>
>> I think it's a bad idea to automatically name bitmap after export. Actually export may
>> have several bitmaps (we just don't support it).
> 
> What are the semantics of multiple dirty bitmaps for same export? How
> users are going
> to use this?

semantic of the protocol defined in docs/interop/nbd.txt:

     ...
     The "qemu" namespace currently contains only one type of context,
     related to exposing the contents of a dirty bitmap alongside the
     associated disk contents.  That context has the following form:

         qemu:dirty-bitmap:<dirty-bitmap-export-name>

     ...

We may export several bitmaps. Still, currently Qemu can export only one bitmap.

> 
>>   "NAME" in Qapi spec is a mistake.
>>
>>>            assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
>>>        }
>>>
>>> diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
>>> index ea69cd4b8b..3068a7c280 100755
>>> --- a/tests/qemu-iotests/223
>>> +++ b/tests/qemu-iotests/223
>>> @@ -167,7 +167,7 @@ $QEMU_IO -r -c 'r -P 0x22 512 512' -c 'r -P 0 512k 512k' -c 'r -P 0x11 1m 1m' \
>>>    $QEMU_IMG map --output=json --image-opts \
>>>      "$IMG" | _filter_qemu_img_map
>>>    $QEMU_IMG map --output=json --image-opts \
>>> -  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b" | _filter_qemu_img_map
>>> +  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n" | _filter_qemu_img_map
>>>
>>>    echo
>>>    echo "=== Contrast to small granularity dirty-bitmap ==="
>>> @@ -175,7 +175,7 @@ echo
>>>
>>>    IMG="driver=nbd,export=n2,server.type=unix,server.path=$SOCK_DIR/nbd"
>>>    $QEMU_IMG map --output=json --image-opts \
>>> -  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
>>> +  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n2" | _filter_qemu_img_map
>>>
>>>    echo
>>>    echo "=== End qemu NBD server ==="
>>> @@ -199,15 +199,15 @@ echo
>>>    echo "=== Use qemu-nbd as server ==="
>>>    echo
>>>
>>> -nbd_server_start_unix_socket -r -f $IMGFMT -B b "$TEST_IMG"
>>> -IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
>>> +nbd_server_start_unix_socket -r -f $IMGFMT -x n -B b "$TEST_IMG"
>>> +IMG="driver=nbd,export=n,server.type=unix,server.path=$nbd_unix_socket"
>>>    $QEMU_IMG map --output=json --image-opts \
>>> -  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b" | _filter_qemu_img_map
>>> +  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n" | _filter_qemu_img_map
>>>
>>> -nbd_server_start_unix_socket -f $IMGFMT -B b2 "$TEST_IMG"
>>> -IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
>>> +nbd_server_start_unix_socket -f $IMGFMT -x n -B b2 "$TEST_IMG"
>>> +IMG="driver=nbd,export=n,server.type=unix,server.path=$nbd_unix_socket"
>>>    $QEMU_IMG map --output=json --image-opts \
>>> -  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
>>> +  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n" | _filter_qemu_img_map
>>>
>>>    # success, all done
>>>    echo '*** done'
>>> diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
>>> index f175598802..9f879add60 100644
>>> --- a/tests/qemu-iotests/223.out
>>> +++ b/tests/qemu-iotests/223.out
>>> @@ -61,7 +61,7 @@ exports available: 2
>>>      max block: 33554432
>>>      available meta contexts: 2
>>>       base:allocation
>>> -   qemu:dirty-bitmap:b
>>> +   qemu:dirty-bitmap:n
>>>     export: 'n2'
>>>      size:  4194304
>>>      flags: 0xced ( flush fua trim zeroes df cache fast-zero )
>>> @@ -70,7 +70,7 @@ exports available: 2
>>>      max block: 33554432
>>>      available meta contexts: 2
>>>       base:allocation
>>> -   qemu:dirty-bitmap:b2
>>> +   qemu:dirty-bitmap:n2
>>>
>>>    === Contrast normal status to large granularity dirty-bitmap ===
>>>
>>> @@ -141,7 +141,7 @@ exports available: 2
>>>      max block: 33554432
>>>      available meta contexts: 2
>>>       base:allocation
>>> -   qemu:dirty-bitmap:b
>>> +   qemu:dirty-bitmap:n
>>>     export: 'n2'
>>>      size:  4194304
>>>      flags: 0xced ( flush fua trim zeroes df cache fast-zero )
>>> @@ -150,7 +150,7 @@ exports available: 2
>>>      max block: 33554432
>>>      available meta contexts: 2
>>>       base:allocation
>>> -   qemu:dirty-bitmap:b2
>>> +   qemu:dirty-bitmap:n2
>>>
>>>    === Contrast normal status to large granularity dirty-bitmap ===
>>>
>>>
>>
>>
>> --
>> Best regards,
>> Vladimir
> 


-- 
Best regards,
Vladimir