On Thu, Dec 19, 2019 at 3:42 PM Vladimir Sementsov-Ogievskiy
<vsement...@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 <ebl...@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
